Extra rootless creatures in jsarray.c

VERIFIED FIXED in mozilla1.8rc1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: brendan)

Tracking

({js1.6, verified1.7.13, verified1.8})

Trunk
mozilla1.8rc1
x86
Linux
js1.6, verified1.7.13, verified1.8
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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);
(Reporter)

Comment 1

12 years ago
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.
Severity: normal → critical
(Assignee)

Comment 2

12 years ago
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
Assignee: general → brendan
Flags: blocking1.8rc1+
Keywords: js1.6
Priority: -- → P1
Target Milestone: --- → mozilla1.8rc1
(Reporter)

Comment 3

12 years ago
(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?

Priority: P1 → --
Target Milestone: mozilla1.8rc1 → ---
(Assignee)

Comment 4

12 years ago
(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
Status: NEW → ASSIGNED
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.
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Whiteboard: [sg:critical?]

Comment 6

12 years ago
Created attachment 200844 [details]
js1_5/Regress/regress-313763.js

Updated

12 years ago
Flags: testcase?

Updated

12 years ago
Flags: blocking1.8rc1+ → blocking1.8rc1?
(Assignee)

Comment 7

12 years ago
Created attachment 200961 [details] [diff] [review]
proposed fix

Straightforward explicit local rooting, yay.

/be
Attachment #200961 - Flags: superreview?(shaver)
Attachment #200961 - Flags: review?(igor.bukanov)
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!
Attachment #200961 - Flags: superreview?(shaver) → superreview+
(Reporter)

Comment 10

12 years ago
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.
(Reporter)

Comment 11

12 years ago
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 ;)
Attachment #200961 - Flags: review?(igor.bukanov) → review+
rval2 is always sp[-1], yes, and only "leaked" from that loop via the SET_PROPERTY in the MAP case.  Safe as can be!
(Assignee)

Comment 13

12 years ago
This should ridealong.

/be
Priority: -- → P1
Whiteboard: [sg:critical?] → [sg:critical]
Target Milestone: --- → mozilla1.8rc1
(Assignee)

Updated

12 years ago
Attachment #200961 - Flags: approval1.8rc1?
(Assignee)

Comment 14

12 years ago
Fixed on trunk, riding along for rc2.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.8rc2?
Resolution: --- → FIXED

Comment 15

12 years ago
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.
Flags: blocking1.8rc1?

Updated

12 years ago
Attachment #200961 - Flags: approval1.8rc1? → approval1.8rc2+
(Assignee)

Comment 16

12 years ago
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
Blocks: 256195
(Assignee)

Comment 17

12 years ago
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
Keywords: fixed1.8

Comment 18

12 years ago
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. 
Flags: blocking1.8rc2?

Comment 19

12 years ago
verified firefox 1.5 rc2 linux/win32 2005-11-07
Keywords: fixed1.8 → verified1.8
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+

Comment 20

12 years ago
testcase+ to get this off my radar. when this is made public, i will check in
the test.
Flags: testcase? → testcase+
Fix checked into the 1.7 branch as part of the bug 311497 checkin.
Keywords: fixed-aviary1.0.8, fixed1.7.13

Comment 22

11 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8

Comment 23

11 years ago
v moz 1.7.13 windows 20060221
Keywords: fixed1.7.13 → verified1.7.13
Group: security

Comment 24

11 years ago
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
You need to log in before you can comment on or make changes to this bug.