Bug 218215

Summary: Null dereference in CompositeEditCommand::splitTreeToNode() due to not checking for top of DOM tree
Product: WebKit Reporter: Julian Gonzalez <julian_a_gonzalez>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, ews-feeder, ews-watchlist, mifenton, product-security, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Julian Gonzalez 2020-10-26 17:25:22 PDT 
Similar to 218132:

#0 0x4ce0df2ce in WTF::OptionSet<WebCore::Node::NodeFlag>::containsAny(WTF::OptionSet<WebCore::Node::NodeFlag>) const+0xbe
#1 0x4ce0df1a9 in WTF::OptionSet<WebCore::Node::NodeFlag>::contains(WebCore::Node::NodeFlag) const+0xd9
#2 0x4ce0df0cc in WebCore::Node::hasNodeFlag(WebCore::Node::NodeFlag) const+0xc
#3 0x4ce0df7dd in WebCore::Node::isElementNode() const+0xd
#4 0x4ce0df7c8 in WTF::TypeCastTraits<WebCore::Element const, WebCore::ContainerNode const, false>::isType(WebCore::Node const&)+0x8 
#5 0x4ce0df7b8 in WTF::TypeCastTraits<WebCore::Element const, WebCore::ContainerNode const, false>::isOfType(WebCore::ContainerNode const&)+0x8
#6 0x4ce0df7e8 in bool WTF::is<WebCore::Element, WebCore::ContainerNode>(WebCore::ContainerNode&)+0x8
#7 0x4d227cce7 in WebCore::CompositeEditCommand::splitTreeToNode(WebCore::Node&, WebCore::Node&, bool)+0x227
#8 0x4d2329f03 in WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*)+0x723
#9 0x4d23293a4 in WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::SimpleRange&)+0xbd4
#10 0x4d23286f6 in WebCore::InsertListCommand::doApply()+0xce6
#11 0x4d22545c6 in WebCore::CompositeEditCommand::apply()+0x216
#12 0x4d23143e8 in WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)+0xc8
#13 0x4d22d8d9b in WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const+0xdb
#14 0x4d1f61aa3 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)+0xf3
Comment 1 Radar WebKit Bug Importer 2020-10-26 17:25:34 PDT 
<rdar://problem/70702563>
Comment 2 Julian Gonzalez 2020-10-26 17:25:50 PDT 
<rdar://problem/66864853> Null Ptr Deref @ WebCore::CompositeEditCommand::splitTreeToNode -- DOS WebKitTestRunner WK2
Comment 3 Julian Gonzalez 2020-10-26 17:32:02 PDT 
Created attachment 412366 [details]
Patch
Comment 4 Ryosuke Niwa 2020-10-28 03:33:20 PDT 
Comment on attachment 412366 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1716
> +    for (node = &start; node && node->parentNode() && node->parentNode() != adjustedEnd; node = node->parentNode()) {
>          if (!is<Element>(*node->parentNode()))

Can we avoid calling parentNode() four times here?
Comment 5 Julian Gonzalez 2020-10-29 14:16:47 PDT 
Fixed that in the new patch, which also addresses the failure of the new test on the debug build.
Comment 6 Julian Gonzalez 2020-10-29 14:28:41 PDT 
Created attachment 412688 [details]
Patch
Comment 7 Ryosuke Niwa 2020-10-30 15:44:31 PDT 
Comment on attachment 412688 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:324
> +        Node* lcnParentNode = listChildNode->parentNode();

Please don't use raw pointer. Use makeRefPtr to store it in RefPtr.
Also please don't abbreviations like lcn.
Comment 8 Ryosuke Niwa 2020-11-01 07:39:57 PST 
Comment on attachment 412688 [details]
Patch

I think these test failures are legitimate.
Comment 9 Julian Gonzalez 2020-11-05 17:21:23 PST 
(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 412688 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412688&action=review
> 
> > Source/WebCore/editing/InsertListCommand.cpp:324
> > +        Node* lcnParentNode = listChildNode->parentNode();
> 
> Please don't use raw pointer. Use makeRefPtr to store it in RefPtr.
> Also please don't abbreviations like lcn.

Fixed in new patch.
Comment 10 Julian Gonzalez 2020-11-05 17:21:49 PST 
(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 412688 [details]
> Patch
> 
> I think these test failures are legitimate.

Agreed, I can now reproduce them locally. I have a new patch that I'm preparing that should address them.
Comment 11 Julian Gonzalez 2020-11-05 17:27:18 PST 
Created attachment 413379 [details]
Patch
Comment 12 Alex Christensen 2020-11-05 17:43:54 PST 
Comment on attachment 413379 [details]
Patch

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

> LayoutTests/editing/inserting/insert-list-in-iframe-in-list-expected.txt:3
> +This tests that we do not crah while inserting either list.

crash
Comment 13 Ryosuke Niwa 2020-11-05 18:38:52 PST 
Comment on attachment 413379 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:324
> +        RefPtr<Node> listChildNodeParentNode = makeRefPtr(listChildNode->parentNode());

Use auto.

> Source/WebCore/editing/InsertListCommand.cpp:325
> +        if (listChildNodeParentNode && listChildNodeParentNode != listNode)

Declare the variable in if as in:
if (auto listChildNodeParentNode = ~; listChildNodeParentNode != listNode)
Comment 14 Alex Christensen 2020-11-06 10:48:45 PST 
Comment on attachment 413379 [details]
Patch

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

>> Source/WebCore/editing/InsertListCommand.cpp:325
>> +        if (listChildNodeParentNode && listChildNodeParentNode != listNode)
> 
> Declare the variable in if as in:
> if (auto listChildNodeParentNode = ~; listChildNodeParentNode != listNode)

if (auto listChildNodeParentNode = makeRefPtr(listChildNode->parentNode()); listChildNodeParentNode && listChildNodeParentNode != listNode)

Hooray C++17!
Comment 15 Ryosuke Niwa 2020-11-06 19:46:12 PST 
There is no security implication here.
Comment 16 Julian Gonzalez 2020-11-09 12:17:26 PST 
(In reply to Alex Christensen from comment #12)
> Comment on attachment 413379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413379&action=review
> 
> > LayoutTests/editing/inserting/insert-list-in-iframe-in-list-expected.txt:3
> > +This tests that we do not crah while inserting either list.
> 
> crash

Thanks :P
Comment 17 Julian Gonzalez 2020-11-09 12:18:05 PST 
(In reply to Alex Christensen from comment #14)
> Comment on attachment 413379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413379&action=review
> 
> >> Source/WebCore/editing/InsertListCommand.cpp:325
> >> +        if (listChildNodeParentNode && listChildNodeParentNode != listNode)
> > 
> > Declare the variable in if as in:
> > if (auto listChildNodeParentNode = ~; listChildNodeParentNode != listNode)
> 
> if (auto listChildNodeParentNode = makeRefPtr(listChildNode->parentNode());
> listChildNodeParentNode && listChildNodeParentNode != listNode)
> 
> Hooray C++17!

Upgrading my mental C++ model is a slow process :) Thank you both.
Comment 18 Julian Gonzalez 2020-11-09 12:19:46 PST 
Created attachment 413617 [details]
Patch
Comment 19 EWS 2020-11-09 15:48:13 PST 
Committed r269609: <https://trac.webkit.org/changeset/269609>

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