Last Comment Bug 714663 - Crash [@ js::HeapPtr] or [@ js::PropDesc::initialize] with Proxy
: Crash [@ js::HeapPtr] or [@ js::PropDesc::initialize] with Proxy
Status: RESOLVED DUPLICATE of bug 720305
[sg:dupe 720305] js-triage-needed
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz 702491
  Show dependency treegraph
 
Reported: 2012-01-02 12:06 PST by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-04 19:10 PST (History)
8 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
-
wontfix
+
fixed
fixed
unaffected


Attachments
stacks (12.80 KB, text/plain)
2012-01-02 12:06 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details

Description Gary Kwong [:gkw] [:nth10sd] 2012-01-02 12:06:57 PST
Created attachment 585317 [details]
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] 2012-01-03 17:43:21 PST
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 [PTO to Dec5] Bill McCloskey (:billm) 2012-01-03 18:13:54 PST
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 [PTO to Dec5] Bill McCloskey (:billm) 2012-01-04 11:04:28 PST
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] (remove +bmo to email) 2012-01-04 12:10:50 PST
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] (remove +bmo to email) 2012-01-27 14:18:04 PST
Blake, is this a dup of the bug/patch I reviewed a couple days ago?
Comment 6 Blake Kaplan (:mrbkap) 2012-01-28 05:49:34 PST
Yeah, it is. It seems like (from comment 4) we should file a followup on making JSProxyHandler::set look more like the spec, though.

*** This bug has been marked as a duplicate of bug 720305 ***
Comment 7 Christian Holler (:decoder) 2013-01-04 19:10:58 PST
A testcase for this bug was already added in the original bug (bug 720305).

Note You need to log in before you can comment on or make changes to this bug.