950407 - Behavior change setting __proto__ on a proxy with an ArrayBuffer as target

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Current behavior on this:

  var ab = ArrayBuffer(5);
  var p = new Proxy(ab, {})
  var ps = Object.getOwnPropertyDescriptor(Object.prototype, "__proto__").set;
  ps.call(p, {});

is *crickets*.  Old behavior before bug 926012 on this was "TypeError: Object.prototype.__proto__ setter called on incompatible ArrayBuffer".

Stepping through a bit, something looks pretty rotten.  JSObject::setProto doesn't have a obj->getTaggedProto().isLazy(), so we fall into SetClassAndProto with a proxy, which seems pretty wrong -- direct proxies should be forwarding to JSObject::setProto on their target, and that doesn't happen.

Or am I missing/forgetting something I knew when I reviewed patches before?

Comment 1

[Eric Faust [:efaust]]

OK, so it looks like there are really two problems at work here.

The first is silly and trivial: I forgot to change ScriptedDirectProxies to using TaggedProto::LazyProto, so it just sets to proto on the proxy to null, which isn't the desired behavior.

The second is more insidious. Even fixing that, we are then in the JSObject::setProto() system, which doesn't account for any of the checks in ProtoSetterImpl.

In particular, I worry about the CheckAccess() call there. Is this something we should be worried about? It seems like maybe it's not something that should go away? Bobby, do you understand enough of that system to know?

If the CheckAccess call isn't scary, then just moving the ArrayBuffer check to the top of JSObject::setProto() and making ScriptedDirectProxies actually forward properly should be enough to fix this.

Comment 2

[Eric Faust [:efaust]]

Attachment: Fix?

I had this thought that maybe we can just move the CheckAccess call at well. I don't see how this is any different than what we used to have.

Bobby, if the code here looks good, then we can just r+ this and stop worrying about it.

This also fixes the original ArrayBuffer complaint.

Comment 3

[Eric Faust [:efaust]]

I /think/ after reading some of the security callbacks, that we need to do a similar change for the forwarding proxies in JSObject::getProto(). If so, that mistake has been there for some time. I guess the right place to land that fix is in bug 950897?

Again, all of this needs a once over from someone who actually understands what's supposed to be going on, but it seems the only logical way to structure all of this, and indeed the way it seems to be structured is to have the callback answer the question "is the access OK for this property ON THIS OBJECT" regardless of the access that originated it.

Maybe none of this matters since we can't pierce security wrappers anyways?

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8348327 [details] [diff] [review]
Fix?

Review of attachment 8348327 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, the CheckAccess call is pretty useless. It could probably go away. It's all tied up in the last bundle of CAPS crap, which can't quite tetris into oblivion until we do bug 794943.

In the mean time, this patch is fine.

::: js/src/jsobjinlines.h
@@ +428,5 @@
> +     * have a mutable [[Prototype]].
> +     */
> +    if (obj->is<js::ArrayBufferObject>()) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_INCOMPATIBLE_PROTO,
> +                             "Object", "__proto__ setter", "ArrayBuffer");

Is "__proto__ setter" still the right string to pass here?

Comment 5

[Eric Faust [:efaust]]

https://bugzilla.mozilla.org/show_bug.cgi?id=950407#c4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for one of the most thorough tree burnings I've seen in recent memory. Achievement unlocked, well played sir :D
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a613dc5e5c7

https://tbpl.mozilla.org/php/getParsedLog.php?id=32597630&tree=Mozilla-Inbound

Comment 7

[Eric Faust [:efaust]]

Attachment: fixTotalMeltdown.patch

Yeah, OK, that patch ended the world. Let's try again with less striking startup-blocking incorrectness.

Remove the CheckAccess call from JSObject::setProto and replace it with an explicit check for Location objects, as discussed on IRC.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8359942 [details] [diff] [review]
fixTotalMeltdown.patch

Review of attachment 8359942 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but I want an sr from Blake on the Location hack.

Blake, the basic deal is that JSACC_PROTO only ever mattered for the Location object, and just handling that explicitly in the JS engine is easier than trying to rejigger all the CheckAccess stuff.

::: js/src/jsobjinlines.h
@@ +441,5 @@
> +    }
> +
> +    /*
> +     * Explicityly disallow mutating the [[Prototype]] of Location objects
> +     * for flash-related security reasons.

How about:

Explicityly disallow mutating the [[Prototype]] of Location objects until Location moves to WebIDL bindings and gets proper [Unforgeable] semantics.

Then, file a follow up bug to remove this hack, and make it depend on bug 832014.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8359942 [details] [diff] [review]
fixTotalMeltdown.patch

I'm fine with this. JSACC_PROTO should die in a fire as fast as possible.

Comment 10

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/19cb3daa91a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b97134e81798

take 2

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19cb3daa91a7
https://hg.mozilla.org/mozilla-central/rev/b97134e81798