Closed Bug 950407 Opened 6 years ago Closed 6 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: efaust)



(Whiteboard: [qa-])


(1 file, 1 obsolete file)

Current behavior on this:

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

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?
Flags: needinfo?(efaustbmo)
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.
Flags: needinfo?(efaustbmo) → needinfo?(bobbyholley+bmo)
Blocks: 926012
Attached patch Fix? (obsolete) — Splinter Review
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.
Assignee: nobody → efaustbmo
Attachment #8348327 - Flags: review?(bobbyholley+bmo)
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 on attachment 8348327 [details] [diff] [review]

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?
Attachment #8348327 - Flags: review?(bobbyholley+bmo) → review+
Flags: needinfo?(bobbyholley+bmo)
Backed out for one of the most thorough tree burnings I've seen in recent memory. Achievement unlocked, well played sir :D
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.
Attachment #8348327 - Attachment is obsolete: true
Attachment #8359942 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8359942 [details] [diff] [review]

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.
Attachment #8359942 - Flags: superreview?(mrbkap)
Attachment #8359942 - Flags: review?(bobbyholley+bmo)
Attachment #8359942 - Flags: review+
Comment on attachment 8359942 [details] [diff] [review]

I'm fine with this. JSACC_PROTO should die in a fire as fast as possible.
Attachment #8359942 - Flags: superreview?(mrbkap) → superreview+
Blocks: 957688
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 968097
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.