Nullptr crash in WebCore::canHaveChildrenForEditing via CompositeEditCommand::insertNode

[Jack]

<rdar://56685655>

    #0 0x6c019d821 in WebCore::Node::getFlag(WebCore::Node::NodeFlags) const (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x19b821)
    #1 0x6c33db02d in WebCore::canHaveChildrenForEditing(WebCore::Node const&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x33d902d)
    #2 0x6c339c496 in WebCore::CompositeEditCommand::insertNodeAt(WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&&, WebCore::Position const&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x339a496)
    #3 0x6c3466d28 in WebCore::InsertListCommand::listifyParagraph(WebCore::VisiblePosition const&, WebCore::QualifiedName const&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3464d28)
    #4 0x6c34654ff in WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::Range*) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34634ff)
    #5 0x6c34647af in WebCore::InsertListCommand::doApply() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34627af)
    #6 0x6c3396376 in WebCore::CompositeEditCommand::apply() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3394376)
    #7 0x6c344d78b in WebCore::executeInsertUnorderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x344b78b)
    #8 0x6c30be1c7 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x30bc1c7)
    #9 0x6c0a33c9a in WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0xa31c9a)
    #10 0x6c08ec150 in long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x8ea150)
    #11 0x3d778e40116a  (<unknown module>)
    #12 0x6d9a7640c in llint_entry (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa3d40c)
    #13 0x6d9a76298 in llint_entry (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa3d298)
    #14 0x6d9a5f878 in vmEntryToJavaScript (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa26878)
    #15 0x6db0140db in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x1fdb0db)
    #16 0x6db5d6fc0 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x259dfc0)
    #17 0x6db5d70c1 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x259e0c1)
    #18 0x6db5d749f in JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x259e49f)
    #19 0x6c29c7174 in WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x29c5174)
    #20 0x6c29ef55c in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x29ed55c)
    #21 0x6c31cf524 in WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul>, WebCore::EventTarget::EventInvokePhase) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x31cd524)
    #22 0x6c31ca7f7 in WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x31c87f7)
    #23 0x6c3f59e37 in WebCore::DOMWindow::dispatchEvent(WebCore::Event&, WebCore::EventTarget*) (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3f57e37)
    #24 0x6c3f6c5cf in WebCore::DOMWindow::dispatchLoadEvent() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3f6a5cf)
    #25 0x6c30a603f in WebCore::Document::dispatchWindowLoadEvent() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x30a403f)
    #26 0x6c30a59f0 in WebCore::Document::implicitClose() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x30a39f0)
    #27 0x6c3d944fb in WebCore::FrameLoader::checkCompleted() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3d924fb)
    #28 0x6c308b978 in WebCore::Document::loadEventDelayTimerFired() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3089978)
    #29 0x6c3110d53 in std::__1::__bind_return<void (WebCore::Document::*)(), std::__1::tuple<WebCore::Document*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::Document::*)(), std::__1::tuple<WebCore::Document*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (WebCore::Document::*&)(), WebCore::Document*>::operator()<>() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x310ed53)
    #30 0x6c42a0ff5 in WebCore::ThreadTimers::sharedTimerFiredInternal() (Safari_ASAN_251639_8651f88be7c86f1fa8b4fafe3bc6b2caded79eda.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x429eff5)

[Jack]

Root cause:
In this case, hr is uneditable, and the script tries to insert an ol after hr, so the code is creating a BR element and inserting it after HR. However, since hr is uneditable, the insertion fails, making the BR element parentless. and that results in an null insertionPos, and it is later referenced in canHaveChildrenForEditing.

<style>
body { -webkit-user-modify: read-write; background-image: url(); }
</style>
<script>
    onload = function fun() {
        document.getSelection().setPosition(HR);
        HR.appendChild(document.createElement("option"));
        document.execCommand("insertOrderedList", false);
    }
</script>
<body><hr id=HR contenteditable="false"></hr>

[Jack]

