627954 - Fix compartment problems with nsXPConnect::VariantToJS et al

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

In theory, nsXPConnect::VariantToJS and all the methods it calls should use 'scope' to determine the correct parent/compartment of any gc-thing created.  Its not too hard to find places where it doesn't do this (where the current context is used, which can have an unrelated scope chain) and this can lead to compartment mismatches (see bug 625251).  Rather than following the transitive closure of this function, I'd like to just remove the 'scope' parameter and ensure that callers have entered the desired compartment with the desired scope chain (which was the fix for bug 625251).  This seems preferable to walking around with a loaded gun.

As a prerequisite, we need to push a dummy frame in AutoCompartment::enter when the origin/destination compartments are the same but the target scope chains' globals are different.

Comment 1

[Luke Wagner [:luke]]

Attachment: part 0: push dummy frame when scope chain changes (but compartment stays the same)

Comment 2

[Luke Wagner [:luke]]

Attachment: part 1: take out compartment hack (asserts b/c no request), just getPrivate()

Comment 3

[Luke Wagner [:luke]]

Attachment: part 2: s/JS_GetGlobalObject/JS_GetGlobalForScopeChain/ in a few places

Comment 4

[Luke Wagner [:luke]]

Attachment: part 3: enter the right compartment for NativeData2JS/VariantData2JS

So actually removing the scope argument isn't right because sometimes the target scope is different than the current scope chain (although in the same compartment).  This can cause, e.g., QueryInterface to mutate the wrong XPCWrappedNative.  Thus, this patch keeps the explicit scope, but puts in AutoEnterCompartment where missing (so, fixes potential bugs) and asserts the  same-compartment-ness property stated above.

Comment 5

[Daniel Veditz [:dveditz]]

Not sure this is exploitable, but if it is it's probably exploitable on the 1.9.x branches and we won't be able to use compartments to fix it there. CC'ing moz_bug_r_a4 in case he wants to play with this.

Comment 6

[Luke Wagner [:luke]]

Green on try.

Assuming some of the fixes in these patches can cause compartment mismatches, then this should be as exploitable as any compartment mismatch.  I don't believe branches are affected.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 507524 [details] [diff] [review]
part 0: push dummy frame when scope chain changes (but compartment stays the same)

I forgot that this patch isn't actually necessary once I backed off the 'explicit scope parameter removal' idea.  Also, until we remove dummy frames (bug 625199), it could add a non-trivial perf overhead.

Comment 8

[Luke Wagner [:luke]]

Attachment: part 2: s/JS_GetGlobalObject/JS_GetGlobalForScopeChain/ in a few places (v.2)

Updating; a few new uses of GetNativeGlobal have popped up.  In theory, all these usage sites are potentially getting the wrong scope.

Comment 9

[Luke Wagner [:luke]]

As comment 6 says, this used to be green on try but now asserts at the
  PR_Assert("!scope || scope->compartment() == GetJSContext()->compartment")
added to NativeData2JS.

I think the problem is the recent change in GetWrappedNativeOfJSObject:
  JSObject* funObjParent = funobj->getParent()->unwrap();
which ultimately causes mCurrentJSObject to be the unwrapped object (i.e., mCurrentJSObject == mFlattenedJSObject).

It seems that we need to either fix the problem warranting
  http://hg.mozilla.org/tracemonkey/diff/a2825fbe23e3/js/src/xpconnect/src/xpcwrappednative.cpp
a different way or stop using GetCurrentJSObject as the scope passed to Native*To*.  Note: practically all uses of GetCurrentJSObject are for Native*To* functions, so this is suspicious.

Comment 10

[Luke Wagner [:luke]]

Also, this assert is catching (much earlier) the compartment mismatch in bug 629822 which itself apparently is hitting a lot for sfink with Firebug.

Comment 11

[Luke Wagner [:luke]]

Has patches, but missing the key patch.

Comment 14

[Luke Wagner [:luke]]

Attachment: part 3: WIP 1

