Null Ptr Deref @ WebCore::ReplaceSelectionCommand::doApply

[Ian Gilbert]

Description:
Crash found by fuzzing. Reproduces on WebKit revision 265372.

Stack Trace
===========

0   com.apple.WebCore             	0x00000001114dd9cb WebCore::ReplaceSelectionCommand::doApply() + 17483
1   com.apple.WebCore             	0x00000001100cbbc4 WebCore::CompositeEditCommand::apply() + 500
2   com.apple.WebCore             	0x00000001114bc59a WebCore::executeInsertFragment(WebCore::Frame&, WTF::Ref<WebCore::DocumentFragment, WTF::DumbPtrTraits<WebCore::DocumentFragment> >&&) + 74
3   com.apple.WebCore             	0x00000001114bc6d4 WebCore::executeInsertNode(WebCore::Frame&, WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&&) + 180
4   com.apple.WebCore             	0x00000001114b784b WebCore::executeInsertImage(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 75
5   com.apple.WebCore             	0x000000011013123d WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 77
6   com.apple.WebCore             	0x000000011053897c WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 428
7   ???                           	0x000026435ce01178 0 + 42070762852728
8   com.apple.JavaScriptCore      	0x000000010de7a95e llint_entry + 104810
9   com.apple.JavaScriptCore      	0x000000010de60dff vmEntryToJavaScript + 216
10  com.apple.JavaScriptCore      	0x000000010e49f536 JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 518

[Radar WebKit Bug Importer]

<rdar://problem/70987255>

[Ian Gilbert]

Created attachment 413027 [details]
Crashing Input

[Ryosuke Niwa]

<rdar://problem/68276643>

[Ian Gilbert]

Created attachment 413269 [details]
Reduced crashing input

[Rob Buis]

Created attachment 413970 [details]
More reduced testcase

[Carlos Garcia Campos]

Created attachment 418028 [details]
Patch

I'm including the test assuming there's no security implication here, since it's just another null check missing. Please confirm.

[Alex Christensen]

Comment on attachment 418028 [details]
Patch

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

Yep, this test can be included.
WebKitLegacy seems to be asserting in fragmentNeedsColorTransformed.  We should probably fix that too before landing.  The null check seems fine though.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1208
> +        if (insertionPos.containerNode() && insertionPos.containerNode()->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()) {

We could use if-with-initializer to avoid calling containerNode twice.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 418028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418028&action=review
> 
> Yep, this test can be included.
> WebKitLegacy seems to be asserting in fragmentNeedsColorTransformed.  We
> should probably fix that too before landing.  The null check seems fine
> though.

Yes, it's here:

RefPtr<Element> editableRoot = insertionPos.rootEditableElement();
ASSERT(editableRoot);
if (!editableRoot)
    return false;

I think the assert is wrong and the early return correct, so we can just remove the assert.

> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1208
> > +        if (insertionPos.containerNode() && insertionPos.containerNode()->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()) {
> 
> We could use if-with-initializer to avoid calling containerNode twice.

Sure.

[Carlos Garcia Campos]

Created attachment 418119 [details]
Patch

[Carlos Garcia Campos]

Now it's hitting another assert, but I can't reproduce it this time. I'll investigate.

[youenn fablet]

Comment on attachment 418119 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1209
> +                splitTextNode(*insertionPos.containerText(), insertionPos.offsetInContainerNode());

Do we have a guarantee that insertionPos.containerText() cannot be null? Is it ensured from containerNode->isTextNode()?
Looking at Position::containerText(), it seems to depend on anchorType being PositionIsOffsetInAnchor.

[Carlos Garcia Campos]

I was running the wrong test, I can indeed reproduce the new assert too.

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #11)
> Comment on attachment 418119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418119&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1209
> > +                splitTextNode(*insertionPos.containerText(), insertionPos.offsetInContainerNode());
> 
> Do we have a guarantee that insertionPos.containerText() cannot be null? Is
> it ensured from containerNode->isTextNode()?
> Looking at Position::containerText(), it seems to depend on anchorType being
> PositionIsOffsetInAnchor.

Not an expert in editing code, but I think it's the whole condition:

containerNode->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()

[Carlos Garcia Campos]

Created attachment 418137 [details]
Patch for landing

[EWS]

commit-queue failed to commit attachment 418137 [details] to WebKit repository.

[Carlos Garcia Campos]

Committed r271787: <https://trac.webkit.org/changeset/271787>