In debug build, it would trigger two assertions:
1. CompositeEditCommand::insertNodeAt
    ASSERT(isEditablePosition(editingPosition));
2. InsertNodeBeforeCommand::InsertNodeBeforeCommand
    ASSERT(m_refChild->parentNode()->hasEditableStyle() || !m_refChild->parentNode()->renderer());

[Jack]

Created attachment 391591 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 391591 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:341
> -    if (start.isNull() || end.isNull())
> +    if (start.isNull() || end.isNull() || !start.deepEquivalent().deprecatedNode()->hasEditableStyle())

Please check the eatability of start.deepEquivalent().containerNode() instead.

[Ryosuke Niwa]

This is not a security bug.

[Jack]

Thanks! Yes, in this case the anchorType is PositionIsOffsetInAnchor, so container node is also m_anchorNode.

(lldb) p anchorType()
(WebCore::Position::AnchorType) $3 = PositionIsOffsetInAnchor

(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 391591 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391591&action=review
> 
> > Source/WebCore/editing/InsertListCommand.cpp:341
> > -    if (start.isNull() || end.isNull())
> > +    if (start.isNull() || end.isNull() || !start.deepEquivalent().deprecatedNode()->hasEditableStyle())
> 
> Please check the eatability of start.deepEquivalent().containerNode()
> instead.

[Jack]

Created attachment 391660 [details]
Patch

[Doug Kelly]

Hey Jack,

I just happened to notice unlistify can crash in much the same way -- does it make sense to apply the check at the level of doApply() instead?

doApply() already has an early-abort for if content is not editable:

if (endingSelection().isNoneOrOrphaned() || !endingSelection().isContentRichlyEditable())
    return;

[Ryosuke Niwa]

Comment on attachment 391660 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:341
> -    if (start.isNull() || end.isNull())
> +    if (start.isNull() || end.isNull() || !start.deepEquivalent().containerNode()->hasEditableStyle())

Sorry, I forgot to point out earlier but we should probably check that the end is also editable.

[Jack]

Thanks, Doug. It is possible. However, the actual inserting position is determined later in the call stack (depending on the tag and its children and some other properties) and can change in other cases, so it can be false positives if the code returns too early.

In this test case, at doApply() the endingSelection has anchor type = PositionIsBeforeAnchor, so the editability is checked on the parent of anchor node, which is <body> hence it does not return here.

When it enters listifyParagraph and after startOfParagraph is called, the anchor type becomes PositionIsOffsetInAnchor so the editability is checked on the anchor node, <hr>, which is uneditable. 

One experiment I did was removing the child (option) of the uneditable element (hr). Even though the anchor node is the uneditable element, the code does continue and inserts ol in front of hr.

It sort of makes sense since if there is nothing to be "listified" inside <hr>, it is not considered "editing" <hr>? 

(In reply to Doug Kelly from comment #8)
> Hey Jack,
> 
> I just happened to notice unlistify can crash in much the same way -- does
> it make sense to apply the check at the level of doApply() instead?
> 
> doApply() already has an early-abort for if content is not editable:
> 
> if (endingSelection().isNoneOrOrphaned() ||
> !endingSelection().isContentRichlyEditable())
>     return;

[Jack]

Thanks! I thought about it but wondered if it is okay to listify contents if only the end is uneditable. Definitely it is more important to prevent crash!

(In reply to Ryosuke Niwa from comment #9)
> Comment on attachment 391660 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391660&action=review
> 
> > Source/WebCore/editing/InsertListCommand.cpp:341
> > -    if (start.isNull() || end.isNull())
> > +    if (start.isNull() || end.isNull() || !start.deepEquivalent().containerNode()->hasEditableStyle())
> 
> Sorry, I forgot to point out earlier but we should probably check that the
> end is also editable.

[Jack]

Created attachment 391723 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391723 [details]
Patch

Clearing flags on attachment: 391723

Committed r257536: <https://trac.webkit.org/changeset/257536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

*** Bug 208515 has been marked as a duplicate of this bug. ***