Bug 218484 – Nullptr crash in RenderObject::parent

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ian Gilbert]

Created attachment 413015 [details]
Crashing input

Description:
Crash found by fuzzing. Reproduces on WebKit revision 268263

Stack Trace
=========

frame #0: WebCore.framework/Versions/A/WebCore`WebCore::RenderObject::parent() const+
frame #1: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::Block::attachIgnoringContinuation(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+
frame #2: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::Block::attach(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+
frame #3: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::BlockFlow::attach(WebCore::RenderBlockFlow&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+
frame #4: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::attachInternal(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+
frame #5: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::attach(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+
frame #6: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::createTextRenderer(WebCore::Text&, WebCore::Style::TextUpdate const*)+
frame #7: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::updateTextRenderer(WebCore::Text&, WebCore::Style::TextUpdate const*)+
frame #8: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&)+
frame #9: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >)+
frame #10: WebCore.framework/Versions/A/WebCore`WebCore::Document::updateRenderTree(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >)+

[Radar WebKit Bug Importer]

<rdar://problem/70985057>

[Ryosuke Niwa]

<rdar://problem/70583715>

[Sergio Villar Senin]

Created attachment 414025 [details]
Super-reduced test case

[Sergio Villar Senin]

Created attachment 414052 [details]
Patch

[Geoffrey Garen]

Comment on attachment 414052 [details]
Patch

r=me

[Ryosuke Niwa]

Comment on attachment 414052 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:59
> +    auto* parentNode = node.parentNode();

Please use RefPtr.

> Source/WebCore/editing/InsertListCommand.cpp:60
> +    if (parentNode && !parentNode->hasEditableStyle())

It seems like we should be checking hasRichlyEditableStyle() instead then?

> Source/WebCore/editing/InsertListCommand.cpp:214
>      Node* listChildNode = enclosingListChild(selectionNode);

Can we make these pointers RefPtr?

[Sergio Villar Senin]

Comment on attachment 414052 [details]
Patch

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

Thanks for the review.

>> Source/WebCore/editing/InsertListCommand.cpp:59
>> +    auto* parentNode = node.parentNode();
> 
> Please use RefPtr.

OK

>> Source/WebCore/editing/InsertListCommand.cpp:60
>> +    if (parentNode && !parentNode->hasEditableStyle())
> 
> It seems like we should be checking hasRichlyEditableStyle() instead then?

Maybe the ChangeLog text is a bit misleading but this is the actual condition used by InsertNodeBeforeCommand  ASSERT.

>> Source/WebCore/editing/InsertListCommand.cpp:214
>>      Node* listChildNode = enclosingListChild(selectionNode);
> 
> Can we make these pointers RefPtr?

Should we do it in a follow-up? Seems not directly related to the bug.

[Darin Adler]

Comment on attachment 414052 [details]
Patch

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

>>> Source/WebCore/editing/InsertListCommand.cpp:59
>>> +    auto* parentNode = node.parentNode();
>> 
>> Please use RefPtr.
> 
> OK

Side comment: The idiom I prefer for RefPtr in case case like this one is this:

    auto parentNode = makeRefPtr(node.parentNode());

[Ryosuke Niwa]

Comment on attachment 414052 [details]
Patch

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

>>> Source/WebCore/editing/InsertListCommand.cpp:60
>>> +    if (parentNode && !parentNode->hasEditableStyle())
>> 
>> It seems like we should be checking hasRichlyEditableStyle() instead then?
> 
> Maybe the ChangeLog text is a bit misleading but this is the actual condition used by InsertNodeBeforeCommand  ASSERT.

Well, we shouldn't be listifying in plain text mode so we need to be checking hasRichlyEditableStyle.
The existence of other code which checks for a wrong condition isn't a good argument for introducing new wrong code.

[Ryosuke Niwa]

(In reply to Sergio Villar Senin from comment #7)
> Comment on attachment 414052 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414052&action=review
>
> >> Source/WebCore/editing/InsertListCommand.cpp:214
> >>      Node* listChildNode = enclosingListChild(selectionNode);
> > 
> > Can we make these pointers RefPtr?
> 
> Should we do it in a follow-up? Seems not directly related to the bug.

Sure, that sounds good.

[Ryosuke Niwa]

Okay, there doesn't seem to any security bug here so can we include the test case in the patch?

[Sergio Villar Senin]

(In reply to Ryosuke Niwa from comment #9)
> Comment on attachment 414052 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414052&action=review
> 
> >>> Source/WebCore/editing/InsertListCommand.cpp:60
> >>> +    if (parentNode && !parentNode->hasEditableStyle())
> >> 
> >> It seems like we should be checking hasRichlyEditableStyle() instead then?
> > 
> > Maybe the ChangeLog text is a bit misleading but this is the actual condition used by InsertNodeBeforeCommand  ASSERT.
> 
> Well, we shouldn't be listifying in plain text mode so we need to be
> checking hasRichlyEditableStyle.
> The existence of other code which checks for a wrong condition isn't a good
> argument for introducing new wrong code.

Hmm then I guess this file should need a review pass later on because it has several appearances of the same check (that's why I thought it was correct) see for example:
* InsertListCommand::unlistifyParagraph()
* InsertListCommand::listifyParagraph()

[Sergio Villar Senin]

Created attachment 414330 [details]
Patch for landing

[Ryosuke Niwa]

Are we waiting for something to land this patch??

[Sergio Villar Senin]

Comment on attachment 414330 [details]
Patch for landing

Actually marking for review

[Ryosuke Niwa]

Comment on attachment 414330 [details]
Patch for landing

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

> LayoutTests/fast/editing/insert-list-in-orphaned-list-item-crash.html:4
> +  testRunner.dumpAsText();

We use 4 space indentation?

[Sergio Villar Senin]

Committed r270018: <https://trac.webkit.org/changeset/270018>