Last Comment Bug 568303 - GC hazards in JS_SetWatchPoint with proxies
: GC hazards in JS_SetWatchPoint with proxies
Status: RESOLVED FIXED
[sg:critical?] fixed-in-tracemonkey [...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: harmony:proxies
  Show dependency treegraph
 
Reported: 2010-05-26 12:19 PDT by Igor Bukanov
Modified: 2010-10-30 18:14 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+
.11+
.11-fixed
.14+
.14-fixed


Attachments
v1 (3.73 KB, patch)
2010-05-27 00:55 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
fix for 192 (2.79 KB, patch)
2010-09-23 05:37 PDT, Igor Bukanov
igor: review+
dveditz: approval1.9.2.11+
Details | Diff | Splinter Review
fix for 191 (2.86 KB, patch)
2010-09-23 05:53 PDT, Igor Bukanov
igor: review+
dveditz: approval1.9.1.14+
Details | Diff | Splinter Review
1.8.0 version (1.45 KB, patch)
2010-10-06 03:00 PDT, Martin Stránský
igor: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2010-05-26 12:19:01 PDT
JS_SetWatchPoint contains the following fragment:

    if (!js_LookupProperty(cx, obj, propid, &pobj, &prop))
        return JS_FALSE;
...
    } else if (pobj != obj) {
...
        if (pobj->isNative()) {
        } else {
            if (!pobj->getProperty(cx, propid, &value) ||
                !pobj->getAttributes(cx, propid, prop, &attrs))
...
        }
        pobj->dropProperty(cx, prop);

        /* Recall that obj is native, whether or not pobj is native. */
        if (!js_DefineNativeProperty(cx, obj, propid, value, getter, setter,
                                     attrs, flags, shortid, &prop)) {
            return JS_FALSE;
        }
 
There are 2 bugs here. First dropProperty is called after getProperty call that can run arbitrary scripts (either via Proxies or liveconnect on older branches). This could be problematic if obj becames thread-shared but in general this is not exploitable in the default firefox configuration.

Second the value passed to the getProperty call is not rooted. With proxies this becomes a GC hazard since the proxy can overwrite both getProperty to return newborn object and getAttributes to force the GC to collect it before it is stored using js_DefineNativeProperty. Note that this is not exploitable with liveconnect since the implementation of getAttributes in the Java bridge is trivial and cannot trigger the GC.

Here is an example demonstrating the second bug:

function force_gc() {
    buffer = null;
    gc();
    buffer = [];
    for (var i = 0; i != 1e5; ++i)
        buffer.push("123456".substring(1));
}

var buffer = [];
for (var i = 0; i != 4000; ++i)
    buffer.push({});

// Point obj to a proxy where the get method creates anew object and
// getOwnPropertyDescriptor collects it when called from 

var obj = { __proto__ : Proxy.create({
          has: function(name) { return name == "x"; },
          get: function(name) { print(111);return {}; },
          getOwnPropertyDescriptor: function (name) { force_gc(); return {}; },
        })};

Object.prototype.watch.call(obj, 'x', function() {});

uneval(obj.x);

Regarding branch nomination: currently I do not see how the bug could affect the older branches, but it is better to play safe and make sure that the problem is fixed everywhere rather than hoping that the analysis is correct.
Comment 1 Igor Bukanov 2010-05-26 13:28:35 PDT
There is another bug in SetWatchPoint. It does not root the result of js_ValueToStringId. With proxies this is exploitable as there is a code path where the id, when passed to one of the proxy methods, is not rooted while a code is executed. With liveconnect AFAICS there is no such path. Here is a test case:

function force_gc() {
    buffer = null;
    gc();
    buffer = [];
    for (var i = 0; i != 1e5; ++i)
        buffer.push(i + 0.3);
}

var buffer = [];
for (var i = 0; i != 1 << 14; ++i)
    buffer.push("abcdefgh".substring(2));

// Point obj to a proxy where the get method creates anew object and
// getOwnPropertyDescriptor collects it when called from 

var obj = { __proto__ : Proxy.create({
          has: function(name) { return true; },
          get get() { force_gc(); return function(name) { return 0; }},
          getOwnPropertyDescriptor: function (name) { return {}; },
        })};

Object.prototype.watch.call(
    obj,
    { toString: function() { return "abcdefgh".substring(1); }},
    function() {});

uneval(obj);
Comment 2 Igor Bukanov 2010-05-27 00:55:51 PDT
Created attachment 447718 [details] [diff] [review]
v1

The patch adds missing rooting.
Comment 3 Brendan Eich [:brendan] 2010-05-27 12:20:29 PDT
Comment on attachment 447718 [details] [diff] [review]
v1

Very old code, would love to overhaul it (watchpoints rule, this old impl does not). Good patch, thanks.

/be
Comment 6 Igor Bukanov 2010-09-23 05:37:19 PDT
Created attachment 477900 [details] [diff] [review]
fix for 192

The patch is a trivial backport to 192 branch to account to various demacrofication that happens on the track. Here is a plain diff between patches:

> diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
> --- a/js/src/jscntxt.h
> +++ b/js/src/jscntxt.h
> @@ -1343,6 +1343,7 @@ class JSAutoTempValueRooter
>  
>      jsval value() { return mTvr.u.value; }
>      jsval *addr() { return &mTvr.u.value; }
> +    void set(jsval v) { mTvr.u.value = v; }
>  
>    protected:
>      JSContext *mContext;
12c15
< @@ -800,12 +800,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
---
> @@ -792,12 +792,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
16c19
< +    AutoValueRooter idroot(cx);
---
> +    JSAutoTempValueRooter idroot(cx);
27c30
< @@ -838,35 +840,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
---
> @@ -830,35 +832,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
32c35
< +        AutoValueRooter valroot(cx);
---
> +        JSAutoTempValueRooter valroot(cx);
37,39c40,42
<          if (pobj->isNative()) {
< -            value = SPROP_HAS_VALID_SLOT(sprop, pobj->scope())
< -                    ? pobj->lockedGetSlot(sprop->slot)
---
>          if (OBJ_IS_NATIVE(pobj)) {
> -            value = SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj))
> -                    ? LOCKED_OBJ_GET_SLOT(pobj, sprop->slot)
41,42c44,45
< +            valroot.set(SPROP_HAS_VALID_SLOT(sprop, pobj->scope())
< +                        ? pobj->lockedGetSlot(sprop->slot)
---
> +            valroot.set(SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj))
> +                        ? LOCKED_OBJ_GET_SLOT(pobj, sprop->slot)
44,47c47,50
<              getter = sprop->getter();
<              setter = sprop->setter();
<              attrs = sprop->attributes();
<              flags = sprop->getFlags();
---
>              getter = sprop->getter;
>              setter = sprop->setter;
>              attrs = sprop->attrs;
>              flags = sprop->flags;
75d77
<
Comment 7 Igor Bukanov 2010-09-23 05:53:22 PDT
Created attachment 477902 [details] [diff] [review]
fix for 191

The fix for 191 has to account for more macro removals but beyond that the backport was trivial. Here is a diff between 192 and 191 versions of the patches:

4c4
< @@ -1343,6 +1343,7 @@ class JSAutoTempValueRooter
---
> @@ -1068,6 +1068,7 @@ class JSAutoTempValueRooter
15,16c15
< @@ -792,12 +792,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
<      if (!obj)
---
> @@ -751,12 +751,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
17a17
>      }
25c25
<          propid = js_CheckForStringIndex(propid);
---
>          CHECK_FOR_STRING_INDEX(propid);
29,30c29,30
<      /*
< @@ -830,35 +832,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
---
>      if (!js_LookupProperty(cx, obj, propid, &pobj, &prop))
> @@ -777,35 +779,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
54,57c54,57
< -            if (!pobj->getProperty(cx, propid, &value) ||
< -                !pobj->getAttributes(cx, propid, prop, &attrs)) {
< -                pobj->dropProperty(cx, prop);
< +            pobj->dropProperty(cx, prop);
---
> -            if (!OBJ_GET_PROPERTY(cx, pobj, propid, &value) ||
> -                !OBJ_GET_ATTRIBUTES(cx, pobj, propid, prop, &attrs)) {
> -                OBJ_DROP_PROPERTY(cx, pobj, prop);
> +            OBJ_DROP_PROPERTY(cx, pobj, prop);
59,60c59,60
< +            if (!pobj->getProperty(cx, propid, valroot.addr()) ||
< +                !pobj->getAttributes(cx, propid, NULL, &attrs)) {
---
> +            if (!OBJ_GET_PROPERTY(cx, pobj, propid, valroot.addr()) ||
> +                !OBJ_GET_ATTRIBUTES(cx, pobj, propid, NULL, &attrs)) {
67c67
< -        pobj->dropProperty(cx, prop);
---
> -        OBJ_DROP_PROPERTY(cx, pobj, prop);
Comment 8 Daniel Veditz [:dveditz] 2010-09-24 10:42:49 PDT
Comment on attachment 477900 [details] [diff] [review]
fix for 192

Approved for 1.9.2.11, a=dveditz for release-drivers
Comment 9 Daniel Veditz [:dveditz] 2010-09-24 10:43:04 PDT
Comment on attachment 477902 [details] [diff] [review]
fix for 191

Approved for 1.9.1.14, a=dveditz for release-drivers
Comment 11 Martin Stránský 2010-10-06 03:00:31 PDT
Created attachment 481171 [details] [diff] [review]
1.8.0 version

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