[webkit-reviews] review granted: [Bug 234862] null ptr deref in WebCore::ModifySelectionListLevelCommand::appendSiblingNodeRange : [Attachment 448507] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gabriel Nava Marino
<gnavamarino at apple.com>'s request for review:
Bug 234862: null ptr deref in
WebCore::ModifySelectionListLevelCommand::appendSiblingNodeRange
https://bugs.webkit.org/show_bug.cgi?id=234862

Attachment 448507: Patch

https://bugs.webkit.org/attachment.cgi?id=448507&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 448507
  --> https://bugs.webkit.org/attachment.cgi?id=448507
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:-203
> +	   auto range = endingSelection().firstRange();
> +	   doApplyForSingleParagraph(false, listTag, *range);
>      }
> -
> -    auto range = endingSelection().firstRange();
> -    doApplyForSingleParagraph(false, listTag, *range);

Looks like a good fix.

> Source/WebCore/editing/ModifySelectionListLevel.cpp:97
>      while (1) {
> +	   ASSERT(node);

Here I would do something other than adding this assertion.

I see no harm in having these functions stop iterating if the node is null.
Does not seem critical to crash rather than returning in that case. It’s fine
to assert so we can catch mistakes, but also just might want to change this to
be:

    while (node) {

instead of while (1). I don’t see how we have a guarantee that we’ll always
reach endNode before null, especially if any mutation is happening during the
loops.

I also think these functions should be using RefPtr for the nodes, not raw
pointers. This code is written as if there was a guarantee that removeNode and
insertNodeAfter have no side effects, and there’s no reason to have such a
risky approach to object lifetimes. Like this:

    RefPtr node = startNode;
    RefPtr next = node->nextSibling();

We could also use "return" rather than "break" to end the loop so we can put an
"assert not reached" after the loop.

All of this applies to the other functions too.