Last Comment Bug 639648 - crash due to stack exhaustion in BindToTree with setUserData
: crash due to stack exhaustion in BindToTree with setUserData
Status: RESOLVED FIXED
[sg:critical?] fixed on m-c by 650493
: crash, testcase, verified1.9.2
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 650493
Blocks: CVE-2011-2378
  Show dependency treegraph
 
Reported: 2011-03-07 15:35 PST by Nils
Modified: 2011-07-12 08:31 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
-
wanted
.18+
.18-fixed
wanted


Attachments
reporter's testcase (737 bytes, text/html)
2011-03-08 10:45 PST, Alex Vincent [:WeirdAl]
no flags Details
Patch to fix (1.41 KB, patch)
2011-05-11 17:15 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bugs: review+
bugzilla: approval‑mozilla‑aurora+
dveditz: approval1.9.2.18+
Details | Diff | Review

Description Nils 2011-03-07 15:35:47 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.14) Gecko/20110221 Ubuntu/10.10 (maverick) Firefox/3.6.14
Build Identifier: 

following javascript in a HTML document will crash the browser:

tmp = document.createElement('iframe');
document.body.appendChild(tmp);
o52=document.createElement('h1');
o62=tmp.contentDocument.createElement('script');
o136=o52.setUserData('key',o62,function (o,k,d,s,ds) { o52.appendChild(d); });
o62.appendChild(o52);


Linux GDB stack backtrace:
(gdb) bt 10
#0  nsStyledElement::BindToTree (this=0x7fffd1463900, aDocument=0x0, 
    aParent=0x7fffd1494e40, aBindingParent=0x0, aCompileEventHandlers=1)
    at nsStyledElement.cpp:157
#1  0x00007ffff6c5287b in nsMappedAttributeElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 
    aBindingParent=0x0, aCompileEventHandlers=1)
    at nsMappedAttributeElement.cpp:51
#2  0x00007ffff6ca4907 in nsGenericHTMLElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 
    aBindingParent=0x0, aCompileEventHandlers=1)
    at nsGenericHTMLElement.cpp:869
#3  0x00007ffff6c4abb6 in nsGenericElement::BindToTree (this=0x7fffd1494e40, 
    aDocument=0x0, aParent=<value optimized out>, aBindingParent=0x0, 
    aCompileEventHandlers=<value optimized out>) at nsGenericElement.cpp:2657
#4  0x00007ffff6c66266 in nsStyledElement::BindToTree (this=0x7fffd1463900, 
    aDocument=0x0, aParent=0x7fffd1494e40, aBindingParent=0x0, 
    aCompileEventHandlers=1) at nsStyledElement.cpp:157
#5  0x00007ffff6c5287b in nsMappedAttributeElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 
    aBindingParent=0x0, aCompileEventHandlers=1)
    at nsMappedAttributeElement.cpp:51
#6  0x00007ffff6ca4907 in nsGenericHTMLElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 

Tested on FF4 beta and FF3 on Windows and Linux


Reproducible: Always
Comment 1 Alex Vincent [:WeirdAl] 2011-03-07 15:59:46 PST
Reading through the W3C specs, I translate the above fragment as follows:

The script element (belonging to an iframe document) is getting an h1 element (from the parent document) appended to it.  The script, in attempting to append the h1, might be adopting the parent node or importing it.

In the user data callback, d is o62, the script element.  There, the h1 element is told to append the script element which is the h1 element's parent (or should be, at some point).

What makes this surprising to me is that the first append operation (h1 as child of script) didn't throw WRONG_DOCUMENT_ERR.  They're supposedly created by different documents.

Nils:  out of curiousity, what is the "o" argument for your user data handler?
Comment 2 Nils 2011-03-07 16:12:57 PST
Alex: "o" is 5 = NODE_ADOPTED.

Interestingly the user data handler is only called once.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-07 18:13:22 PST
> They're supposedly created by different documents.

That part of the spec got changed.  Inserting into a different document auto-adopts.

