Bug 218492 - Null Ptr Deref @ WebCore::DeleteSelectionCommand::doApply+19653
Summary: Null Ptr Deref @ WebCore::DeleteSelectionCommand::doApply+19653
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-03 02:35 PST by Ian Gilbert
Modified: 2020-11-17 08:29 PST (History)
13 users (show)

See Also:


Attachments
Crashing input (1.66 KB, text/html)
2020-11-03 02:36 PST, Ian Gilbert
no flags Details
Slightly reduced testcase (1.63 KB, text/html)
2020-11-05 05:09 PST, Rob Buis
no flags Details
Reduced test case (377 bytes, text/html)
2020-11-05 18:50 PST, Ryosuke Niwa
no flags Details
Patch (1.54 KB, patch)
2020-11-12 12:55 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.60 KB, patch)
2020-11-13 03:17 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.55 KB, patch)
2020-11-17 01:56 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Gilbert 2020-11-03 02:35:28 PST
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
Comment 1 Radar WebKit Bug Importer 2020-11-03 02:35:39 PST
<rdar://problem/70987165>
Comment 2 Ian Gilbert 2020-11-03 02:36:29 PST
Created attachment 413026 [details]
Crashing input
Comment 3 Ryosuke Niwa 2020-11-03 13:07:05 PST
<rdar://problem/67426413>
Comment 4 Rob Buis 2020-11-05 05:08:22 PST
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&)
Comment 5 Rob Buis 2020-11-05 05:09:54 PST
Created attachment 413284 [details]
Slightly reduced testcase
Comment 6 Ryosuke Niwa 2020-11-05 18:50:42 PST
Created attachment 413384 [details]
Reduced test case
Comment 7 Rob Buis 2020-11-12 12:55:48 PST
Created attachment 413968 [details]
Patch
Comment 8 Alex Christensen 2020-11-12 15:16:17 PST
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())
Comment 9 Ryosuke Niwa 2020-11-12 21:36:52 PST
Comment on attachment 413968 [details]
Patch

Yeah this doesn't look like a security issue. Please include the test.
Comment 10 Rob Buis 2020-11-13 03:17:21 PST
Created attachment 414027 [details]
Patch
Comment 11 Rob Buis 2020-11-13 06:29:35 PST
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.
Comment 12 Rob Buis 2020-11-13 06:30:11 PST
Comment on attachment 414027 [details]
Patch

Testcase is added.
Comment 13 EWS 2020-11-13 08:01:04 PST
Committed r269776: <https://trac.webkit.org/changeset/269776>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414027 [details].
Comment 14 Ryan Haddad 2020-11-13 14:51:05 PST
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
Comment 15 Ryan Haddad 2020-11-13 14:57:17 PST
Reverted r269776 for reason:

The test added with this change is asserting

Committed r269803: <https://trac.webkit.org/changeset/269803>
Comment 16 Rob Buis 2020-11-17 01:56:35 PST
Created attachment 414321 [details]
Patch
Comment 17 Rob Buis 2020-11-17 05:24:08 PST
Comment on attachment 414321 [details]
Patch

As suspected, after landing 414321 the test case now passes on Debug (Mac-debug-wk1).
Comment 18 EWS 2020-11-17 08:29:49 PST
Committed r269902: <https://trac.webkit.org/changeset/269902>

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