Bug 234862 - null ptr deref in WebCore::ModifySelectionListLevelCommand::appendSiblingNodeRange
Summary: null ptr deref in WebCore::ModifySelectionListLevelCommand::appendSiblingNode...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gabriel Nava Marino
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-04 14:27 PST by Gabriel Nava Marino
Modified: 2022-01-10 10:26 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.90 KB, patch)
2022-01-04 14:43 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2022-01-04 17:07 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (3.82 KB, patch)
2022-01-04 17:08 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2022-01-04 17:53 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (9.34 KB, patch)
2022-01-06 09:49 PST, Gabriel Nava Marino
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (8.15 KB, patch)
2022-01-06 17:49 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (8.17 KB, patch)
2022-01-06 20:59 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Nava Marino 2022-01-04 14:27:48 PST
In some instances, the startNode might not have any siblings.
Comment 1 Gabriel Nava Marino 2022-01-04 14:31:04 PST
<rdar://problem/86567729>
Comment 2 Gabriel Nava Marino 2022-01-04 14:43:05 PST
Created attachment 448336 [details]
Patch
Comment 3 Gabriel Nava Marino 2022-01-04 17:07:09 PST
Created attachment 448350 [details]
Patch
Comment 4 Gabriel Nava Marino 2022-01-04 17:08:07 PST
Created attachment 448351 [details]
Patch
Comment 5 Gabriel Nava Marino 2022-01-04 17:53:22 PST
Created attachment 448355 [details]
Patch
Comment 6 Gabriel Nava Marino 2022-01-04 17:54:37 PST
Removed dependence on the webkitSpeechRecognition API in the test that was added, since it is not needed to reproduce the issue.
Comment 7 Gabriel Nava Marino 2022-01-06 09:49:39 PST
Created attachment 448507 [details]
Patch
Comment 8 Darin Adler 2022-01-06 10:02:12 PST
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.
Comment 9 Gabriel Nava Marino 2022-01-06 16:12:30 PST
(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().
Comment 10 Gabriel Nava Marino 2022-01-06 17:49:51 PST
Created attachment 448551 [details]
Patch
Comment 11 Gabriel Nava Marino 2022-01-06 20:59:18 PST
Created attachment 448558 [details]
Patch
Comment 12 Darin Adler 2022-01-08 10:17:07 PST
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.
Comment 13 EWS 2022-01-08 10:23:03 PST
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].
Comment 14 Gabriel Nava Marino 2022-01-10 10:07:48 PST
(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.
Comment 15 Gabriel Nava Marino 2022-01-10 10:26:42 PST
(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.