Last Comment Bug 313763 - Extra rootless creatures in jsarray.c
: Extra rootless creatures in jsarray.c
: js1.6, verified1.7.13, verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
P1 critical (vote)
: mozilla1.8rc1
Assigned To: Brendan Eich [:brendan]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: sbb?
  Show dependency treegraph
Reported: 2005-10-25 08:58 PDT by Igor Bukanov
Modified: 2006-06-14 17:09 PDT (History)
4 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

js1_5/Regress/regress-313763.js (2.67 KB, text/plain)
2005-10-26 01:08 PDT, Bob Clary [:bc:]
no flags Details
proposed fix (16.50 KB, patch)
2005-10-26 22:50 PDT, Brendan Eich [:brendan]
igor: review+
shaver: superreview+
asa: approval1.8rc2+
Details | Diff | Splinter Review

Description User image Igor Bukanov 2005-10-25 08:58:44 PDT
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 {
} catch (e) {
		throw e;

var expected = "1";
var actual = array[N];
print(expected === actual);
Comment 1 User image Igor Bukanov 2005-10-25 09:08:25 PDT
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.
Comment 2 User image Brendan Eich [:brendan] 2005-10-25 09:13:29 PDT
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.

Comment 3 User image Igor Bukanov 2005-10-25 09:22:41 PDT
(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?

Comment 4 User image Brendan Eich [:brendan] 2005-10-25 09:34:50 PDT
(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.

Comment 5 User image Daniel Veditz [:dveditz] 2005-10-25 12:12:36 PDT
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.
Comment 6 User image Bob Clary [:bc:] 2005-10-26 01:08:48 PDT
Created attachment 200844 [details]
Comment 7 User image Brendan Eich [:brendan] 2005-10-26 22:50:28 PDT
Created attachment 200961 [details] [diff] [review]
proposed fix

Straightforward explicit local rooting, yay.

Comment 8 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-27 01:42:40 PDT
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 9 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-27 01:42:54 PDT
Comment on attachment 200961 [details] [diff] [review]
proposed fix

sr=shaver, I say!
Comment 10 User image Igor Bukanov 2005-10-27 05:30:12 PDT
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 11 User image Igor Bukanov 2005-10-28 03:26:13 PDT
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 ;)
Comment 12 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-28 15:47:54 PDT
rval2 is always sp[-1], yes, and only "leaked" from that loop via the SET_PROPERTY in the MAP case.  Safe as can be!
Comment 13 User image Brendan Eich [:brendan] 2005-10-29 09:32:33 PDT
This should ridealong.

Comment 14 User image Brendan Eich [:brendan] 2005-10-30 21:35:56 PST
Fixed on trunk, riding along for rc2.

Comment 15 User image Asa Dotzler [:asa] 2005-10-31 14:46:48 PST
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
Comment 16 User image Brendan Eich [:brendan] 2005-10-31 17:16:01 PST
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).

Comment 18 User image Scott MacGregor 2005-11-04 17:59:01 PST
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. 
Comment 19 User image Bob Clary [:bc:] 2005-11-07 22:31:30 PST
verified firefox 1.5 rc2 linux/win32 2005-11-07
Comment 20 User image Bob Clary [:bc:] 2005-12-27 10:36:20 PST
testcase+ to get this off my radar. when this is made public, i will check in
the test.
Comment 21 User image Blake Kaplan (:mrbkap) 2006-02-14 16:35:46 PST
Fix checked into the 1.7 branch as part of the bug 311497 checkin.
Comment 22 User image Bob Clary [:bc:] 2006-02-17 04:02:22 PST
verified firefox/1.7.13,,1.8,1.9a1 win/linux/mac, note that 1.7.5/1.7.12 passed the test as well.
Comment 23 User image Bob Clary [:bc:] 2006-02-22 03:53:25 PST
v moz 1.7.13 windows 20060221
Comment 24 User image Bob Clary [:bc:] 2006-06-14 17:09:01 PDT
RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-313763.js,v
Checking in regress-313763.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-313763.js,v  <--  regress-313763.js
initial revision: 1.1

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