Bug 210004 – Nullptr crash in CompositeEditCommand::splitTreeToNode when inserting image in anchor element that has uneditable parent

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Jack]

<rdar://61206583>

[Jack]

    #0 0x15836d155 in WebCore::CompositeEditCommand::splitTreeToNode(WebCore::Node&, WebCore::Node&, bool) (Safari_ASAN_259384_90de49be478170ac8513448015dcb21abd3583ea.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3512155)
    #1 0x158439bf5 in WebCore::ReplaceSelectionCommand::doApply() (Safari_ASAN_259384_90de49be478170ac8513448015dcb21abd3583ea.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x35debf5)
    #2 0x1583451d6 in WebCore::CompositeEditCommand::apply() (Safari_ASAN_259384_90de49be478170ac8513448015dcb21abd3583ea.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34ea1d6)
    #3 0x158401dc1 in WebCore::executeInsertFragment(WebCore::Frame&, WTF::Ref<WebCore::DocumentFragment, WTF::DumbPtrTraits<WebCore::DocumentFragment> >&&) (Safari_ASAN_259384_90de49be478170ac8513448015dcb21abd3583ea.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x35a6dc1)
    #4 0x1584020d2 in WebCore::executeInsertNode(WebCore::Frame&, WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&&) (Safari_ASAN_259384_90de49be478170ac8513448015dcb21abd3583ea.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x35a70d2)
    #5 0x1583fba54 in WebCore::executeInsertImage(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) (Safari_ASAN_259384_90de49be478170ac8513448015dcb21abd3583ea.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x35a0a54)
    #6 0x158074f47 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) (Safari_ASAN_259384_90de49be478170ac8513448015dcb21abd3583ea.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3219f47)

[Jack]

Created attachment 395455 [details]
Patch

[Jack]

(In reply to Jack from comment #2)
> Created attachment 395455 [details]
> Patch

When inserting image in anchor we try to push down the anchor element by first moving anchor's children to its parent. If anchor's parent is uneditable, the move fails and leaves the children dangling and parentless. Later the code crashes when function splitTreeToNode dereferences the parent of the dangling children.

RemoveNodePreservingChildren can fail and leave the children dangling if the parent of the node is uneditable. Added editability check for the to-be-removed node.

Also pushAnchorElementDown would fail silently and we cannot avoid anchor element in function positionAvoidingSpecialElementBoundary. Added check for same enclosing anchor and return empty Position to the caller.

Lastly in ReplaceSelectionCommand, added check for empty Position returned by positionAvoidingSpecialElementBoundary and terminate the command.

[Jack]

Created attachment 395469 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 395469 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1724
> +                Element* newEnclosingAnchor = enclosingAnchorElement(original);
> +                if (newEnclosingAnchor != enclosingAnchor)
> +                    enclosingAnchor = newEnclosingAnchor;
> +                else
> +                    return { };

What is this change about? It should be explained in the change log as a per function comment.
I don't think this function is ever supposed to return null position like this.
See lines below where we set result to original if result is null.
r- because of this.

> Source/WebCore/editing/RemoveNodePreservingChildrenCommand.cpp:44
> +    ContainerNode* parent = m_node->parentNode();

Please use makeRefPtr and auto.

[Jack]

Thanks. I added a comment in the log but not under the function. Do you mean this is where it should go?

(WebCore::CompositeEditCommand::positionAvoidingSpecialElementBoundary):
       Also pushAnchorElementDown would fail silently and we cannot avoid anchor element in function positionAvoidingSpecialElementBoundary. Added check for same enclosing anchor and return empty Position to the caller.

(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 395469 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395469&action=review
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:1724
> > +                Element* newEnclosingAnchor = enclosingAnchorElement(original);
> > +                if (newEnclosingAnchor != enclosingAnchor)
> > +                    enclosingAnchor = newEnclosingAnchor;
> > +                else
> > +                    return { };
> 
> What is this change about? It should be explained in the change log as a per
> function comment.
> I don't think this function is ever supposed to return null position like
> this.
> See lines below where we set result to original if result is null.
> r- because of this.
> 
> > Source/WebCore/editing/RemoveNodePreservingChildrenCommand.cpp:44
> > +    ContainerNode* parent = m_node->parentNode();
> 
> Please use makeRefPtr and auto.

[Ryosuke Niwa]

(In reply to Jack from comment #6)
> Thanks. I added a comment in the log but not under the function. Do you mean
> this is where it should go?
> 
> (WebCore::CompositeEditCommand::positionAvoidingSpecialElementBoundary):
>        Also pushAnchorElementDown would fail silently and we cannot avoid
> anchor element in function positionAvoidingSpecialElementBoundary. Added
> check for same enclosing anchor and return empty Position to the caller.

Yes, although it should start after : in the first line.

Regardless, we shouldn't be returning null Position there.

[Jack]

Thanks. Yeah, I thought about that too and experimented with returning original position. The insertion command will continue and insert the image in anchor element without avoiding it. Therefore I thought if we return empty position, the caller can terminate the command earlier. 

Or is it okay just to let the image be inserted?

> (In reply to Ryosuke Niwa from comment #5)
> > Comment on attachment 395469 [details]
> > Patch
> > I don't think this function is ever supposed to return null position like
> > this.
> > See lines below where we set result to original if result is null.
> > r- because of this.
> > 
> > > Source/WebCore/editing/RemoveNodePreservingChildrenCommand.cpp:44
> > > +    ContainerNode* parent = m_node->parentNode();
> >

[Ryosuke Niwa]

(In reply to Jack from comment #8)
> Thanks. Yeah, I thought about that too and experimented with returning
> original position. The insertion command will continue and insert the image
> in anchor element without avoiding it. Therefore I thought if we return
> empty position, the caller can terminate the command earlier. 
> 
> Or is it okay just to let the image be inserted?

Yes. That's the exiting fallback path anyway for when the position becomes null.

[Ryosuke Niwa]

There is no security implication here.

[Jack]

I see. Thanks!

(In reply to Ryosuke Niwa from comment #9)
> (In reply to Jack from comment #8)
> > Thanks. Yeah, I thought about that too and experimented with returning
> > original position. The insertion command will continue and insert the image
> > in anchor element without avoiding it. Therefore I thought if we return
> > empty position, the caller can terminate the command earlier. 
> > 
> > Or is it okay just to let the image be inserted?
> 
> Yes. That's the exiting fallback path anyway for when the position becomes
> null.

[Jack]

Created attachment 395629 [details]
Patch

[EWS]

Committed r259624: <https://trac.webkit.org/changeset/259624>

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