Consider the following example: var set = new XML('<a><b>text</b></a>').children('b'); var counter = 0; Object.prototype.unrooter getter = function() { ++counter; if (counter == 5) { set[0] = new XML('<c/>'); if (typeof gc == "function") { gc(); var tmp = Math.sqrt(2), tmp2; for (i = 0; i != 50000; ++i) tmp2 = tmp / 2; } } return undefined; } set.unrooter(); Currently it crashes when accessing GC-ed xml object. This happens because xml_getMethod does not root the first child of XML-list when it searches for a method there. The script exploits it defining a getter in Object.prototype that at the right moment removes the child from the list cutting all its roots. Thus the following forced GC collects the child leading to a crash during toString conversion of the child.

The previous test case on Linux may not crash as Math.sqrt(2) when reinterpreted as JSObject could produce a valid map. So here is a better one: var expected = "SOME RANDOM TEXT"; var set = <a><b>{expected}</b></a>.children('b'); var counter = 0; function unrooter_impl() { return String(this); } Object.prototype.unrooter getter = function() { ++counter; if (counter == 7) return unrooter_impl; if (counter == 5) { set[0] = new XML('<c/>'); if (typeof gc == "function") { gc(); var tmp = 1e500, tmp2; for (i = 0; i != 50000; ++i) tmp2 = tmp / 1.1; } } return undefined; } var actual = set.unrooter(); print(actual === expected);

The fix tries to be proactive to be new-feature-proof. But it also demonstrates that getMethod is API with bad signature. It should really demand rooted JSObject ** as argument to place the result.

Comment on attachment 239145 [details] [diff] [review] Fix > static JSObject * > xml_getMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp) Please file a bug on changing the getMethod signature. We should bite the bullet and always use out parameters, and for type safety do something even stronger than what you proposed (special attributes checked by a separate tool): require a "rooter" or "holder" type to be passed by its address. In my Unix kernel hacking days, with good example code from Sun and 4.4BSD, SGI moved toward a uniform pattern for all out parameters: always return error status via the return value (errno in the kernel case), with out parameters for result pointers. In Mozilla, with XPCOM, this has led to bloat for useless status return values and extra memory bandwidth for reference-counted pointers via out parameters, and it costs in source and compiled code and in cycles. Many of the abuses of XPCOM should not even add a reference, rather they can return a reference guaranteed to live as long as the object whose method was being called. With exact GC, things are different, and I'm in favor of moving to out parameters. > { > JSXML *xml; >- jsval fval; >+ JSTempValueRooter tvr; >+ jsval roots[2]; >+#define FUN_ROOT 0 >+#define OBJ_ROOT 1 Nit: line up the values. /be

Comment on attachment 239145 [details] [diff] [review] Fix >+ jsval roots[2]; >+#define FUN_ROOT 0 >+#define OBJ_ROOT 1 Or even nicer, to avoid #undef unsightliness, use an anonymous enum. /be

(In reply to comment #3) > (From update of attachment 239145 [details] [diff] [review] [edit]) > > static JSObject * > > xml_getMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp) > > Please file a bug on changing the getMethod signature. See bug 353365. In fact this how I started the patch but then it grew too big to qualify for a security-only fix :( > With exact GC, things are different, and I'm in favor of moving to out > parameters. IMO this has nothing to do with the way to pass parameters. Rather it is about giving the callee an access to a rooted location that the caller has to have in any case. In C this translates to passing the address. In C++ one can still use return values but hide the rooting besides a wrapper class similar to auto_ptr<T>.

Patch to commit that uses anonymous enum instead of preprocessor defines.

I committed the patch from comment 6 to the trunk: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.126; previous revision: 3.125 done

Restoring lost blocking flag

(In reply to comment #5) > > With exact GC, things are different, and I'm in favor of moving to out > > parameters. > > IMO this has nothing to do with the way to pass parameters. Rather it is about > giving the callee an access to a rooted location that the caller has to have in > any case. In C this translates to passing the address. We're talking about C code, so it does have to do with the way to pass parameters in this codebase. > In C++ one can still use > return values but hide the rooting besides a wrapper class similar to > auto_ptr<T>. Sure. Should we switch to a sane dialect of C++? I'm not against it by nature. SpiderMonkey has lots of consumers (some unknown to us), but in 2006 one can finally count on most of the ISO C++ standard to work on GCC and MSVC-modern or VS.whatever. I'd want to have a sane plan before committing any changes, and run it by the m.d.t.js-engine newsgroup and as many embedders as we can reach first. /be

verified fixed 1.9 20060921 windows/mac*/linux

This is also an exploitable GC hazard in XML. The patch from 1.8.1 bug 354145 depends on this bug fixed.

Fix b apply to branch? If so please nom..

Comment on attachment 239229 [details] [diff] [review] Fix b This is a fix for exploitable GC hazard.

Comment on attachment 239229 [details] [diff] [review] Fix b Approved for RC2

Comment on attachment 239229 [details] [diff] [review] Fix b Approved for RC3

I committed the patch from comment 6 to MOZILLA_1_8_BRANCH: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.50; previous revision: 3.50.2.49 done

Comment on attachment 239229 [details] [diff] [review] Fix b approved for 1.8.0 branch, a=dveditz for drivers

The patch for trunk/1.8.1 does not apply to 1.8.0 as it lacks JS_ARRAY_LENGTH macro and there AFAICS whitespace issues with GetFinctiom. So here is an update.

Comment on attachment 241750 [details] [diff] [review] Fix b for 1.8.0 branch moving approval to the correct branch patch

I committed the patch from comment 19 to MOZILLA_1_8_0_BRANCH: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.15.2.20; previous revision: 3.50.2.15.2.19 done

verified fixed 1.8 20061010 windows/mac*/linux

verified fixed 1.8.0.8 on mac/win/linux firefox_1.8.0.8pre_2006101114_dbg firefox_1.8.0.8pre_2006101114_opt

Bug not relevant to aviary/moz1.7 branches

/cvsroot/mozilla/js/tests/e4x/extensions/regress-353165.js,v <-- regress-353165.js moved to extensions/ due to getter