Bug 232837

Summary:

nullptr deref in CompositeEditCommand::insertNodeAt

Product:

WebKit

Reporter:

Gabriel Nava Marino <gnavamarino>

Component:

HTML Editing

Assignee:

Gabriel Nava Marino <gnavamarino>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, ews-watchlist, mifenton, rniwa, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-

Gabriel Nava Marino

Reported 2021-11-08 12:46:53 PST

This seems to occur when p.deprecatedNode() is nullptr.

Attachments
Patch (3.92 KB, patch) 2021-11-08 12:59 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.82 KB, patch) 2021-11-08 14:03 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.75 KB, patch) 2021-11-08 15:49 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (4.15 KB, patch) 2021-11-10 10:21 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (4.02 KB, patch) 2021-11-10 16:41 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.91 KB, patch) 2021-11-11 10:31 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.65 KB, patch) 2021-11-11 17:46 PST, Gabriel Nava Marino	no flags	Details Formatted Diff Diff
Patch (3.69 KB, patch) 2021-11-11 17:50 PST, Gabriel Nava Marino	ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Gabriel Nava Marino

Comment 1 2021-11-08 12:59:13 PST

Created attachment 443593 [details] Patch

Gabriel Nava Marino

Comment 2 2021-11-08 14:03:30 PST

Created attachment 443600 [details] Patch

Wenson Hsieh

Comment 3 2021-11-08 15:08:13 PST

Comment on attachment 443600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443600&action=review > LayoutTests/fast/editing/editing-position-crash.html:1 > +<!DOCTYPE html> The rest of the change seems fine, but this test case looks like it could use some cleanup. A few confusing/odd details: - The html closing tag on line 2 - The use of try-catch in the event handlers (as well as the names of all the event handler functions) - The text node after the end of the body element. - Lack of explicit closing tags on a few of the elements below, such as the svg.

Gabriel Nava Marino

Comment 4 2021-11-08 15:49:25 PST

Created attachment 443625 [details] Patch

Gabriel Nava Marino

Comment 5 2021-11-10 10:21:36 PST

Created attachment 443830 [details] Patch

Wenson Hsieh

Comment 6 2021-11-10 13:16:15 PST

Comment on attachment 443830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443830&action=review It looks like the new test is crashing underneath: ``` ASSERTION FAILED: ancestor ./dom/Position.cpp(1586) : WebCore::Position WebCore::positionInParentBeforeNode(WebCore::Node *) 1 0x13a6f4928 WTFCrash 2 0x118de87a0 WebCore::CachedResourceHandleBase::operator!() const 3 0x119784e14 WebCore::positionInParentBeforeNode(WebCore::Node*) 4 0x1198b0e10 WebCore::CreateLinkCommand::doApply() 5 0x1198951d8 WebCore::CompositeEditCommand::apply() 6 0x119929a70 WebCore::executeCreateLink(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) 7 0x119900498 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const 8 0x1195b8bd4 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) 9 0x116b7e268 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) 10 0x116b7dd10 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) ``` > LayoutTests/ChangeLog:6 > + Reviewed by Reviewed by Wenson Hsieh. Since I haven't reviewed (added r+ to) this patch, this should say `Reviewed by NOBODY (OOPS!)`. (But also, there is a redundant "Reviewed by" here)

Gabriel Nava Marino

Comment 7 2021-11-10 13:38:44 PST

