Closed
Bug 795532
Opened 12 years ago
Closed 12 years ago
Inconsistency in cross-compartment map
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: billm, Assigned: billm)
Details
Attachments
(1 file, 1 obsolete file)
1.78 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
I think I found a problem at the end of JS_TransplantObject: if (origobj->compartment() != destination) { RootedObject newIdentityWrapper(cx, newIdentity); AutoCompartment ac(cx, origobj); if (!JS_WrapObject(cx, newIdentityWrapper.address())) return NULL; if (!origobj->swap(cx, newIdentityWrapper)) return NULL; origobj->compartment()->crossCompartmentWrappers.put(ObjectValue(*newIdentity), origv); } We have some object O, we create a wrapper W for it, then we swap the contents of W with some other object W'. Finally, we need to update the CCW map to say that W' is the wrapper for O, rather than W. However, when we're creating the wrapper W, it may be that W doesn't directly wrap O. That's because JSCompartment::wrap first unwraps O before wrapping it. So it may be that W is really a wrapper for some other object O'. The problem is that we still update the wrapper map to say that W' is a wrapper for O. It seems like we should really say that it's a wrapper for O'. That's what JSCompartment::wrap does, at least.
Attachment #666137 -
Flags: review?(bobbyholley+bmo)
Comment 1•12 years ago
|
||
Comment on attachment 666137 [details] [diff] [review] patch Hm, so the two places in Gecko where we use a same-compartment wrapper as an identity object are (1) Xray waivers and (2) outer windows. For (1), we never call transplant. For (2), we're saved by the fact that JSCompartment::wrap passes stopAtOuter=true to Unwrap(), meaning that we don't actually unwrap the outer window proxy. So is the concern in: > + // JS_WrapObject isn't guaranteed to return a wrapper for > + // newIdentity. It might end up wrapping a slightly different object. Purely theoretical?
Attachment #666137 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 2•12 years ago
|
||
OK, I wasn't sure if this could actually happen. I found the issue by mucking with a test case, and it asserted unexpectedly. I still think it would be better to change the behavior here. It doesn't cost us anything, and what we're doing right now seems like it might cause problems in the future. I can change the comment or something to better reflect reality.
Group: core-security
Comment 3•12 years ago
|
||
So, after clarifying this in my head a little bit: The contract we have is that the Prewrap callback returns the identity object as far as the cross-compartment wrapper map is concerned. For outer windows, we handle this by just never unwrapping the outer window in the first place (stopAtOuter = false). For Xray Waivers, we explicitly rewrap the object in a waiver during the prewrap callback (giving us the appropriate separate identity). I'm pretty sure stuff will break if these invariants aren't true. So I think we should actually be asserting that the before and after versions of this patch give the same results. That is to say, that key == ObjectValue(*newIdentity). Does that make sense?
Assignee | ||
Comment 4•12 years ago
|
||
Sounds good to me.
Attachment #666137 -
Attachment is obsolete: true
Attachment #672535 -
Flags: review?(bobbyholley+bmo)
Updated•12 years ago
|
Attachment #672535 -
Flags: review?(bobbyholley+bmo) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/67728fe694e2
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/67728fe694e2
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•