Peter, want to take a look?
Comment 4 Alex Vincent [:WeirdAl] 2011-03-08 10:45:44 PST
Created attachment 517801 [details]
reporter's testcase
Comment 5 Alex Vincent [:WeirdAl] 2011-03-08 10:57:25 PST
Stepping through in the debugger, script thinks h1 is its child, and h1 thinks script is its child.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-17 09:12:27 PDT
The fundamental problem is that the check done IsAllowedAsChild can be invalidated by userdata handlers running off of adoptNode.

This affects both nsINode::ReplaceOrInsertBefore and nsINode::doInsertChildAt (even though the latter usually doesn't do an IsAllowedAsChild check.

Jonas, this should be doable with a mutation guard, but what should the expected mutation count for the adopt be?  Just 0, so if the node was in the DOM somewhere we'd still recompute stuff?
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-04-28 17:03:30 PDT
Yeah, I guess a mutation guard expecting 0 mutations is the best we can safely do.

For trunk, bug 650493 should fix this.
Comment 8 Daniel Veditz [:dveditz] 2011-05-04 10:41:50 PDT
(In reply to comment #7)
> For trunk, bug 650493 should fix this.

but we'll need a spot fix for aurora and 1.9.2
Comment 9 Daniel Veditz [:dveditz] 2011-05-05 14:00:38 PDT
In reply to comment 6:
> The fundamental problem is that the check done IsAllowedAsChild can
> be invalidated by userdata handlers running off of adoptNode.

Can we write a testcase that demonstrates this in a scarier-looking way than a stack exhaustion crash?
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-05 14:20:48 PDT
I'm not sure.  insPos also becomes invalid, but we may protect against that well enough....  As in, get misbehavior but possibly not crashes.
Comment 11 Asa Dotzler [:asa] 2011-05-10 14:35:55 PDT
Jonas, can we get an eta on a spot fix? Aurora is rapidly approaching the train station.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-11 17:15:03 PDT
Created attachment 531810 [details] [diff] [review]
Patch to fix
Comment 13 Sheila Mooney 2011-05-12 14:39:09 PDT
Jonas, can you give us a risk assessment for the patch and understanding of side effects and test cases for side effects?
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-12 17:26:08 PDT
The risk is very low. We run the exact same code in many other locations in the same function to deal with exactly the same situation. All the patch does is add the same code in a situation that we had previously missed.

The main downside is that we will be slightly slower in some cases. However these cases are fairly rare (auto-adopting a node which is in another document, not just created another document) or really really rare (pages using userdata handlers or mutation listeners which make other DOM modifications). And the perf regression is small even when it happens. So it's not something that I think we need to worry about.
Comment 15 Johnathan Nightingale [:johnath] 2011-05-16 14:42:10 PDT
Comment on attachment 531810 [details] [diff] [review]
Patch to fix

Can we get this in today?
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-16 16:30:29 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/0302a2047cac

Done!
Comment 17 Daniel Veditz [:dveditz] 2011-05-18 10:46:09 PDT
Comment on attachment 531810 [details] [diff] [review]
Patch to fix

Approved for 1.9.2.18, a=dveditz for release-drivers
Comment 18 Asa Dotzler [:asa] 2011-05-18 22:14:41 PDT
Can we get a verification, please, that this actually fixes the crash for Aurora/Beta?
Comment 19 Asa Dotzler [:asa] 2011-05-19 11:30:17 PDT
OK. The "reporter's testcase" does not crash for me in latest Aurora build.
Comment 20 Al Billings [:abillings] 2011-05-19 17:36:04 PDT
The reporter's testcase is verified to crash pre-fix and to not crash post-fix on the same machine using the nightly Aurora build (Mozilla/5.0 (Windows NT 5.1; rv:5.0a2) Gecko/20110517 Firefox/5.0a2).
Comment 21 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-06-03 11:13:51 PDT
The patch doesn't apply cleanly to 1.9.2
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-05 22:49:15 PDT
Checked in on 1.9.2 branch.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d12c5c6f9c02
Comment 23 Al Billings [:abillings] 2011-06-06 17:21:59 PDT
Verified fixed in 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.18pre) Gecko/20110606 Namoroka/3.6.18pre (.NET CLR 3.5.30729) using attached testcase. Testcase crashes 1.9.1.17 cleanly.

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