Bug 218215 – Null dereference in CompositeEditCommand::splitTreeToNode() due to not checking for top of DOM tree

• Component: HTML Editing
• Status: RESOLVED FIXED

[Julian Gonzalez]

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

[Radar WebKit Bug Importer]

<rdar://problem/70702563>

[Julian Gonzalez]

<rdar://problem/66864853> Null Ptr Deref @ WebCore::CompositeEditCommand::splitTreeToNode -- DOS WebKitTestRunner WK2

[Julian Gonzalez]

Created attachment 412366 [details]
Patch

[Ryosuke Niwa]

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?

[Julian Gonzalez]

Fixed that in the new patch, which also addresses the failure of the new test on the debug build.

[Julian Gonzalez]

Created attachment 412688 [details]
Patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 412688 [details]
Patch

I think these test failures are legitimate.

[Julian Gonzalez]

(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.

[Julian Gonzalez]

(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.

[Julian Gonzalez]

Created attachment 413379 [details]
Patch

[Alex Christensen]

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

[Ryosuke Niwa]

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)

[Alex Christensen]

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!

[Ryosuke Niwa]

There is no security implication here.

[Julian Gonzalez]

(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

[Julian Gonzalez]

(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.

[Julian Gonzalez]

Created attachment 413617 [details]
Patch

[EWS]

Committed r269609: <https://trac.webkit.org/changeset/269609>

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