Bugzilla@Mozilla – Bug 568303
GC hazards in JS_SetWatchPoint with proxies
Last modified: 2010-10-30 18:14:22 PDT
Summon comment box
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.
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);
Created attachment 447718 [details] [review] v1 The patch adds missing rooting.
Comment on attachment 447718 [details] [review] v1 Very old code, would love to overhaul it (watchpoints rule, this old impl does not). Good patch, thanks. /be
http://hg.mozilla.org/tracemonkey/rev/ea4ad4f59286
http://hg.mozilla.org/mozilla-central/rev/ea4ad4f59286
Created attachment 477900 [details] [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 <
Created attachment 477902 [details] [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 on attachment 477900 [details] [review] fix for 192 Approved for 1.9.2.11, a=dveditz for release-drivers
Comment on attachment 477902 [details] [review] fix for 191 Approved for 1.9.1.14, a=dveditz for release-drivers
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/550135aa114c http://hg.mozilla.org/releases/mozilla-1.9.1/rev/059a056b8e88
Created attachment 481171 [details] [review] 1.8.0 version