I spent a bit of time trying to understand what is meant by all the JSObject*'s maintained by the XPCCallContext (viz., mCurrentJSObject, mOperandJSObject, mFlattenedJSObject, mCallee).  My conclusion is that 3 of these fields (mCallee, mCurrentJSObject, mOperandJSObject) can be replaced by a single new field mScopeForNewJSObjects which is mostly what mOperandJSObject was but it gets set in cases where it would currently be left null.

A nice resulting simplification is that I can go back and remove the explicit 'scope' param but, instead of using JS_GetScopeChainForGlobal (which was problematic), just use the new mScopeForNewJSObjects field.

I still need to try-server, but the patch seems to run a browser fine and fix the Firebug/AdBlock compartment asserts (once you comment out an unrelated compartment assert in JS_GetStringCharsAndLength that sfink patched in another bug that escapes me atm).

Comment 15

[Luke Wagner [:luke]]

Attachment: part 2: use the right compartment for NativeData2JS et al

Removed a few overzealous asserts in the WIP patch.  Fixed TEST-UNEXPECTED-PASS whose assertion incidentally was fixed by the patch (for reference, its the crash test from bug 512815).  With that, looks green on try, but pushing again to verify.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 508953 [details] [diff] [review]
part 2: s/JS_GetGlobalObject/JS_GetGlobalForScopeChain/ in a few places (v.2)

>diff --git a/dom/base/nsIScriptContext.h b/dom/base/nsIScriptContext.h
>-// a7139c0e-962c-44b6-bec3-e4166bfe84eb
>+// 90989b94-f40e-4da7-939a-82fdfdc1c867
> #define NS_ISCRIPTCONTEXT_IID \
>-{ 0xa7139c0e, 0x962c, 0x44b6, \
>-  { 0xbe, 0xc3, 0xe4, 0x16, 0x6b, 0xfe, 0x84, 0xeb } }
>+{ 0x90989b94, 0xf40e, 0x4da7, \
>+  { 0x93, 0x9a, 0x82, 0xfd, 0xfd, 0xc1, 0xc8, 0x67 } }

We probably can't change nsIScriptContext at this late point. We need an nsIScriptContext_MOZILLA_2_0 or somesuch instead.

>diff --git a/dom/base/nsJSEnvironment.cpp b/dom/base/nsJSEnvironment.cpp
>-nsJSContext::GetNativeGlobal()
>+nsJSContext::GetGlobalForScopeChain()
>+{
>+    JSAutoRequest ar(mContext);
>+    return ::JS_GetGlobalForScopeChain(mContext);

Let's not add new ::JS junk where it's not needed. IMO it's pretty ugly.

>+void *
>+nsJSContext::GetContextGlobal()

We should file a bug on figuring out who really wants the outer window here. It's almost always not what you want.

Clearing the review request based on seeing the new interface.

Comment 17

[Luke Wagner [:luke]]

(In reply to comment #16)
> Let's not add new ::JS junk where it's not needed. IMO it's pretty ugly.

Agreed; just copying surrounding style.

> We should file a bug on figuring out who really wants the outer window here.
> It's almost always not what you want.

Sure; this patch isn't actually a pre-requisite, but just a set of related bugs found while rooting around.

Comment 18

[Luke Wagner [:luke]]

Filed bug 632250.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 510085 [details] [diff] [review]
part 2: use the right compartment for NativeData2JS et al

What's with the /* TODO */ comments?

Should probably do PR_ASSERT -> NS_ASSERTION (or at least NS_ABORT_IF_FALSE).

Looks good otherwise.

Comment 20

[Luke Wagner [:luke]]

(In reply to comment #19)
> What's with the /* TODO */ comments?

Oh yeah, I put those in to remind myself to write comments explaining what they meant.

> Should probably do PR_ASSERT -> NS_ASSERTION (or at least NS_ABORT_IF_FALSE).

Ah, I see the style guideline now.  Bummer, PR_ASSERT was so much shorter...

Comment 22

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/d518bc36d7b4
http://hg.mozilla.org/tracemonkey/rev/02be97f9ef0d

Comment 23

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d518bc36d7b4
http://hg.mozilla.org/mozilla-central/rev/02be97f9ef0d