(In reply to Wenson Hsieh from comment #6) > Comment on attachment 443830 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443830&action=review > > It looks like the new test is crashing underneath: I think this is likely pointing at the root cause being upstream. I am taking a look. Thank you for pointing this out. > > ``` > ASSERTION FAILED: ancestor > ./dom/Position.cpp(1586) : WebCore::Position > WebCore::positionInParentBeforeNode(WebCore::Node *) > 1 0x13a6f4928 WTFCrash > 2 0x118de87a0 WebCore::CachedResourceHandleBase::operator!() const > 3 0x119784e14 WebCore::positionInParentBeforeNode(WebCore::Node*) > 4 0x1198b0e10 WebCore::CreateLinkCommand::doApply() > 5 0x1198951d8 WebCore::CompositeEditCommand::apply() > 6 0x119929a70 WebCore::executeCreateLink(WebCore::Frame&, WebCore::Event*, > WebCore::EditorCommandSource, WTF::String const&) > 7 0x119900498 WebCore::Editor::Command::execute(WTF::String const&, > WebCore::Event*) const > 8 0x1195b8bd4 WebCore::Document::execCommand(WTF::String const&, bool, > WTF::String const&) > 9 0x116b7e268 > WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, > JSC::CallFrame*, WebCore::JSDocument*) > 10 0x116b7dd10 long long > WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore:: > jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, > JSC::CallFrame*, WebCore::JSDocument*)), > (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, > char const*) > ``` > > > LayoutTests/ChangeLog:6 > > + Reviewed by Reviewed by Wenson Hsieh. > > Since I haven't reviewed (added r+ to) this patch, this should say `Reviewed > by NOBODY (OOPS!)`. Thank you for this clarification. I will add this Reviewed by after receiving an r+. > > (But also, there is a redundant "Reviewed by" here) Thank you for pointing this out.

Gabriel Nava Marino

Comment 8 2021-11-10 16:41:47 PST

Created attachment 443877 [details] Patch

Gabriel Nava Marino

Comment 9 2021-11-11 10:31:01 PST

Created attachment 443971 [details] Patch

Gabriel Nava Marino

Comment 10 2021-11-11 10:33:06 PST

<rdar://problem/82958441>

Radar WebKit Bug Importer

Comment 11 2021-11-11 10:33:40 PST

<rdar://problem/85305520>

Darin Adler

Comment 12 2021-11-11 17:01:23 PST

Comment on attachment 443971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443971&action=review > LayoutTests/fast/editing/editing-position-crash-expected.txt:1 > +CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. Is there a way to make this test not depend on our recursion limit, but still cover this crash?

Darin Adler

Comment 13 2021-11-11 17:29:34 PST

Comment on attachment 443971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443971&action=review > Source/WebCore/editing/CreateLinkCommand.cpp:51 > + else if (!endingSelection().isOrphan()) { Shouldn’t we check if it’s an orphan up at the top of the function, rather than only after the isRange check?

Darin Adler

Comment 14 2021-11-11 17:30:51 PST

Comment on attachment 443971 [details] Patch This patch is OK, but I think it could be made a little better by making the test not depend on a "call stack size exceeded" exception and by doing the orphan check earlier. But neither of those seems a reason to not land it ... just that each of those things would be a significant improvement.

Gabriel Nava Marino

Comment 15 2021-11-11 17:35:45 PST

Comment on attachment 443971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443971&action=review >> Source/WebCore/editing/CreateLinkCommand.cpp:51 >> + else if (!endingSelection().isOrphan()) { > > Shouldn’t we check if it’s an orphan up at the top of the function, rather than only after the isRange check? Thank you for the feedback! Yes, I will change the endingSelection().isNone check above to endingSelection().isNoneOrOrphaned(). >> LayoutTests/fast/editing/editing-position-crash-expected.txt:1 >> +CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. > > Is there a way to make this test not depend on our recursion limit, but still cover this crash? Yes, I will pass an option so the event listener is only invoked at most once.

Gabriel Nava Marino

Comment 16 2021-11-11 17:46:42 PST

Created attachment 444030 [details] Patch

Gabriel Nava Marino

Comment 17 2021-11-11 17:50:39 PST

Created attachment 444031 [details] Patch

Gabriel Nava Marino

Comment 18 2021-11-15 10:49:21 PST

Comment on attachment 444031 [details] Patch It looks like the unrelated layout test failure it is hitting was addressed in Bug 233043 - Regression(r285639) fast/dom/Geolocation/cached-position-iframe.html is frequently crashing on Mac-wk1 However, the bot is not yet running with that revision. Moving to cq?

Wenson Hsieh

Comment 19 2021-11-15 10:52:39 PST

Comment on attachment 444031 [details] Patch Indeed, those geolocation tests have been flaky somewhat recently.

EWS

Comment 20 2021-11-15 10:57:06 PST

Committed r285813 (244257@main): <https://commits.webkit.org/244257@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444031 [details].

Rob Buis

Comment 21 2022-06-15 22:55:09 PDT

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

Note You need to log in before you can comment on or make changes to this bug.