Closed
Bug 763225
Opened 12 years ago
Closed 12 years ago
"Assertion failure: compartment mismatched" with Components.lookupMethod(v, "onclick")
Categories
(Core :: XPConnect, defect)
Tracking
()
VERIFIED
FIXED
mozilla16
People
(Reporter: jruderman, Assigned: bzbarsky)
References
Details
(Keywords: assertion, testcase, Whiteboard: [advisory-tracking+])
Attachments
(4 files)
419 bytes,
text/html
|
Details | |
12.57 KB,
text/plain
|
Details | |
2.63 KB,
patch
|
smaug
:
review+
bholley
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
Assertion failure: compartment mismatched, at js/src/jscntxtinlines.h:237
Reporter | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
So.. We call lookupMethod on a cross-compartment wrapper. This returns what looks like a Function object created in the compartment of the caller. That is, it's created in the parent page's compartment. Then we call this method. That lands us, through NS_InvokeByIndex_P in nsInlineEventHandlersTearoff::SetOnclick and hence in nsINode::SetOnclick. At this point cx and v are both in the the parent page compartment. The nsINode::SetOnclick code looks something like this: JSObject *obj = GetWrapper(); if (!obj) { /* Just silently do nothing */ return NS_OK; } return elm->SetJSEventListenerToJsval(nsGkAtoms::onclick, cx, obj, v); because it's not expecting to be called while cx is in compartments other than the node's own compartment. This probably means it fails for XrayWrapper too, by the way... But ignoring that, now we have cx and v in one compartment (parent page) and obj in a different one (subframe). The fact that these are different compartments is the result of cpg. In any case, now it's just a time-bomb that happens to blow up when we call nsJSUtils::GetStaticScriptContext which calls JS_GetGlobalForObject which asserts that cx and the object are in the same compartment. Which they're not. So as far as fixing goes: 1) LookupMethod should perhaps enter the compartment of the underlying object while creating the function object and return a cross-compartment wrapper in this case. 2) We should figure out whether SetOn* and GetOn* need to be able to deal with being called in compartments that don't match the element's compartment and if so how they should behave.
Blocks: cpg
Assignee | ||
Comment 4•12 years ago
|
||
Looking over the code, I think we should aim to store things that are in the node's compartment in the nsIJSEventListener. What that means is that the setter needs to: 1) Enter the node's compartment as needed. 2) JS_WrapValue the argument after that, again as needed. Olli, does that seem reasonable?
Comment 5•12 years ago
|
||
I think so.
Comment 6•12 years ago
|
||
...not that I know what JS_WrapValue does.
Assignee | ||
Comment 7•12 years ago
|
||
It takes as input a jsval and a JSContext which may be in different compartments and outputs a jsval that is in the same compartment as the JSContext (by creating cross-compartment wrappers as needed).
Comment 8•12 years ago
|
||
is this a regression from compartment-per-global, or a pre-existing problem?
Assignee | ||
Comment 9•12 years ago
|
||
The fact that web content can trigger this is a regression from cpg. chrome could trigger it before as well; that part is probably a regression from bug 659350.
Blocks: 659350
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #632999 -
Flags: review?(bugs)
Attachment #632999 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 11•12 years ago
|
||
Jesse, should the testcase in this bug land as a crashtest with the patch? Or wait till later?
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Reporter | ||
Comment 12•12 years ago
|
||
Since content couldn't trigger the bug before cpg, I'd prefer for the patch to land with a testcase.
Comment 13•12 years ago
|
||
Be warned - most of the compartment mismatches that we found from CPG can also be triggered pre-cpg by using document.domain to create same-origin cross-compartment scopes.
Comment 14•12 years ago
|
||
Comment on attachment 632999 [details] [diff] [review] Take a bit more care about our compartments in SetJSEventListenerToJsval. >+ // Now ensure that we're working in the compartment of aScope from now on. >+ JSAutoEnterCompartment ac; >+ if (js::GetObjectCompartment(handler) != js::GetObjectCompartment(aScope)) { Why is this necessary? JSAutoEnterCompartment::enter is a no-op if cx->compartment == aScope->compartment(). Is the issue that we just don't care about the cx compartment if the two objects match (and we only enter the compartment to do the wrapping?). If so, I think that scoping the JSAutoEnterCompartment to the JS_WrapValue call would be clearer. Or we could just enter the compartment unconditionally. The overhead of the extra equality check is probably worse than the overhead of an unconditional compartment enter. >+ MOZ_ASSERT(tempVal.isObject()); >+ handler = &tempVal.toObject(); .toObject() JS_ASSERTs isObject, so I don't think the MOZ_ASSERT here is necessary. I don't understand the broader context of this patch, but presumably smaug does.
Attachment #632999 -
Flags: review?(bobbyholley+bmo) → review+
Comment 15•12 years ago
|
||
Comment on attachment 632999 [details] [diff] [review] Take a bit more care about our compartments in SetJSEventListenerToJsval. Does JS_WrapValue always wrap, even if the compartments are the same? And if not, is it fast when it doesn't wrap? Would be great if we could do this all unconditionally. { JSAutoEnterCompartment ac; NS_ENSURE_STATE(ac.enter(cx, aScope)); jsval tempVal = v; NS_ENSURE_STATE(JS_WrapValue(cx, &tempVal)); handler = &tempVal.toObject(); } But if JS_WrapValue does always wrap, then I guess we need to do what the patch does.
Attachment #632999 -
Flags: review?(bugs) → review+
Comment 16•12 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #15) > Comment on attachment 632999 [details] [diff] [review] > Take a bit more care about our compartments in SetJSEventListenerToJsval. > > Does JS_WrapValue always wrap, even if the compartments are the same? It does not. > And if not, is it fast when it doesn't wrap? Pretty fast. It still has to go through a few conditionals, but nothing heavyweight. > Would be great if we could do this all unconditionally. Agreed.
Assignee | ||
Comment 17•12 years ago
|
||
> Why is this necessary To avoid the cost of the JS_WrapValue in the common case. > The overhead of the extra equality check is probably worse than the overhead of an > unconditional compartment enter. In my testing, the overhead of the well-predicted branch is way lower than the cost of the JS_WrapValue call... > Pretty fast. It still has to go through a few conditionals, but nothing heavyweight. I guess it depends on your timescales. In the binding code, JS_WrapValue is a pretty noticeable cost compared to the rest of the binding code, since it involves a slow out of line function call in addition to a bunch of conditionals. But you might be right that on the timescale of this setter it doesn't matter. I'll measure, and if that's the case I'll remove the conditional goop.
Assignee | ||
Comment 18•12 years ago
|
||
And yes, this could be triggered with document.domain pre-cpg, I bet.... Jesse?
Assignee | ||
Comment 19•12 years ago
|
||
So I did some measuring. Using a typical microbenchmark setup (one element, one function, set onclick over and over again, which is dumb of course), my patch takes about 1.4 microseconds per call. If I make the compartment stuff unconditional, it gets about 10% slower (so figure about 140ns to do the ac.enter() and JS_WrapValue bits, which about matches my experiences with profiling bindings). Olli, preferences? Note that in the common case (on* being set first time) the time will probably be much larger since we have to allocate the nsJSEventListener and whatnot.
Assignee | ||
Comment 20•12 years ago
|
||
Assignee | ||
Comment 21•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5674051c17d7 I made the wrapvalue unconditional, per discussion with Olli. For now landed without the testcase due to the document.domain thing.
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla16
Assignee | ||
Comment 22•12 years ago
|
||
Comment on attachment 633178 [details] [diff] [review] Take a bit more care about our compartments in SetJSEventListenerToJsval. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 659350 User impact if declined: Mostly safe, I think, though there _may_ be a security problem when document.domain is in use. Testing completed (on m-c, etc.): Passes attached testcase. Risk to taking this patch (and alternatives if risky): Low risk, imo. But the other option is to just do nothing about this on branches. String or UUID changes made by this patch: None.
Attachment #633178 -
Flags: approval-mozilla-beta?
Attachment #633178 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Updated•12 years ago
|
Keywords: sec-review-needed
Comment 24•12 years ago
|
||
We're looking for a security rating on this bug, and also a decision from sec group as comment 22 is questioning the need for this on branches.
Comment 25•12 years ago
|
||
Comment on attachment 633178 [details] [diff] [review] Take a bit more care about our compartments in SetJSEventListenerToJsval. [Triage Comment] Dan let us know over IRC that he plans to give this an sg:crit rating, so let's land on all branches.
Attachment #633178 -
Flags: approval-mozilla-beta?
Attachment #633178 -
Flags: approval-mozilla-beta+
Attachment #633178 -
Flags: approval-mozilla-aurora?
Attachment #633178 -
Flags: approval-mozilla-aurora+
Comment 26•12 years ago
|
||
Please also prepare a patch for the ESR branch.
Assignee | ||
Comment 27•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/c01bd3028fc7 http://hg.mozilla.org/releases/mozilla-beta/rev/f40e8de1ed49
status-firefox14:
--- → fixed
status-firefox15:
--- → fixed
Assignee | ||
Comment 28•12 years ago
|
||
Comment on attachment 633178 [details] [diff] [review] Take a bit more care about our compartments in SetJSEventListenerToJsval. This patch applies just fine to ESR.
Attachment #633178 -
Flags: approval-mozilla-esr10?
Comment 29•12 years ago
|
||
Comment on attachment 633178 [details] [diff] [review] Take a bit more care about our compartments in SetJSEventListenerToJsval. Thanks, we were just waiting on the beta landing before approving for ESR - please go ahead.
Attachment #633178 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Comment 30•12 years ago
|
||
Verified fixed in 2012-06-22 Beta, Aurora, and Nightly debug builds.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 31•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-esr10/rev/a55c87cdd4db
Updated•12 years ago
|
Whiteboard: [advisory-tracking+]
Comment 32•12 years ago
|
||
Verified fixed in Firefox 10.0.6esrpre Debug 2012-07-11 using the attached testcase.
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•