468538 - Crash [@ nsParser::ParseFragment] setting innerHTML in mixed-content document

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

ML Parsing Error: prefix not bound to a namespace
Location: 
Line Number 1, Column 1:
<xul:box><div xmlns="http://www.w3.org/1999/xhtml">
^

###!!! ASSERTION: ParseFragment requires a fragment content sink: 'fragSink', file /Users/jruderman/central/parser/htmlparser/src/nsParser.cpp, line 2073

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/xpcom/nsCOMPtr.h, line 796

Regression from bug 460437, perhaps?

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Most likely.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

So... This fixes the crash. But it's kinda ugly.

Basically Parse() is failing but returning NS_OK. I think that is the real problem, but I can't imagine how much other code might depend on that currently. So this may be the simplest thing to do for now. mrbkap, what do you think?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 352020 [details] [diff] [review]
Patch, v1

I'd rather not do this if possible. Calling into the parser when no sink is null is a dangerous proposition at best. Can we, instead detect that the sink became null after the call to parse (assert that this is the XML mode) and bail early if so?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 353134 [details] [diff] [review]
Patch, v1.1

Yeah, let's try this.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 324151b8a4b9 to mozilla-central.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 445c44c3c804 to mozilla-1.9.1

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Verified on trunk and 1.9.1 with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090131 Minefield/3.2a1pre ID:20090131034630

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre ID:20090201020604

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre ID:20090201033606

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090201020520

Comment 9

[Bob Clary [:bc] (inactive)]

1.9.0 !exploitable report:
PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gkparser!nsParser::ParseFragment

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(responding as "private" since comment 9 is private).

This isn't exploitable -- we're calling a virtual function through a guaranteed null pointer. This is a false positive like the one that dveditz pointed out on the s-g list.

Comment 11

[Daniel Veditz [:dveditz]]

We've taken bug 460437 in the 1.9.0.11 release, we should probably take this regression fix before we ship a release with the problem.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for 1.9.0 branch

Comment 13

[Samuel Sidler (old account; do not CC)]

Comment on attachment 376091 [details] [diff] [review]
Patch for 1.9.0 branch

Approved for 1.9.0.11. a=ss for release-drivers

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed rev 3.412 of nsParser.cpp to cvs.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.11 with my debug build: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051111 GranParadiso/3.0.11pre. No crash.

Comment 16

[Daniel Veditz [:dveditz]]

Doesn't affect the 1.8.1 branch