Bug 234862 – null ptr deref in WebCore::ModifySelectionListLevelCommand::appendSiblingNodeRange

• Component: New Bugs
• Status: RESOLVED FIXED

[Gabriel Nava Marino]

In some instances, the startNode might not have any siblings.

[Gabriel Nava Marino]

<rdar://problem/86567729>

[Gabriel Nava Marino]

Created attachment 448336 [details]
Patch

[Gabriel Nava Marino]

Created attachment 448350 [details]
Patch

[Gabriel Nava Marino]

Created attachment 448351 [details]
Patch

[Gabriel Nava Marino]

Created attachment 448355 [details]
Patch

[Gabriel Nava Marino]

Removed dependence on the webkitSpeechRecognition API in the test that was added, since it is not needed to reproduce the issue.

[Gabriel Nava Marino]

Created attachment 448507 [details]
Patch

[Darin Adler]

Comment on attachment 448507 [details]
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.

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #8)
> Comment on attachment 448507 [details]
> 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.

Thank you for the feedback. I have applied all of these changes locally and will include them in the updated patch.

I am investigating EWS failures related to the proposed changes in InsertListCommand::doApply().

[Gabriel Nava Marino]

Created attachment 448551 [details]
Patch

[Gabriel Nava Marino]

Created attachment 448558 [details]
Patch

[Darin Adler]

Comment on attachment 448558 [details]
Patch

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

> Source/WebCore/editing/VisibleUnits.cpp:1199
> +        if (r->isBR() || is<HTMLBRElement>(n) || isBlock(n))

Is there a correctness implication here? Can a <br> element be styled so it is inline and not a block? Just want to be cautious about testing only the crashing aspect of this.

[EWS]

Committed r287812 (245864@main): <https://commits.webkit.org/245864@main>

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

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #12)
> Comment on attachment 448558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448558&action=review
> 
> > Source/WebCore/editing/VisibleUnits.cpp:1199
> > +        if (r->isBR() || is<HTMLBRElement>(n) || isBlock(n))
> 
> Is there a correctness implication here? Can a <br> element be styled so it
> is inline and not a block? Just want to be cautious about testing only the
> crashing aspect of this.

Thank you for pointing this out. Upon calling 'CompositeEditCommand::moveParagraph' I noticed the node's RenderObject bitfield m_isLineBreak changed from a 1 to a 0, as well as other flags, hence the need for is<HTMLBRElement>(n), as r->isBR() will return false. It appears there is some style state change going on here which might also impact isBlock(n). I will check whether this state change is an invalid state.

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #12)
> Comment on attachment 448558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448558&action=review
> 
> > Source/WebCore/editing/VisibleUnits.cpp:1199
> > +        if (r->isBR() || is<HTMLBRElement>(n) || isBlock(n))
> 
> Is there a correctness implication here? Can a <br> element be styled so it
> is inline and not a block? Just want to be cautious about testing only the
> crashing aspect of this.

I received some further clarification: <br> element is by default inline.