Last Comment Bug 313763 - Extra rootless creatures in jsarray.c
: Extra rootless creatures in jsarray.c
Status: VERIFIED FIXED
[sg:critical]
: 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]
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 | Review

Description 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 {
	array.unshift(1);
} catch (e) {
	if (e !== LOOP_TERMINATOR)
		throw e;
}

var expected = "1";
var actual = array[N];
print(expected === actual);
Comment 1 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 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.

/be
Comment 3 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 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.

/be
Comment 5 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 Bob Clary [:bc:] 2005-10-26 01:08:48 PDT
Created attachment 200844 [details]
js1_5/Regress/regress-313763.js
Comment 7 Brendan Eich [:brendan] 2005-10-26 22:50:28 PDT
Created attachment 200961 [details] [diff] [review]
proposed fix

Straightforward explicit local rooting, yay.

/be
Comment 8 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 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 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 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 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 Brendan Eich [:brendan] 2005-10-29 09:32:33 PDT
This should ridealong.

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

/be
Comment 15 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 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).

/be
Comment 18 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 Bob Clary [:bc:] 2005-11-07 22:31:30 PST
verified firefox 1.5 rc2 linux/win32 2005-11-07
Comment 20 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 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-02-14 16:35:46 PST
Fix checked into the 1.7 branch as part of the bug 311497 checkin.
Comment 22 Bob Clary [:bc:] 2006-02-17 04:02:22 PST
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.
Comment 23 Bob Clary [:bc:] 2006-02-22 03:53:25 PST
v moz 1.7.13 windows 20060221
Comment 24 Bob Clary [:bc:] 2006-06-14 17:09:01 PDT
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

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