Bug 218492 – Null Ptr Deref @ WebCore::DeleteSelectionCommand::doApply+19653

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

[Ian Gilbert]

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

Stack Trace
=========

frame #0: WebCore`WebCore::DeleteSelectionCommand::doApply()+19653
frame #1: WebCore`WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&)+79
frame #2: WebCore`WebCore::TypingCommand::deleteKeyPressed(WebCore::TextGranularity, bool)+7376
frame #3: WebCore`WebCore::CompositeEditCommand::apply()+385
frame #4: WebCore`WebCore::TypingCommand::deleteKeyPressed(WebCore::Document&, unsigned int, WebCore::TextGranularity)+331
frame #5: WebCore`WebCore::executeDelete(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)+85
frame #6: WebCore`WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)+77
frame #7: WebCore`WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*)+426
frame #8: JavaScriptCore`llint_entry+102726
frame #9: JavaScriptCore`vmEntryToJavaScript+200
frame #10: JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)+582

[Radar WebKit Bug Importer]

<rdar://problem/70987165>

[Ian Gilbert]

Created attachment 413026 [details]
Crashing input

[Ryosuke Niwa]

<rdar://problem/67426413>

[Rob Buis]

Bt on Linux GTK:
ASSERTION FAILED: endingSelection().isCaretOrRange()
../../Source/WebCore/editing/CompositeEditCommand.cpp(1487) : void WebCore::CompositeEditCommand::moveParagraphs(const WebCore::VisiblePosition&, const WebCore::VisiblePosition&, const WebCore::VisiblePosition&, bool, bool)
1   0x7fa333f023d1 WTFCrash
2   0x7fa3426fa097 /app/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xcc39097) [0x7fa3426fa097]
3   0x7fa3474bba4a WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool)
4   0x7fa3474baf15 WebCore::CompositeEditCommand::moveParagraph(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool)
5   0x7fa345885316 WebCore::DeleteSelectionCommand::mergeParagraphs()
6   0x7fa345886303 WebCore::DeleteSelectionCommand::doApply()
7   0x7fa3474b556c WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&)
8   0x7fa3474b7d6c WebCore::CompositeEditCommand::deleteSelection(WebCore::VisibleSelection const&, bool, bool, bool, bool, bool)
9   0x7fa34592067f WebCore::TypingCommand::deleteKeyPressed(WebCore::TextGranularity, bool)
10  0x7fa34591e339 WebCore::TypingCommand::doApply()
11  0x7fa3474b4fde WebCore::CompositeEditCommand::apply()
12  0x7fa34591d46f WebCore::TypingCommand::deleteKeyPressed(WebCore::Document&, unsigned int, WebCore::TextGranularity)
13  0x7fa3458b8c89 /app/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xfdf7c89) [0x7fa3458b8c89]
14  0x7fa3458bdf66 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
15  0x7fa345653dd9 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)

[Rob Buis]

Created attachment 413284 [details]
Slightly reduced testcase

[Ryosuke Niwa]

Created attachment 413384 [details]
Reduced test case

[Rob Buis]

Created attachment 413968 [details]
Patch

[Alex Christensen]

Comment on attachment 413968 [details]
Patch

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

Is this a UAF?  If not, could we just include the test with the fix?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:764
> +    if (endingSelection().start().anchorNode() && endingSelection().start().anchorNode()->isConnected())

C++17 if with initializer would be useful here so we don't have to call start and anchorNode twice.
if (auto* anchorNode = endingSelection.start().anchorNode(); anchorNode && anchorNode->isConnected())

[Ryosuke Niwa]

Comment on attachment 413968 [details]
Patch

Yeah this doesn't look like a security issue. Please include the test.

[Rob Buis]

Created attachment 414027 [details]
Patch

[Rob Buis]

Comment on attachment 413968 [details]
Patch

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

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:764
>> +    if (endingSelection().start().anchorNode() && endingSelection().start().anchorNode()->isConnected())
> 
> C++17 if with initializer would be useful here so we don't have to call start and anchorNode twice.
> if (auto* anchorNode = endingSelection.start().anchorNode(); anchorNode && anchorNode->isConnected())

Nice! Done.

[Rob Buis]

Comment on attachment 414027 [details]
Patch

Testcase is added.

[EWS]

Committed r269776: <https://trac.webkit.org/changeset/269776>

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

[Ryan Haddad]

The test added with this change appears to be asserting on debug bots:

ASSERTION FAILED: endingSelection().isCaretOrRange()
./editing/CompositeEditCommand.cpp(1487) : void WebCore::CompositeEditCommand::moveParagraphs(const WebCore::VisiblePosition &, const WebCore::VisiblePosition &, const WebCore::VisiblePosition &, bool, bool)

https://build.webkit.org/results/Apple-Catalina-Debug-WK1-Tests/r269785%20(8307)/results.html

https://results.webkit.org/?suite=layout-tests&test=editing%2Fdeleting%2Fdelete-contenteditable-crash.html

[Ryan Haddad]

Reverted r269776 for reason:

The test added with this change is asserting

Committed r269803: <https://trac.webkit.org/changeset/269803>

[Rob Buis]

Created attachment 414321 [details]
Patch

[Rob Buis]

Comment on attachment 414321 [details]
Patch

As suspected, after landing 414321 the test case now passes on Debug (Mac-debug-wk1).

[EWS]

Committed r269902: <https://trac.webkit.org/changeset/269902>

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