568303 - GC hazards in JS_SetWatchPoint with proxies

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

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]

Attachment: v1

The patch adds missing rooting.

Comment 3

[Brendan Eich [:brendan]]

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 4

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/ea4ad4f59286

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ea4ad4f59286

Comment 6

[Igor Bukanov]

Attachment: 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]

Attachment: 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]]

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]]

Comment on attachment 477902 [details] [diff] [review]
fix for 191

Approved for 1.9.1.14, a=dveditz for release-drivers

Comment 10

[Igor Bukanov]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/550135aa114c

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/059a056b8e88

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 version