| Summary: | Nullptr crash in DeleteSelectionCommand::removeNode | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
| Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, svillar, webkit-bug-importer, wenson_hsieh | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Ryosuke Niwa
2021-04-21 13:57:38 PDT
The crash is in DeleteSelectionCommand::removeNode() in line:
if (!node.parentNode()->hasEditableStyle()) {
because parentNode() is nullptr. This is happening because CompositeEditCommand::removeNode() is called twice for both the embed element and the input element. The second time it's called for the input element, it has already been removed from the tree, so parent is nullptr. And the reason why it's called twice for the same nodes is the weird thing here. There are two calls to InsertText, but the second one happens before the first one has finished, so the tree is in inconsistent state. I suspect it might be a bug in WTR because that doesn't happen when running in MiniBrowser. From MiniBrowser the second InsertText command is executed after the first one (as expected) and doesn't clear the selection, because it was already cleared by the previous one. Note that it doesn't happen from WTR if we add a waitUntilDone() at the end of the script and call notifyDone() after the InsertText in the timeout function.
(In reply to Carlos Garcia Campos from comment #1) > The crash is in DeleteSelectionCommand::removeNode() in line: > > if (!node.parentNode()->hasEditableStyle()) { > > because parentNode() is nullptr. This is happening because > CompositeEditCommand::removeNode() is called twice for both the embed > element and the input element. The second time it's called for the input > element, it has already been removed from the tree, so parent is nullptr. > And the reason why it's called twice for the same nodes is the weird thing > here. There are two calls to InsertText, but the second one happens before > the first one has finished, so the tree is in inconsistent state. That happens all the time with editing tests because various editing code triggers a synchronous layout update, which can end up executing scripts. > I suspect it might be a bug in WTR because that doesn't happen when running in > MiniBrowser. From MiniBrowser the second InsertText command is executed > after the first one (as expected) and doesn't clear the selection, because > it was already cleared by the previous one. Note that it doesn't happen from > WTR if we add a waitUntilDone() at the end of the script and call > notifyDone() after the InsertText in the timeout function. Hm... maybe? If it's the timing of when notifyDone is called, then I'm not sure if there is an easy way to fix that since a lot of tests depend on the exact timing of notifyDone. It's not exactly an problem of when notifyDone is called, current test doesn't call notifyDone at all, since a timer is used, the timer is executed after the page has loaded. WTR finishes the test on page loaded when there's no waitUntilDone/notifyDone. (In reply to Carlos Garcia Campos from comment #3) > It's not exactly an problem of when notifyDone is called, current test > doesn't call notifyDone at all, since a timer is used, the timer is executed > after the page has loaded. WTR finishes the test on page loaded when there's > no waitUntilDone/notifyDone. Ok, then I don't think this is an issue with WTR. It's well understood that invoking execCommand can invoke another execCommand in the middle of it. What makes the difference in WTR is the force repaint that happens before dumping the results. That's the one causing the second InsertText to be executed while the first one hasn't finished yet. Created attachment 427253 [details]
Patch
Comment on attachment 427253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427253&action=review > Source/WebCore/editing/DeleteSelectionCommand.cpp:515 > + if (!node.nonShadowBoundaryParentNode()) > + return; Why nonShadowBoundaryParentNode() instead of parentNode()? (In reply to Ryosuke Niwa from comment #7) > Comment on attachment 427253 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427253&action=review > > > Source/WebCore/editing/DeleteSelectionCommand.cpp:515 > > + if (!node.nonShadowBoundaryParentNode()) > > + return; > > Why nonShadowBoundaryParentNode() instead of parentNode()? It's what CompositeEditCommand::removeNode() does, so I assumed that's what we want here too. (In reply to Carlos Garcia Campos from comment #8) > (In reply to Ryosuke Niwa from comment #7) > > Comment on attachment 427253 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review > > > > > Source/WebCore/editing/DeleteSelectionCommand.cpp:515 > > > + if (!node.nonShadowBoundaryParentNode()) > > > + return; > > > > Why nonShadowBoundaryParentNode() instead of parentNode()? > > It's what CompositeEditCommand::removeNode() does, so I assumed that's what > we want here too. Hm... the only difference will be that this will return nullptr when the parent is a shadow root. I don't think that'll make a difference here. That function was introduced back when Google was first implementing the shadow DOM API as a blanket replacement for parentNode to avoid issues in the editing code. Comment on attachment 427253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427253&action=review > Source/WebCore/ChangeLog:7 > + This is not a security bug, right? Can we add a test? (In reply to Ryosuke Niwa from comment #9) > (In reply to Carlos Garcia Campos from comment #8) > > (In reply to Ryosuke Niwa from comment #7) > > > Comment on attachment 427253 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review > > > > > > > Source/WebCore/editing/DeleteSelectionCommand.cpp:515 > > > > + if (!node.nonShadowBoundaryParentNode()) > > > > + return; > > > > > > Why nonShadowBoundaryParentNode() instead of parentNode()? > > > > It's what CompositeEditCommand::removeNode() does, so I assumed that's what > > we want here too. > > Hm... the only difference will be that this will return nullptr when the > parent is a shadow root. I don't think that'll make a difference here. That > function was introduced back when Google was first implementing the shadow > DOM API as a blanket replacement for parentNode to avoid issues in the > editing code. Ok, I'll use parentNode() then. (In reply to Ryosuke Niwa from comment #10) > Comment on attachment 427253 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427253&action=review > > > Source/WebCore/ChangeLog:7 > > + > > This is not a security bug, right? Can we add a test? It's a use after free (In reply to Carlos Garcia Campos from comment #12) > (In reply to Ryosuke Niwa from comment #10) > > Comment on attachment 427253 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review > > > > > Source/WebCore/ChangeLog:7 > > > + > > > > This is not a security bug, right? Can we add a test? > > It's a use after free I'm not sure why though, but looking at the backtrace the hasEditableStyle() is called. Created attachment 427331 [details]
Patch
(In reply to Carlos Garcia Campos from comment #13) > (In reply to Carlos Garcia Campos from comment #12) > > (In reply to Ryosuke Niwa from comment #10) > > > Comment on attachment 427253 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review > > > > > > > Source/WebCore/ChangeLog:7 > > > > + > > > > > > This is not a security bug, right? Can we add a test? > > > > It's a use after free > > I'm not sure why though, but looking at the backtrace the hasEditableStyle() > is called. Could you figure out why it's a use-after-free? We probably need to put more Ref/RefPtr somewhere. (In reply to Ryosuke Niwa from comment #15) > (In reply to Carlos Garcia Campos from comment #13) > > (In reply to Carlos Garcia Campos from comment #12) > > > (In reply to Ryosuke Niwa from comment #10) > > > > Comment on attachment 427253 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review > > > > > > > > > Source/WebCore/ChangeLog:7 > > > > > + > > > > > > > > This is not a security bug, right? Can we add a test? > > > > > > It's a use after free > > > > I'm not sure why though, but looking at the backtrace the hasEditableStyle() > > is called. > > Could you figure out why it's a use-after-free? We probably need to put more > Ref/RefPtr somewhere. In my case the bt finishes in: #0 0x00007f0d04748db0 in WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, WebCore::Node::ShouldUpdateStyle) const () it doesn't get that far, so the node isn't actually used. Maybe it depends on the compiler? (In reply to Carlos Garcia Campos from comment #16) > > > Could you figure out why it's a use-after-free? We probably need to put more > > Ref/RefPtr somewhere. > > In my case the bt finishes in: > > #0 0x00007f0d04748db0 in > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, > WebCore::Node::ShouldUpdateStyle) const () > > it doesn't get that far, so the node isn't actually used. Maybe it depends > on the compiler? So presumably you're seeing an ASAN failures? Could you post the full ASAN stack? (In reply to Ryosuke Niwa from comment #17) > (In reply to Carlos Garcia Campos from comment #16) > > > > > Could you figure out why it's a use-after-free? We probably need to put more > > > Ref/RefPtr somewhere. > > > > In my case the bt finishes in: > > > > #0 0x00007f0d04748db0 in > > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, > > WebCore::Node::ShouldUpdateStyle) const () > > > > it doesn't get that far, so the node isn't actually used. Maybe it depends > > on the compiler? > > So presumably you're seeing an ASAN failures? Could you post the full ASAN > stack? Aha, that must be the difference, my build is a normal release build, not asan. (In reply to Carlos Garcia Campos from comment #18) > (In reply to Ryosuke Niwa from comment #17) > > (In reply to Carlos Garcia Campos from comment #16) > > > > > > > Could you figure out why it's a use-after-free? We probably need to put more > > > > Ref/RefPtr somewhere. > > > > > > In my case the bt finishes in: > > > > > > #0 0x00007f0d04748db0 in > > > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, > > > WebCore::Node::ShouldUpdateStyle) const () > > > > > > it doesn't get that far, so the node isn't actually used. Maybe it depends > > > on the compiler? > > > > So presumably you're seeing an ASAN failures? Could you post the full ASAN > > stack? > > Aha, that must be the difference, my build is a normal release build, not > asan. If you're using non-ASAN builds, how did you determine that this is a use-after-free? (In reply to Ryosuke Niwa from comment #19) > (In reply to Carlos Garcia Campos from comment #18) > > (In reply to Ryosuke Niwa from comment #17) > > > (In reply to Carlos Garcia Campos from comment #16) > > > > > > > > > Could you figure out why it's a use-after-free? We probably need to put more > > > > > Ref/RefPtr somewhere. > > > > > > > > In my case the bt finishes in: > > > > > > > > #0 0x00007f0d04748db0 in > > > > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, > > > > WebCore::Node::ShouldUpdateStyle) const () > > > > > > > > it doesn't get that far, so the node isn't actually used. Maybe it depends > > > > on the compiler? > > > > > > So presumably you're seeing an ASAN failures? Could you post the full ASAN > > > stack? > > > > Aha, that must be the difference, my build is a normal release build, not > > asan. > > If you're using non-ASAN builds, how did you determine that this is a > use-after-free? Because fo the backtrace you posted in bug description. (In reply to Carlos Garcia Campos from comment #20) > (In reply to Ryosuke Niwa from comment #19) > > (In reply to Carlos Garcia Campos from comment #18) > > > (In reply to Ryosuke Niwa from comment #17) > > > > (In reply to Carlos Garcia Campos from comment #16) > > > > > > > > > > > Could you figure out why it's a use-after-free? We probably need to put more > > > > > > Ref/RefPtr somewhere. > > > > > > > > > > In my case the bt finishes in: > > > > > > > > > > #0 0x00007f0d04748db0 in > > > > > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, > > > > > WebCore::Node::ShouldUpdateStyle) const () > > > > > > > > > > it doesn't get that far, so the node isn't actually used. Maybe it depends > > > > > on the compiler? > > > > > > > > So presumably you're seeing an ASAN failures? Could you post the full ASAN > > > > stack? > > > > > > Aha, that must be the difference, my build is a normal release build, not > > > asan. > > > > If you're using non-ASAN builds, how did you determine that this is a > > use-after-free? > > Because of the backtrace you posted in bug description. I don't understand. I don't think there is any indiction that this is a UAF based on the backtrace. We're crashing here: void DeleteSelectionCommand::removeNode(Node& node, ShouldAssumeContentIsAlwaysEditable shouldAssumeContentIsAlwaysEditable) { Ref<Node> protectedNode = node; if (m_startRoot != m_endRoot && !(node.isDescendantOf(m_startRoot.get()) && node.isDescendantOf(m_endRoot.get()))) { // If a node is not in both the start and end editable roots, remove it only if its inside an editable region. if (!node.parentNode()->hasEditableStyle()) { // <- this line. There is protectedNode which refs this node. Here, the issue is that parentNode() is null. treeScope() is where we crash since that's the first place in computeEditability that dereferences "this" inside document(). Node::Editability Node::computeEditability(UserSelectAllTreatment treatment, ShouldUpdateStyle shouldUpdateStyle) const { if (!document().hasLivingRenderTree() || isPseudoElement()) return Editability::ReadOnly; Comment on attachment 427331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427331&action=review > Source/WebCore/ChangeLog:7 > + At this point, I'm pretty sure there is no use-after-free here. Please add a test. Ok, the backtrace confused me, sorry. The problem with the test is that this is not reproducible when using waitUntilDone/notifyDone, because it's the UI process the one triggering the force repaint at a very specific time when he load finishes. If we don't use the waitUntilDone, with the crash fixed, the timeout will be executed after the test has finished, so I wonder if it might affect the next test run. (In reply to Carlos Garcia Campos from comment #23) > Ok, the backtrace confused me, sorry. The problem with the test is that this > is not reproducible when using waitUntilDone/notifyDone, because it's the UI > process the one triggering the force repaint at a very specific time when he > load finishes. If we don't use the waitUntilDone, with the crash fixed, the > timeout will be executed after the test has finished, so I wonder if it > might affect the next test run. Can we add two test files ~-1.html & ~-2.html, and add a comment saying that ~-1.html passes if we didn't hit a crash in ~-2.html? (In reply to Ryosuke Niwa from comment #24) > (In reply to Carlos Garcia Campos from comment #23) > > Ok, the backtrace confused me, sorry. The problem with the test is that this > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI > > process the one triggering the force repaint at a very specific time when he > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the > > timeout will be executed after the test has finished, so I wonder if it > > might affect the next test run. > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that > ~-1.html passes if we didn't hit a crash in ~-2.html? I'm not sure I get how that could work. How does WTR know it has run two files? (In reply to Carlos Garcia Campos from comment #25) > (In reply to Ryosuke Niwa from comment #24) > > (In reply to Carlos Garcia Campos from comment #23) > > > Ok, the backtrace confused me, sorry. The problem with the test is that this > > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI > > > process the one triggering the force repaint at a very specific time when he > > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the > > > timeout will be executed after the test has finished, so I wonder if it > > > might affect the next test run. > > > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that > > ~-1.html passes if we didn't hit a crash in ~-2.html? > > I'm not sure I get how that could work. How does WTR know it has run two > files? Tests are always ordered in the lexicological order. (In reply to Ryosuke Niwa from comment #26) > (In reply to Carlos Garcia Campos from comment #25) > > (In reply to Ryosuke Niwa from comment #24) > > > (In reply to Carlos Garcia Campos from comment #23) > > > > Ok, the backtrace confused me, sorry. The problem with the test is that this > > > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI > > > > process the one triggering the force repaint at a very specific time when he > > > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the > > > > timeout will be executed after the test has finished, so I wonder if it > > > > might affect the next test run. > > > > > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that > > > ~-1.html passes if we didn't hit a crash in ~-2.html? > > > > I'm not sure I get how that could work. How does WTR know it has run two > > files? > > Tests are always ordered in the lexicological order. But you should be able to run tests individually. They are supposed to be independent from each other (In reply to Carlos Garcia Campos from comment #27) > (In reply to Ryosuke Niwa from comment #26) > > (In reply to Carlos Garcia Campos from comment #25) > > > (In reply to Ryosuke Niwa from comment #24) > > > > (In reply to Carlos Garcia Campos from comment #23) > > > > > Ok, the backtrace confused me, sorry. The problem with the test is that this > > > > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI > > > > > process the one triggering the force repaint at a very specific time when he > > > > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the > > > > > timeout will be executed after the test has finished, so I wonder if it > > > > > might affect the next test run. > > > > > > > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that > > > > ~-1.html passes if we didn't hit a crash in ~-2.html? > > > > > > I'm not sure I get how that could work. How does WTR know it has run two > > > files? > > > > Tests are always ordered in the lexicological order. > > But you should be able to run tests individually. They are supposed to be > independent from each other Yeah, so it's not great but we may not have much choice in this case. It's definitely a lot better than not adding a test. Have you tried running testRunner.display() ? (In reply to Ryosuke Niwa from comment #28) > (In reply to Carlos Garcia Campos from comment #27) > > (In reply to Ryosuke Niwa from comment #26) > > > (In reply to Carlos Garcia Campos from comment #25) > > > > (In reply to Ryosuke Niwa from comment #24) > > > > > (In reply to Carlos Garcia Campos from comment #23) > > > > > > Ok, the backtrace confused me, sorry. The problem with the test is that this > > > > > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI > > > > > > process the one triggering the force repaint at a very specific time when he > > > > > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the > > > > > > timeout will be executed after the test has finished, so I wonder if it > > > > > > might affect the next test run. > > > > > > > > > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that > > > > > ~-1.html passes if we didn't hit a crash in ~-2.html? > > > > > > > > I'm not sure I get how that could work. How does WTR know it has run two > > > > files? > > > > > > Tests are always ordered in the lexicological order. > > > > But you should be able to run tests individually. They are supposed to be > > independent from each other > > Yeah, so it's not great but we may not have much choice in this case. It's > definitely a lot better than not adding a test. Have you tried running > testRunner.display() ? Yes, I tried that, at different places, but when triggered from the web process, it's always done when the InsertText command has completed. (In reply to Carlos Garcia Campos from comment #29) > (In reply to Ryosuke Niwa from comment #28) > > (In reply to Carlos Garcia Campos from comment #27) > > > > Yeah, so it's not great but we may not have much choice in this case. It's > > definitely a lot better than not adding a test. Have you tried running > > testRunner.display() ? > > Yes, I tried that, at different places, but when triggered from the web > process, it's always done when the InsertText command has completed. Yeah, so in that case, we probably just need to add two files. I have debugged this further. It's not the UI process causing the force repaint, it happens inside the web process when the main resource load finishes. And the main load finish si caused by the embed element removal caused by the delete selection command. During the command executions, there's an event scope, so we can't add a listener for DOMNodeRemoved to force the repaint at that point. I think this bug is impossible to reproduce outside of WTR. What we can do to make a test is to add testRunner API to tell WTR to force a repaint on load finished. Created attachment 427878 [details]
Patch
Comment on attachment 427878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427878&action=review > Source/WebCore/ChangeLog:8 > + Test: editing/inserting/insert-text-force-repaint-on-load-crash.html Skip the test on WK1 maybe? Alternatively, you can call testRunner.displayOnLoadFinish conditionally. Created attachment 427992 [details]
Patch for landing
Downloading mechanize-0.4.5... Installing mechanize-0.4.5... Installed mechanize-0.4.5! ChangeLog entry in Tools/ChangeLog contains OOPS!. Committed r277202 (237474@main): <https://commits.webkit.org/237474@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427992 [details]. |