Bug 218484

Summary: Nullptr crash in RenderObject::parent
Product: WebKit Reporter: Ian Gilbert <iang>
Component: HTML EditingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cgarcia, darin, ews-feeder, ews-watchlist, ggaren, mifenton, product-security, rbuis, rniwa, sam, svillar, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Crashing input
none
Super-reduced test case
none
Patch
none
Patch for landing rniwa: review+

Description Ian Gilbert 2020-11-03 01:05:36 PST
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> >)+
Comment 1 Radar WebKit Bug Importer 2020-11-03 01:05:54 PST
<rdar://problem/70985057>
Comment 2 Ryosuke Niwa 2020-11-03 13:04:21 PST
<rdar://problem/70583715>
Comment 3 Sergio Villar Senin 2020-11-13 02:49:57 PST
Created attachment 414025 [details]
Super-reduced test case
Comment 4 Sergio Villar Senin 2020-11-13 09:34:39 PST
Created attachment 414052 [details]
Patch
Comment 5 Geoffrey Garen 2020-11-13 10:56:06 PST
Comment on attachment 414052 [details]
Patch

r=me
Comment 6 Ryosuke Niwa 2020-11-14 14:01:02 PST
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?
Comment 7 Sergio Villar Senin 2020-11-16 01:25:50 PST
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.
Comment 8 Darin Adler 2020-11-16 09:54:59 PST
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());
Comment 9 Ryosuke Niwa 2020-11-16 22:19:37 PST
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.
Comment 10 Ryosuke Niwa 2020-11-16 22:22:52 PST
(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.
Comment 11 Ryosuke Niwa 2020-11-16 22:23:18 PST
Okay, there doesn't seem to any security bug here so can we include the test case in the patch?
Comment 12 Sergio Villar Senin 2020-11-17 02:58:11 PST
(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()
Comment 13 Sergio Villar Senin 2020-11-17 04:50:23 PST
Created attachment 414330 [details]
Patch for landing
Comment 14 Ryosuke Niwa 2020-11-17 14:31:03 PST
Are we waiting for something to land this patch??
Comment 15 Sergio Villar Senin 2020-11-18 04:11:00 PST
Comment on attachment 414330 [details]
Patch for landing

Actually marking for review
Comment 16 Ryosuke Niwa 2020-11-18 09:18:42 PST
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?
Comment 17 Sergio Villar Senin 2020-11-19 00:41:16 PST
Committed r270018: <https://trac.webkit.org/changeset/270018>