714663 - Crash [@ js::HeapPtr] or [@ js::PropDesc::initialize] with Proxy

Status: RESOLVED DUPLICATE of bug 720305

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stacks

try {
    (function() {
        x = Proxy.create((function() {
            return {
                defineProperty: function(name, desc) {
                    Object.defineProperty(x, name, desc)
                },
                getOwnPropertyDescriptor: function() {
                    return this
                },
                set: undefined
            }
        })());
        x.t = eval
    })()
} catch (e) {}


crashes js debug shell on m-c changeset d77b056ed4bd without any CLI arguments at js::HeapPtr<js::BaseShape, unsigned long>::operator js::BaseShape* and crashes js opt shell at js::PropDesc::initialize

Setting s-s because $pc seems to access $ecx, which is a weird memory address of 0xb8e58955.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   82693:49bd0c9665e5
user:        Bobby Holley
date:        Thu Dec 15 11:40:57 2011 -0800
summary:     Bug 702491 - Don't set JSPROP_READONLY for accessor properties. r=Waldo

Comment 1

[David Mandelin [:dmandelin]]

Looks like it's related to write barriers. AFAICT it's just a normal bad pointer read crash, and so not a vulnerability. Bill, could you check this out?

Comment 2

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

This doesn't appear to be related to write barriers. It has something to do with a property descriptor with an invalid setter. I'll look into it tomorrow, I guess.

Comment 3

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

Well, here's what's going wrong. At some point we call ParsePropertyDescriptorObject with the object shown in the test, where the "set" property is undefined. It calls PropDesc::initialize. This function looks up the "set" property, and sets d->set to undefined and puts JSPROP_SETTER in d->flags. Then when ParsePropertyDescriptorObject converts the PropDesc to a PropertyDescriptor, it calls d->setter(), which returns NULL because d->set == undefined.

Later, we end up in ProxyHandler::set. It gets the same PropertyDescriptor and says:
        if (!desc.setter)
            desc.setter = JS_StrictPropertyStub;
So it sets setter to JS_StrictPropertyStub. However, JSPROP_SETTER is still in desc.attrs. Jason says this isn't supposed to happen. Later on, in PropDesc::initFromPropertyDescriptor, we do this:
        set = ((desc.attrs & JSPROP_SETTER) && desc.setter)
              ? CastAsObjectJsval(desc.setter)
              : UndefinedValue();
Since the flag is set, we call CastAsObjectJsval on JS_StrictPropertyStub. Later on, this value is exposed to JS code, which leads to a crash.

My initial thought was that if the "set" part of the property descriptor is undefined, then we shouldn't be parsing it as if there were a setter. So I made this change:

--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -2034,17 +2034,17 @@ PropDesc::initialize(JSContext *cx, cons
         attrs &= ~JSPROP_READONLY;
         if (checkAccessors && !checkGetter(cx))
             return false;
     }
 
     /* 8.10.7 step 8 */
     if (!HasProperty(cx, desc, ATOM_TO_JSID(cx->runtime->atomState.setAtom), &v, &found))
         return false;
-    if (found) {
+    if (found && !v.isUndefined()) {
         hasSet = true;
         set = v;
         attrs |= JSPROP_SETTER | JSPROP_SHARED;
         attrs &= ~JSPROP_READONLY;
         if (checkAccessors && !checkSetter(cx))
             return false;
     }
 
However, this caused some test failures (namely ecma_5/Object/15.2.3.6-new-definition.js). I don't understand the test, so I think I'm going to have to ask for help. Jeff, do you know what the right fix is?

Comment 4

[Jeff Walden [:Waldo]]

It looks like ProxyHandler::set was depending on a JSPROP_READONLY being in attrs here, and just happened to blindly stumble into "correct" behavior.  Sigh.

The reason the undefined check you tried doesn't work is that property descriptor objects are categorized based on their properties, not the property values.  So { get: v } is an accessor, { set: v } is also one, and { get: v, set: v2 } is one as well, regardless what the v/v2 values are.  (This assumes there are no get/set properties along the prototype chains.)  Only an undefined value or a callable value will actually pass the immediate tests, and proceed to being used, tho.

The problem with the readonly bit removed is that in some cases it's necessary to have JSPROP_SETTER with a null setter being the same as no setter at all.  get+undefined set and set+undefined get could avoid this case, so there's really only one instance that needs the attr set with null in the get/set field: { get: undefined, set: undefined }.  It's stupid.  It's also the spec.

So, what to do for ProxyHandler::set...

It looks like the method is implementing ES5 8.12.5 [[Put]], except it's doing it in our own special non-stepwise way, probably because of our own non-standard special cases.  The method's algorithm should be refactored to be more like the one in the spec, as much as possible.  So behavior should depend on how desc is categorized -- as a data descriptor, or as an accessor descriptor.  Control flow in the accessor case shouldn't depend on whether JSPROP_READONLY is present or not.

Anyway.  The bug is in ProxyHandler::set, and the fix is to make the algorithm implemented there more like the one in 8.12.5.

Comment 5

[Jeff Walden [:Waldo]]

Blake, is this a dup of the bug/patch I reviewed a couple days ago?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, it is. It seems like (from comment 4) we should file a followup on making JSProxyHandler::set look more like the spec, though.

Comment 7

[Christian Holler (:decoder)]

A testcase for this bug was already added in the original bug (bug 720305).