313763 - Extra rootless creatures in jsarray.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

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]

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]]

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]

(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]]

(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]]

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] (inactive)]

Attachment: js1_5/Regress/regress-313763.js

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix

Straightforward explicit local rooting, yay.

/be

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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)]

Comment on attachment 200961 [details] [diff] [review]
proposed fix

sr=shaver, I say!

Comment 10

[Igor Bukanov]

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]

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)]

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]]

This should ridealong.

/be

Comment 14

[Brendan Eich [:brendan]]

Fixed on trunk, riding along for rc2.

/be

Comment 15

[Asa Dotzler [:asa]]

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]]

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 17

[Brendan Eich [:brendan]]

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

Comment 18

[Scott MacGregor]

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] (inactive)]

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

Comment 20

[Bob Clary [:bc] (inactive)]

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

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 22

[Bob Clary [:bc] (inactive)]

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] (inactive)]

v moz 1.7.13 windows 20060221

Comment 24

[Bob Clary [:bc] (inactive)]

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