1403716 - Fix the underlying issues that make the patch for bug 1355109 crashy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Filing this as a separate, security-sensitive, bug, because there may be security issues here and I don't really want to mark all of bug 1355109 as security-sensitive.

So here's what I know right now.  With the patch for bug 1355109 landed and the steps to reproduce from bug 1386490, we end up in js::RemapWrapper with wobjArg a dead object proxy, at least in my local Linux opt build.  Then origTarget is null (because dead object proxies do not have a wrappedObject and aren't even Wrapper instances!), the MOZ_ASSERT fails (assuming we're in a debug build; I can't reproduce in debug), and then eventually we crash messing with that null object or similar.

Now it seems to me that fundamentally passing a non-wrapper to RemapWrapper a wobjArg is wrong.

The caller is js::RecomputeWrappers.  This builds up a list of things to recompute and then calls RemapWrapper on them all.  So either it's getting the list wrong to start with or wrappers are getting nuked partway through the list creation or something.

Bug 1386490 comment 8 mentions XrayTraits::cloneExpandoChain as a thing that might be creating these dead object proxies, or the objects that will eventually become dead object wrappers.  cloneExpandoChain is called from ReparentWrapper, so on node adoption or document.open or so.  RecomputeWrappers happens on document.domain set...

In theory it _is_ possible to land in XrayTraits::cloneExpandoChain with one of the wrapperHolder objects being a dead object proxy.  For example, we could have nuked an extension compartment that had expandos on an Xray.  If that happens, then "exclusiveWrapper" in cloneExpandoChain would still be a dead object proxy.  But that wouldn't explain why RecomputeWrappers would see that proxy as a wrapper...

Anyway, that's all I have so far.  I really wish I could reproduce this under rr, but I haven't managed to so far, either opt or debug.

Comment 1

[Boris Zbarsky [:bzbarsky]]

And of course Bugzilla's UI manages to lose the security flag...

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, so a bit more data.  The first place I have so far where things go awry is when we land in js::RemapWrapper with wobjArg a cross-compartment wrapper.  Wrapper::wrappedObject(wobj) (aka origTarget) is a dead object proxy.  So we have a cross-compartment wrapper for a dead object proxy, which seems moderately suspect.

In this case origTarget == newTarget by the way.

Anyway, RemapWrapper proceeds to call JSCompartment::rewrap(), passing newTarget.  When passed a dead object proxy, rewrap() will always return a dead object proxy.  Then we proceed to do:

    MOZ_ASSERT(Wrapper::wrappedObject(wobj) == newTarget);
    // Update the entry in the compartment's wrapper map to point to the old
    // wrapper, which has now been updated (via reuse or swap).
    MOZ_ASSERT(wobj->is<WrapperObject>());
    if (!wcompartment->putWrapper(cx, CrossCompartmentKey(newTarget), ObjectValue(*wobj)))
        MOZ_CRASH();

That first assert would fail in a debug build (converted it to a printf in my opt build to check), because wobj is a dead object proxy not a wrapper.  The second assert would also fail.  Then we put the dead object proxy into the compartment's hashtable.

Now the next time we RecomputeWrappers() we lose, because we have nonwrappers in the wrapper hashtable.

I'm still looking into how we end up with a cross-compartment wrapper with a dead object proxy as its target.  That seems like it shouldn't happen, since we should never have wrappers for wrappers and only wrappers should become dead object proxies.  And we certainly shouldn't be creating a wrapper for an _existing_ dead object proxy.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, so now I simply don't know what's going on.  :(

I added logging to BaseProxyHandler::objectMoved to log the "proxy" and "old" values.  I added logging at the end of js::NewDeadProxyObject to log the value it's returning.  I added logging in ProxyObject::nuke to log "this".

None of the pointers any of that logs are equal to the dead-proxy newTargetArg I'm seeing passed to RemapWrapper.  So I really don't know where that dead object proxy is coming from.  I _really_ wish I could reproduce this under rr...

Comment 4

[Daniel Veditz [:dveditz]]

Some of the crashes (~15%) appeared to be use-after-frees.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Talking about this with Jason, and doing more logging, we've made some progress.

The sequence of events seems to be:

1)  We call dom::ReparentWrapper for an HTMLDocument object.  This is coming from document.open().
2)  We JS_CloneObject the old reflector.
3)  We call XrayUtils::CloneExpandoChain(aCx, newobj, aObj) where "newobj" is the clone
    and aObj is the old reflector.
4)  In CloneExpandoChain, the patch for bug 1355109 added code at https://dxr.mozilla.org/mozilla-release/rev/48668c5c603e7ef7a722d4376dea0100baec0a9b/js/xpconnect/wrappers/XrayWrapper.cpp#1329-1337
    that creates a cross-compartment wrapper (Xray) for "dst".  In this case, that's newobj, the new reflector.
