Summary: | Null Ptr Deref @ WebCore::DeleteSelectionCommand::doApply+19653 | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ian Gilbert <iang> | ||||||||||||||
Component: | HTML Editing | Assignee: | Rob Buis <rbuis> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cgarcia, ews-feeder, ews-watchlist, gpoo, mifenton, product-security, rbuis, rniwa, rwlbuis, svillar, webkit-bug-importer, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Ian Gilbert
2020-11-03 02:35:28 PST
Created attachment 413026 [details]
Crashing input
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&) Created attachment 413284 [details]
Slightly reduced testcase
Created attachment 413384 [details]
Reduced test case
Created attachment 413968 [details]
Patch
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 on attachment 413968 [details]
Patch
Yeah this doesn't look like a security issue. Please include the test.
Created attachment 414027 [details]
Patch
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 on attachment 414027 [details]
Patch
Testcase is added.
Committed r269776: <https://trac.webkit.org/changeset/269776> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414027 [details]. 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 Reverted r269776 for reason: The test added with this change is asserting Committed r269803: <https://trac.webkit.org/changeset/269803> Created attachment 414321 [details]
Patch
Comment on attachment 414321 [details]
Patch
As suspected, after landing 414321 the test case now passes on Debug (Mac-debug-wk1).
Committed r269902: <https://trac.webkit.org/changeset/269902> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414321 [details]. |