606585 - XPCCompartmentMap uses string keys in a broken way

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We have cases when identical URI strings mean different principals (about: stuff, say) and other cases where different such strings mean same principal (file:// URIs if you flip the strict file pref).

So the XPCCompartmentMap should be checking the principal, not just the string, and ideally hashing the string while taking into account what NS_SecurityHashURI looks like.  And then using nsIPrincipal::Equals for the equality compare.

Incidentally, the assert in the if(!equals) case in xpc_CreateGlobalObject is wrong (has a stray '!'), which has been hiding some of the issues related to this bug.

Comment 1

[Andreas Gal :gal]

dbaron hit that assert yesterday.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Well, if he hit it then he had a case where site A didn't set domain but site B did...  ;)

Comment 3

[Johnny Stenback (:jst)]

We need this for beta7 per mrbkap.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

ETA for completion? Known effects?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

ETA for completion is today. Effects will be that we'll use wrappers more correctly and put objects in the right compartments.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v1

This should do the trick. Boris, mind taking a look to make sure I'm doing this right?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486257 [details] [diff] [review]
Proposed fix v1

>+     *                  comartments, as for sandboxes.

"compartments".


> xpc_CreateGlobalObject(JSContext *cx, JSClass *clasp,
>+    NS_ASSERTION(NS_IsMainThread(), "using a principal off the main thread?");

Should that be NS_ABORT_IF_FALSE?

>+    NS_PRECONDITION(principal, "bad key");

Likewise.

>+            return false;

That's not an nsresult.

>+xpc_CreateMTGlobalObject(JSContext *cx, JSClass *clasp,
>+    NS_ASSERTION(!NS_IsMainThread(), "not using a principal on the main thread?");

Again, should that abort?

>+            return false;

nsresult?

> nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext * aJSContext,
>+    NS_ASSERTION(!aExtraPtr || aPrincipal, "must be able to find a compartment");

The '!' looks wrong to me.

>@@ -3204,35 +3213,21 @@ xpc_CreateSandboxObject(JSContext * cx, 

>+    rv = xpc_CreateGlobalObject(cx, &SandboxClass, new Identity(), principal,

Please hold a ref to the Identity object before passing it to callees?

>+++ b/js/src/xpconnect/src/xpcprivate.h
>+    PtrAndPrincipalHashKey(nsISupports *aPtr, nsIPrincipal *aPrincipal)

Assert aPrincipal not null?

>+      return NS_SUCCEEDED(mPrincipal->Equals(aKey->mPrincipal, &eq)) && eq;

Doesn't that do the wrong thing when .domain is set one one but not the other?

Seems like you want to SecurityCompareURIs the codebases instead.

All this stuff should be inside XPCONNECT_STANDALONE ifdefs, if we still support that.  Do we?  I hope not...

The rest looks fine.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v2

This is hitting the map.Get() assertion on leaktests (!). I'm still debugging that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

>+// Needed to distinguish multiple compartments with the same origin for each

s/for/from/

The +3 in HashKey could use a comment.

I don't see how this can be hitting those asserts.  :(

Comment 10

[[e-mail]]

JFTR this fixes my use case (XBL in 2nd about:feeds document not running).

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v3

This addresses all comments and fixes the assertion.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486508 [details] [diff] [review]
Proposed fix v3

>+        mSavedHash = mURI ? NS_SecurityHashURI(mURI) : 0;

Shouldn't the part after ':' be NS_PTR_TO_INT32(mPtr.get()) >> 2 or some such?

Please document why mSavedHash is needed; it's really not the most obvious thing in the world, right?  ;)

r=me.  I'm sorry again about the CAPS hoops needed here.  :(

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 486508 [details] [diff] [review]
Proposed fix v3

r=peterv IRL.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/aa2f365c42eb

Note that the checked-in patch differs from the last patch posted in the bug (cosmetic changes only, most notably, xpc_CreateGlobalObject's ordering of principal and ptr now match nsIXPConnect::InitClassesWithNewWrappedGlobal's).

Comment 15

[Johnny Stenback (:jst)]

Pushed to mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/d1e19c288645