Closed Bug 636818 Opened 9 years ago Closed 9 years ago

Crash [@ js::CallJSPropertyOpSetter] or [@ js_SetProperty]

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- .x+

People

(Reporter: gkw, Assigned: jorendorff)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [fixed-in-tracemonkey])

Crash Data

Attachments

(2 files)

a = evalcx('');
a.__proto__ = ''.__proto__;
a.length = 3;

is a testcase which crashes js debug (@ js::CallJSPropertyOpSetter) and opt shells (@ js_SetProperty) on TM changeset f007657da01e without -m nor -j.

Hoping for at least .x+, this is flooding a newly-improved fuzzer, locking s-s to err on the side of caution.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   56817:9ec91c8f9b8e
user:        Blake Kaplan
date:        Fri Oct 29 10:42:35 2010 -0700
summary:     Bug 596031 - 'this' is wrong in getters and setters when a proxy object is on the prototype chain. r=brendan/jorendorff/gal
Unhiding and taking.
Assignee: general → jorendorff
Group: core-security
blocking2.0: ? → .x+
The first thing js_SetPropertyHelper does is:

    protoIndex = js_LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags,
                                            &pobj, &prop);
    if (protoIndex < 0)
        return JS_FALSE;
    if (prop) {
        if (!pobj->isNative()) {
            if (pobj->isProxy()) {
                AutoPropertyDescriptorRooter pd(cx);
                if (!JSProxy::getPropertyDescriptor(cx, pobj, id, true, &pd))
                    return false;

                if (pd.attrs & JSPROP_SHARED)
                    return CallSetter(cx, obj, id, pd.setter, pd.attrs, pd.shortid, strict, vp);

                if (pd.attrs & JSPROP_READONLY) {
                    if (strict)
                        return obj->reportReadOnly(cx, id);
                    if (cx->hasStrictOption())
                        return obj->reportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
                    return true;
                }
            }
            ...

In this case, obj is a sandbox and pobj is its prototype, which is a proxy, a cross-compartment wrapper of String.prototype. The property being set is "length", which is both SHARED and READONLY. It is a SHARED data property.

Ideally we would not have such crazy properties, but since we have them, this code needs to cope.

I have a tiny patch, but it needs a bit of testing and bug 636879 is more pressing right now. Will post tomorrow.
Attached patch v1Splinter Review
Attachment #515760 - Flags: review?(gal)
Attachment #515760 - Flags: review?(gal) → review+
Attachment #515760 - Flags: approval2.0?
Attachment #515760 - Flags: approval2.0? → approval2.0+
backed out, breaks the build, removing approval

http://hg.mozilla.org/mozilla-central/rev/7536e407b706
Attachment #515760 - Flags: approval2.0+
This caused bustage because the landed patch removed security/manager/boot/public/nsISSLStatusProvider.idl (not included in the patch here).
Sigh. I have no idea how that could possibly have happened. hg rebase craziness, I guess. Sorry.
http://hg.mozilla.org/tracemonkey/rev/121e1af64e31
Whiteboard: [fixed-in-tracemonkey]
Original landing: http://hg.mozilla.org/mozilla-central/rev/25027d672f50
Backout: http://hg.mozilla.org/mozilla-central/rev/7536e407b706
Relanding: http://hg.mozilla.org/mozilla-central/rev/121e1af64e31
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Crash Signature: [@ js::CallJSPropertyOpSetter] [@ js_SetProperty]
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-636818.js.
Flags: in-testsuite+
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.