Bug 204332

Summary:

Nullptr crash in Node::setTextContent via Document::setTitle if title element is removed before setTextContent call.

Product:

WebKit

Reporter:

Sunny He <sunny_he>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, product-security, rniwa, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

macOS 10.15

Attachments:

Description	Flags
Repro input	none
Patch	none
Patch	none

Sunny He

Reported 2019-11-18 17:17:59 PST

Created attachment 383812 [details] Repro input Document::setTitle may append a new title element if none currently exists. If this triggers an event handler which deletes this new title element, the call to setTextContent will cause a crash.

Attachments
Repro input (440 bytes, text/html) 2019-11-18 17:17 PST, Sunny He	no flags	Details
Patch (4.27 KB, patch) 2019-11-18 17:30 PST, Sunny He	no flags	Details Formatted Diff Diff
Patch (5.91 KB, patch) 2019-11-19 14:15 PST, Sunny He	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-11-18 17:18:09 PST

<rdar://problem/57305579>

Sunny He

Comment 2 2019-11-18 17:19:43 PST

<rdar://56626071>

Ryosuke Niwa

Comment 3 2019-11-18 17:24:25 PST

This is not a security bug.

Sunny He

Comment 4 2019-11-18 17:30:23 PST

Created attachment 383816 [details] Patch

Ryosuke Niwa

Comment 5 2019-11-18 17:30:55 PST

WebKit's behavior of document.title's setter matches the current spec: https://html.spec.whatwg.org/multipage/dom.html#document.title which says: > Let element be the result of creating an element given the document element's node document, title, and the HTML namespace. > > Append element to the head element. Because the spec doesn't have mutation events they don't have to deal with the situation in which appending the element to the head element results in its removal :(

Ryosuke Niwa

Comment 6 2019-11-18 17:32:58 PST

Comment on attachment 383816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383816&action=review > Source/WebCore/dom/Document.cpp:1601 > + // Another null check since insertBefore may have triggered event that deletes m_titleElement This is a bit verbose. I think we can just say: "insertBefore above may have ran scripts which removed m_titleElement" > Source/WebCore/dom/Document.cpp:1613 > + // Another null check since appendChild may have triggered event that deletes m_titleElement > + if (m_titleElement) Ditto. > LayoutTests/fast/dom/Document/title-property-set-with-dom-event.html:1 > +<html> Can we add another test case with a SVG document since you're also making the code change to SVG code path?

Ryosuke Niwa

Comment 7 2019-11-18 17:33:40 PST

I guess one thing we should check is what other browsers do. Do they set the value of title element despite of the fact they're no longer in the document? What do they do if they insert the removed element into a new document (e.g. into an iframe's contentDocument).

Sunny He

Comment 8 2019-11-18 17:53:26 PST

Firefox does update the text value, but Chrome does not. So this patch will align with Chrome's behavior.

Ryosuke Niwa

Comment 9 2019-11-18 21:17:35 PST

(In reply to Sunny He from comment #8) > Firefox does update the text value, but Chrome does not. So this patch will > align with Chrome's behavior. I don't think Firefox updates the text value per se. I think the behavior we observed is that Firefox would set the text content value of HTMLTitleElement before DOMNodeInserted event fires. In neither browsers, document's title was updated because the title element is ultimately removed.

Sunny He

Comment 10 2019-11-19 14:15:26 PST

Created attachment 383904 [details] Patch

Ryosuke Niwa

Comment 11 2019-11-19 16:52:45 PST

Comment on attachment 383904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383904&action=review > LayoutTests/fast/dom/Document/title-property-set-with-dom-event-svg.html:5 > + if (window.testRunner) { WebKit style is to not use curly braces around a single line statement but it’s okay to keep it like this for tests.

WebKit Commit Bot

Comment 12 2019-11-19 17:27:42 PST

Comment on attachment 383904 [details] Patch Clearing flags on attachment: 383904 Committed r252667: <https://trac.webkit.org/changeset/252667>

WebKit Commit Bot

Comment 13 2019-11-19 17:27:44 PST

All reviewed patches have been landed. Closing bug.

Simon Fraser (smfr)

Comment 14 2019-11-19 21:07:44 PST

Comment on attachment 383904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383904&action=review > Source/WebCore/dom/Document.cpp:1601 > + // insertBefore above may have ran scripts which removed m_titleElement ...may have run scripts... Also period at the end (we like comments to be full sentences). > Source/WebCore/dom/Document.cpp:1612 > + // appendChild above may have ran scripts which removed m_titleElement Same.

Ryosuke Niwa

Comment 15 2019-12-04 00:07:12 PST

Committed r253096: <https://trac.webkit.org/changeset/253096>

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