Bug 224518 – Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ian Gilbert]

removeRedundantBlocks() may run into a null pointer while iterating over the tree, if the tree is unexpectedly modified during the execution of a command. Adding null check to quit this step if a parent node cannot be resolved.

[Ian Gilbert]

<rdar://problem/76264118>

[Ian Gilbert]

Created attachment 425933 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 425933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425933&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:869
> +    RefPtr<Node> node = makeRefPtr(m_endingPosition.containerNode());
> +    RefPtr<Node> rootNode = makeRefPtr(node->rootEditableElement());

Use auto?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:871
> +    while (node && (node != rootNode)) {

We don't usually nest parentheses like this.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:877
> -            node = m_endingPosition.anchorNode();
> +            node = makeRefPtr(m_endingPosition.anchorNode());

No need to call makeRefPtr since node is already of type RefPtr<Node>.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:879
> -            node = node->parentNode();
> +            node = makeRefPtr(node->parentNode());

Ditto.

> LayoutTests/editing/execCommand/remove-node-during-command-crash.html:24
> +    document.write('PASS')

Missing semicolon at the end.
Also, can we say that this test passes if WebKit doesn't crash?

[Ian Gilbert]

Created attachment 426154 [details]
Patch

[Ian Gilbert]

Tried to add the text to the test body saying the test passes if it doesn't crash but it either would stop the test from reproducing, or would get eaten by the command. Hopefully that is appropriate.

[Ryosuke Niwa]

Comment on attachment 426154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426154&action=review

> Source/WebCore/ChangeLog:3
> +        Editing null pointer dereference while resolving command

I've just renamed the bug. Please update the title here. It's overly generic.
Sorry I didn't notice earlier.

> LayoutTests/ChangeLog:3
> +        Editing null pointer dereference while resolving command

Ditto.

> LayoutTests/ChangeLog:8
> +        Add regression test.

Nit: Add *a* regression test.

[Ian Gilbert]

Created attachment 426267 [details]
Patch

[Ryosuke Niwa]

Let's at least wait for the stress EWS to make sure the new test isn't flaky, etc...

[EWS]

Committed r276186 (236668@main): <https://commits.webkit.org/236668@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426267 [details].