Closed Bug 606585 Opened 9 years ago Closed 9 years ago

XPCCompartmentMap uses string keys in a broken way

Categories

(Core :: XPConnect, defect, major)

x86
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: bzbarsky, Assigned: mrbkap)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

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.
dbaron hit that assert yesterday.
Well, if he hit it then he had a case where site A didn't set domain but site B did...  ;)
blocking2.0: --- → ?
We need this for beta7 per mrbkap.
blocking2.0: ? → beta7+
ETA for completion? Known effects?
ETA for completion is today. Effects will be that we'll use wrappers more correctly and put objects in the right compartments.
Attached patch Proposed fix v1 (obsolete) — Splinter Review
This should do the trick. Boris, mind taking a look to make sure I'm doing this right?
Attachment #486257 - Flags: review?(peterv)
Attachment #486257 - Flags: review?(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.
Attachment #486257 - Flags: review?(bzbarsky) → review-
Attached patch Proposed fix v2 (obsolete) — Splinter Review
This is hitting the map.Get() assertion on leaktests (!). I'm still debugging that.
Attachment #486257 - Attachment is obsolete: true
Attachment #486291 - Flags: review?(peterv)
Attachment #486291 - Flags: review?(bzbarsky)
Attachment #486257 - Flags: review?(peterv)
>+// 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.  :(
JFTR this fixes my use case (XBL in 2nd about:feeds document not running).
Attached patch Proposed fix v3Splinter Review
This addresses all comments and fixes the assertion.
Attachment #486291 - Attachment is obsolete: true
Attachment #486508 - Flags: review?(peterv)
Attachment #486508 - Flags: review?(bzbarsky)
Attachment #486291 - Flags: review?(peterv)
Attachment #486291 - Flags: review?(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.  :(
Attachment #486508 - Flags: review?(bzbarsky) → review+
Comment on attachment 486508 [details] [diff] [review]
Proposed fix v3

r=peterv IRL.
Attachment #486508 - Flags: review?(peterv) → review+
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).
Whiteboard: fixed-in-tracemonkey
Pushed to mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/d1e19c288645
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.