Closed Bug 795532 Opened 8 years ago Closed 8 years ago

Inconsistency in cross-compartment map


(Core :: JavaScript Engine, defect)

Not set





(Reporter: billm, Assigned: billm)



(1 file, 1 obsolete file)

Attached patch patch (obsolete) — 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 on attachment 666137 [details] [diff] [review]

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)
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
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?
Attached patch assertion patchSplinter Review
Sounds good to me.
Attachment #666137 - Attachment is obsolete: true
Attachment #672535 - Flags: review?(bobbyholley+bmo)
Attachment #672535 - Flags: review?(bobbyholley+bmo) → review+
Closed: 8 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.