Closed
Bug 606585
Opened 14 years ago
Closed 14 years ago
XPCCompartmentMap uses string keys in a broken way
Categories
(Core :: XPConnect, defect)
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)
38.02 KB,
patch
|
bzbarsky
:
review+
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
dbaron hit that assert yesterday.
Reporter | ||
Comment 2•14 years ago
|
||
Well, if he hit it then he had a case where site A didn't set domain but site B did... ;)
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 4•14 years ago
|
||
ETA for completion? Known effects?
Assignee | ||
Comment 5•14 years ago
|
||
ETA for completion is today. Effects will be that we'll use wrappers more correctly and put objects in the right compartments.
Assignee | ||
Comment 6•14 years ago
|
||
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)
Reporter | ||
Comment 7•14 years ago
|
||
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-
Assignee | ||
Comment 8•14 years ago
|
||
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)
Reporter | ||
Comment 9•14 years ago
|
||
>+// 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•14 years ago
|
||
JFTR this fixes my use case (XBL in 2nd about:feeds document not running).
Assignee | ||
Comment 11•14 years ago
|
||
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)
Reporter | ||
Comment 12•14 years ago
|
||
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+
Assignee | ||
Comment 13•14 years ago
|
||
Comment on attachment 486508 [details] [diff] [review] Proposed fix v3 r=peterv IRL.
Attachment #486508 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 14•14 years ago
|
||
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
Comment 15•14 years ago
|
||
Pushed to mozilla-central. http://hg.mozilla.org/mozilla-central/rev/d1e19c288645
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 623810
You need to log in
before you can comment on or make changes to this bug.
Description
•