5)  After this, ReparentWrapper calls JS_TransplantObject with the old reflector as "origobj"
    and the new one as "target".
6)  In JS_TransplantObject the compartments are not the same.  So we look up "origobj" in
    the cross-compartment wrapper map for "target"'s compartment, and find it there.  I don't
    know yet why that CCW exists at this point.
7)  We nuke the CCW we found and swap() it with "target". This means that the Xray we created in step 4
    now has a dead proxy target.
8)  We now remap wrappers to "origobj".  But not to "target".  And now we're in the situation
    of the start of comment 2: we have an Xray to a dead object proxy.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and if we were doing a node adoption, not a document.open, then it would be really easy to have a CCW in step 6 and the rest of the steps in comment 5 would happen like they do now, I expect.  So trying to change whatever happens in the document.open case to avoid having a CCW there is not the right solution here, because there are legitimate cases when we definitely _should_ have a CCW.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, actually, I know where the CCW from step 6 comes from.  When document.open creates a new Window global, we grab a reference to the document and store it in a slot on the global, because Window.document is [StoreInSlot] in the IDL.  This creates the CCW, because at that point the document's reflector is still associated with the old window but we need it in the scope of the new window.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the fundamental problem is that on entry to JS_TransplantObject we have wrappers for both "origobj" and "newtarget" from the same (Xray) compartment.

Plausible fixes for this problem:

1)  Change ReparentWrapper to JS_TransplantObject _before_ doing CloneExpandoChain.  I believe this is
    the right general fix, because it also fixes the problem in bug 1404107.  I'm a little concerned about
    how safe that change is and whether we can backport it to 57, much less 56.
2)  Change the setup from bug 1355109 to not involve creation of an Xray as part of CloneExpandoChain.
    I'm not quite sure how to do that offhand.
3)  Make JS_TransplantObject somehow handle this situation.  It's not immediately clear how.
    We _could_ change the "existing CCW" path to turn all existing CCWs to "newtarget" into
    dead proxies.  That would avoid the "ccw to dead proxy" problem.  If we do it unconditionally,
    not just in the "existing CCW to origobj" case, it would also solve the problem where we
    have an Xray to origobj _and_ an Xray to newtarget from the same compartment, try to remap
    the former to point to newtarget, and fail the assertion at http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/js/src/proxy/CrossCompartmentWrapper.cpp#622-623

The problem with option 3 is that it will now have a dead object proxy, not an Xray, in the slot pointed to by the expando chain stuff that is trying to keep the xray alive (and the whole reason CloneExpandoChain creates an xray in the first place).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, I meant to say... the "turn it into a dead proxy" idea is jorendorff's, and maybe there is some other way to make JS_TransplantObject handle this.  But we haven't come up with one yet.

Comment 10

[Jason Orendorff [:jorendorff]]

For option 2, what if we add an attachExpandoObject signature that reuses an existing holder object?

We would use that in cloneExpandoChain, when we're preparing to reparent src into dst. The new expando chain and the old chain would then point to the same set of holder objects. When we actually transplant, the Xray CCWs will be changed to point to dst, and the new chain will then be correct.

(And if we fail before we get to transplanting, all these objects in the new chain are dead anyway.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> The new expando chain and the old chain would then point to the same set of holder objects.

Ah, because the holder lives in the Xray compartment anyway, right.  So we'd have the new expando chain pointing to the same holder (via a new CCW) which points to the same Xray.

I like it!

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, I tried the suggestion from comment 2, but that makes even more of the test in bug 1404107 fail.  The reason that happens is that when we end up doing RemapWrapper on the Xray that actually creates a _new_ Xray wrapper (without a holder allocated for it or anything) and then swaps it with the old thing.  In the process the pointer to the old holder (stored in JSSLOT_XRAY_HOLDER on the Xray) is lost.  And the expando object hangs off the holder in the "exclusive owner" case.  So the upshot is that we lose expandos on adopt.  This remapping machinery really doesn't seem like it works all that well for wrappers that store state (which Xrays do)....

Now per bug 1404107 we may lose expandos on adopt _anyway_ depending on what CCWs are around, so maybe this is OK.  But the behavior change makes this far from a risk-free fix, unfortunately.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

> OK, I tried the suggestion from comment 2

I meant comment 10...

Comment 14

[Boris Zbarsky [:bzbarsky]]

The patches in bug 1404107 fix this bug, by implementing comment 8 option 1.  I am not adding a dependency so there will be no indication that bug 1404107 is security-related.

Comment 15

[Daniel Veditz [:dveditz]]

That should make this FIXED on m-c.

Comment 16

[Julien Cristau [:jcristau]]

bug 1404107 was uplifted to 57