Here is another unrooted access bug in jsarray.c. array_unshift contains the pattern: if (!OBJ_GET_PROPERTY(cx, obj, id, &v)) return JS_FALSE; if (!IndexToId(cx, last + argc, &id2)) return JS_FALSE; if (!OBJ_SET_PROPERTY(cx, obj, id2, &v)) return JS_FALSE; If last + argc exceeds JSVAL_INT_MAX, IndexToId would GC allocate string for the index. Thus if v holds unrooted value after OBJ_GET_PROPERTY and that allocation would trigger GC, then OBJ_SET_PROPERTY would store GC-collected value. The example bellow demonstrates the bug in jsshell compiled with TOO_MUCH_GC. On my box it gives segmentation fault on the last expected === actual compare operation. The same pattern present in jsarray.c also exists in array_splice, array_concat, array_slice, array_extra, but in this case a bug demo would have to loop through array holes until JSVAL_INT_MAX which would be a log story. var N = 0x80000002; var array = Array(N); array[N - 1] = 1; array[N - 2] = 2; // Set getter not to wait untill engine loops through 2^31 holes in the array. var LOOP_TERMINATOR = "stop_long_loop"; array.__defineGetter__(N - 2, function() { throw "stop_long_loop"; }); var prepared_string = String(1); array.__defineGetter__(N - 1, function() { var tmp = prepared_string; prepared_string = null; return tmp; }) try { array.unshift(1); } catch (e) { if (e !== LOOP_TERMINATOR) throw e; } var expected = "1"; var actual = array[N]; print(expected === actual);

When I was looking at jarray.c for unrooted bugs on previous occasions, I assumed that IndexToId would never call GC so it was seemed OK to use local jsval to hold potentially unrooted jsval before passing it to OBJ_SET_PROPERTY. Given that I would really suggest to use explicit local roots for all the cases where jsarray.c currently use just plain local C jsval variable.

Didn't we fix array_reverse already? We are too friend to make consistent changes in all the similar places. I at least have no excuse, since I know that IndexToId can GC. Thanks again, Igor. /be

(In reply to comment #2) > Didn't we fix array_reverse already? > Yes, that was fixed. Now I have another question. What exactly roots the results of IndexToId when they are atoms? Just cx->lastAtom or there is another refrence that can not be overwritten by a "smart" getter/setter?

(In reply to comment #3) > Now I have another question. What exactly roots the > results of IndexToId when they are atoms? Last-ditch GCs do not collect atoms. See GC_KEEP_ATOMS and JS_{,UN}KEEP_ATOMS. /be

At a quick glance the 1.0 branch appears to be safe (the IndexToId happens before the get/set rather than between), but nominating for that branch so we remember to take a closer look.

Straightforward explicit local rooting, yay. /be

Comment on attachment 200961 [details] [diff] [review] proposed fix sr=shaver. I had to remind myself of how the extra local explicit roots work, but I'm much better now!

Comment on attachment 200961 [details] [diff] [review] proposed fix sr=shaver, I say!

Comment on attachment 200961 [details] [diff] [review] proposed fix I would like to check before giving "+" that rval2 from array_extra after js_Invoke just aliases a value that is always stored in GC-scanned area.

Comment on attachment 200961 [details] [diff] [review] proposed fix I still not 100% about that rval2 in array_extra - no time to check, but if there is something, it would go to another bug (if ever ;)

rval2 is always sp[-1], yes, and only "leaked" from that loop via the SET_PROPERTY in the MAP case. Safe as can be!

This should ridealong. /be

Fixed on trunk, riding along for rc2. /be

moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.

Hrm, this wasn't checked in on the trunk (I suck). I'll let it bake overnight then hit the 1.8 branch if all looks well (as expected). /be

Forgot to mark this fixed1.8, but the patch landed here: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=mozilla%2Fjs%2Fsrc&file=jsarray.c&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=week&mindate=&maxdate=+&cvsroot=%2Fcvsroot /be

clearing the nomination flag now that this is checked in on the branch and this isn't something internal QA is going to be able to verify.

verified firefox 1.5 rc2 linux/win32 2005-11-07

testcase+ to get this off my radar. when this is made public, i will check in the test.

Fix checked into the 1.7 branch as part of the bug 311497 checkin.

verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac, note that 1.7.5/1.7.12 passed the test as well.

v moz 1.7.13 windows 20060221

RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-313763.js,v done Checking in regress-313763.js; /cvsroot/mozilla/js/tests/js1_5/GC/regress-313763.js,v <-- regress-313763.js initial revision: 1.1 done