914830 - Review existing JS_GetPrototype call sites for safety

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

bz was surprised to discover that JS_GetPrototype can call proxy handler hooks. It may GC and may even fail.

And, ES6 lets users implement a .getPrototypeOf proxy trap, which can mutate prototype chains at any time. This isn't implemented yet but it's something to watch out for.

We should probably review existing call sites to make sure we're coping correctly.

For example, this loop in nsDocument::Register

5217     while (protoProto) {
5218       if (protoProto == htmlProto) {
5219         break;
5220       }
5221       if (!JS_GetPrototype(aCx, protoProto, &protoProto)) {
5222         rv.Throw(NS_ERROR_UNEXPECTED);
5223         return nullptr;
5224       }
5225     }

may never terminate in ES6.

Comment 1

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

I did my best to audit all the callsites for failure/GC safety when doing bug 787856. The signature changed, so every use had to be updated. It's possible I missed something of course.

When doing that bug I tried to make provisions for scriptable getPrototypeOf, but I remember there were several issues that I decided not to deal with. I don't remember what they were though :-(.

Comment 2

[Eric Faust [:efaust]]

Bill points out that  http://mxr.mozilla.org/mozilla-central/source/js/src/jsarray.cpp#369 is another example of potential trouble. 

Although I am not particularly enamoured of the idea, I suggest an enum

ALLOW_HOOKS
DISALLOW_HOOKS

or something similar and make that a parameter to JSObject::getProto(). I think from there we should leave JS_GetPrototype invoking hooks, and, if necessary, add a friendapi thing that doesn't.

While it's ugly, it at least forces every caller to consider what behavior they want, and allows us to do squirrely things like we have pointed out.

This still won't cover non-terminating loops and the rest of the audit, but at least it gets us over this hurdle.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Eric Faust [:efaust] from comment #2)
> Bill points out that 
> http://mxr.mozilla.org/mozilla-central/source/js/src/jsarray.cpp#369 is
> another example of potential trouble.

I want to fix that in bug 895223. I just have to figure out how to do it without breaking xpconnect.

> Although I am not particularly enamoured of the idea, I suggest an enum
> 
> ALLOW_HOOKS
> DISALLOW_HOOKS
>
> or something similar and make that a parameter to JSObject::getProto().

Adding an argument doesn't seem warranted based on the 2 examples of troublesome code we've seen so far. What we have right now is two getProto() signatures:

    // infallible, no hooks, can't gc, asserts that this JSObject isn't a proxy
    JSObject *getProto() const;

    // fallible, calls hooks, can gc, works for all objects
    static bool getProto(JSContext *cx, HandleObject obj, MutableHandleObject protop);

I think that's probably sufficient; that is, I don't think the DISALLOW_HOOKS variant is useful.

We could rename the existing signatures to indicate that the former is dangerous because it contains a tricky assertion that callers need to know about, and the latter is dangerous because (in the future) it can call into arbitrary JS code.

But when we add the .getPrototypeOf trap, we'll still need to review existing callers. Renaming would be like putting up warning signs. Maybe a good idea; but it doesn't save a bunch of work.

Comment 4

[Jeff Walden [:Waldo]]

There are JSObject::getProto calls in SuppressDeletedPropertyHelper that look highly dodgy on this front.  No way can random [[Prototype]] arbitrary-script trap invocations be okay in what's supposed to be entirely invisible caching hackwork.