1101123 - Make it impossible to use JS_Define* to redefine a non-configurable getter on a native object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Then we can do the optimization in bug 1100757.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'm going to scope this down a bit, because the full generality here is not actually viable.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't allow redefining the getter of a non-configurable accessor property on native objects, even via the low-level JSAPI methods

Comment 3

[Boris Zbarsky [:bzbarsky]]

So apparently sandbox also violates this invariant.  Specifically, in test_writeToGlobalPrototype.js we have code like this:

50   Cu.evalInSandbox("Object.defineProperty(this, 'y', {get: function() { this.gotten = true; return 3; }})", s);

with this patch, this throws, because sandbox_addProperty, which is called _after_ the prop has been added, does something to the proto, and then does:

445     if (!JS_GetPropertyDescriptorById(cx, obj, id, &pd))
446         return false;
447     unsigned attrs = pd.attributes() & ~(JSPROP_GETTER | JSPROP_SETTER);
448     if (!JS_DefinePropertyById(cx, obj, id, vp,
449                                attrs | JSPROP_PROPOP_ACCESSORS,
450                                JS_PROPERTYOP_GETTER(writeToProto_getProperty),
451                                JS_PROPERTYOP_SETTER(writeToProto_setProperty)))
452         return false;

which is totally redefining the accessor for a non-configurable property.  Bill, what's this code trying to do and can we make it stop violating basic invariants?

Comment 4

[Boris Zbarsky [:bzbarsky]]

And similar for Xrays.  In test_xrayToJS.xul we have:

    trickyObject.getterSetterProp = 'redefined';

(though for some reason the actual line number SpiderMonkey reports is off-by-one).

In this case "trickyObject" is actually trickyArray, which had this done to it:

375                  Object.defineProperty(trickyArray, 'getterSetterProp', { get: function() { return 3; }, set: function(x) {}});

Anyway, we land in js::Proxy::set which gets a property descriptor, finds nothing (because it's an Xray that filters out the prop), and does JSObject::defineGeneric to define a value property.  That lands us in xpc::JSXrayTraits::defineProperty which then tried to redefine the property on the target.  But the property on the target is non-configurable, so this throws.

Bobby, what are the intended semantics of Xrays here, exactly? Because what they're doing right now (reconfiguring content-side non-configurable properties) is not so great...

Comment 5

[Bobby Holley (:bholley)]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #4)
> Bobby, what are the intended semantics of Xrays here, exactly? Because what
> they're doing right now (reconfiguring content-side non-configurable
> properties) is not so great...

I think we discussed this in the DOM bindings meeting sometime in June, right? I think we concluded that there are no easy solutions here and that we should just propagate the non-configurable exception to the Xray caller (which I _thought_ was what we were doing, but apparently not).

The other option would be to use the expando object in this case but that's probably more trouble and confusion than it's worth.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: With the xray test adjusted to the behavior we want and now have

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8526204 [details] [diff] [review]
With the xray test adjusted to the behavior we want and now have

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

r=me on the test fix. Thanks for doing that.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The sandbox_addProperty code is trying to intercept new properties added to certain globals and modify them to have a special getter and setter. I can imagine a few ways to deal with this:

1. Add a new hook that's similar to addProperty but runs before the property is added. I'm guessing there will be a lot of resistance to adding a new hook though.

2. It sounds like this problem doesn't necessarily negate the optimization in bug 1100757 since (I think) the property is re-defined before the JIT ever sees it. However, I'm guessing that you want to make JS_Define* fail when re-defining a non-configurable property, so we'd have to hack around that somehow in the sandbox_addProperty case. It would probably be a little gross.

3. We could use a proxy instead of a class hook. However, we don't support proxies as globals, so we'd have to implement support for that. I suspect it would be a ton of work.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> since (I think) the property is re-defined before the JIT ever sees it.

In this case, yes.  But yes, I want to make this not allowed at all, because otherwise it's too big a footgun.

For now, can we not just switch from JS_NULL_OBJECT_OPS to ops that have define* hooks that do what we want for SandboxWriteToProtoClass?  We'd probably need to expose baseops::Define* via some sort of friend API so we can forward to it, but other than that...

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #9)
> For now, can we not just switch from JS_NULL_OBJECT_OPS to ops that have
> define* hooks that do what we want for SandboxWriteToProtoClass?

This seems about right, to have the sandbox act as a proxy that intercepts behaviors it wants to wrap.

> We'd
> probably need to expose baseops::Define* via some sort of friend API so we
> can forward to it, but other than that...

I think this can be avoided with a delegate object of sorts.  I'd really like to avoid exposing baseops stuff if at all possible; it's too low-level to be a sensible API, it's hard to describe, hard to know when to use, and just not a good API even for the friendzone incest.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So I was thinking we wouldn't need the extra object, but we do, because sandbox wants to forward some, but not all, definitions to the proto global..

Comment 12

[Boris Zbarsky [:bzbarsky]]

So... I'm not sure how to create sane define/lookup hooks without access to JSObject::lookupGeneric/defineGeneric.  Define I might be able to do via jsapi, but I see nothing reasonable for lookup...  Jeff, do we want to just add friend API for that?

Comment 13

[Jeff Walden [:Waldo]]

Temporary friendapi seems reasonable, if it's needed.  It looks like JS_Lookup{Property,Element} would do what you want, but you presumably have only jsid, and JS_LookupPropertyById is not (!) the same semantics as those two.  So you probably need something new.

I thought this might overlap with jorendorff work on adjusting uses of lookups to not be so stupid sometimes, but IRC suggests not, so no worries on that front, seemingly.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: With a hackaround for the sandbox bits

Comment 15

[Eric Faust [:efaust]]

Comment on attachment 8529314 [details] [diff] [review]
With a hackaround for the sandbox bits

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

Man, this is ugly, but it's also what we agreed upon. Sigh. I hope this is removed quickly.

Comment 16

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c44465f2a483

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c44465f2a483