crash due to stack exhaustion in BindToTree with setUserData

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Nils, Assigned: sicking)

Tracking

({crash, testcase, verified1.9.2})

unspecified
crash, testcase, verified1.9.2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5+ fixed, blocking2.0 -, status2.0 wanted, blocking1.9.2 .18+, status1.9.2 .18-fixed, status1.9.1 wanted)

Details

(Whiteboard: [sg:critical?] fixed on m-c by 650493)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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
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?
(Reporter)

Comment 2

6 years ago
Alex: "o" is 5 = NODE_ADOPTED.

Interestingly the user data handler is only called once.
> 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?
Created attachment 517801 [details]
reporter's testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, testcase
Stepping through in the debugger, script thinks h1 is its child, and h1 thinks script is its child.
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?
Group: core-security
Assignee: nobody → jonas
blocking1.9.2: --- → ?
status1.9.2: --- → wanted
status2.0: --- → wanted
status-firefox5: --- → affected
tracking-firefox5: --- → +
Whiteboard: [sg:critical?]
Blocks: 648065
Yeah, I guess a mutation guard expecting 0 mutations is the best we can safely do.

For trunk, bug 650493 should fix this.
(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
blocking1.9.2: ? → .18+
blocking2.0: --- → -
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?
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

6 years ago
Jonas, can we get an eta on a spot fix? Aurora is rapidly approaching the train station.
OS: Linux → All
Created attachment 531810 [details] [diff] [review]
Patch to fix
Attachment #531810 - Flags: review?(Olli.Pettay)

Updated

6 years ago
Attachment #531810 - Flags: review?(Olli.Pettay) → review+
Attachment #531810 - Flags: approval-mozilla-aurora?

Comment 13

6 years ago
Jonas, can you give us a risk assessment for the patch and understanding of side effects and test cases for side effects?
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.
status1.9.1: --- → wanted
Depends on: 650493
Whiteboard: [sg:critical?] → [sg:critical?] fixed on m-c by 650493
Version: unspecified → 5 Branch
Comment on attachment 531810 [details] [diff] [review]
Patch to fix

Can we get this in today?
Attachment #531810 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/0302a2047cac

Done!
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox5: affected → fixed
Resolution: --- → FIXED
Attachment #531810 - Flags: approval1.9.2.18?
Comment on attachment 531810 [details] [diff] [review]
Patch to fix

Approved for 1.9.2.18, a=dveditz for release-drivers
Attachment #531810 - Flags: approval1.9.2.18? → approval1.9.2.18+

Comment 18

6 years ago
Can we get a verification, please, that this actually fixes the crash for Aurora/Beta?

Comment 19

6 years ago
OK. The "reporter's testcase" does not crash for me in latest Aurora build.
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).
Version: 5 Branch → unspecified
The patch doesn't apply cleanly to 1.9.2
Checked in on 1.9.2 branch.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d12c5c6f9c02
status1.9.2: wanted → .18-fixed
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.
Keywords: verified1.9.2
Group: core-security
You need to log in before you can comment on or make changes to this bug.