345967 - Yet another unrooted atom in jsarray.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The fix for bug 341956 missed the hazard in array_shift. There id for the last element is accessed after changing the element before that. So a setter there can trigger GC which would collect the last id.

Fortunately this is only exploitable on 64bit computer with tens of gigabyte of memory due to bug 345961. Due to that bug a sparse array would be turned into dense one during shift loop. Thus the loop can only start to produce atoms when the array would have  at least JSVAL_INT_MAX + 2 or 2^30 + 1 elements. That is not possible to allocate with 32-bit CPU and on 64-bit one just the slot array would take 8GB of memory plus the slot map hashtable overhead.

But if one fixes bug 345961 or if one can find a host object where a setter by default for indexes would do nothing but where one can set object.length to any value, then something like the following can expose the bug after a few minutes:

var JSVAL_INT_MAX = (1 << 30) - 1;

var a = new Array(JSVAL_INT_MAX + 2);
a[JSVAL_INT_MAX] = 0;
a[JSVAL_INT_MAX + 1] = 1;

a.__defineGetter__(JSVAL_INT_MAX, function() { return 0; });

a.__defineSetter__(JSVAL_INT_MAX, function(value) {
	delete a[JSVAL_INT_MAX + 1];
	var tmp = [];
	tmp[JSVAL_INT_MAX + 2] = 2;

        if (typeof gc == 'function')
                gc();
        for (var i = 0; i != 50000; ++i) {
                var tmp = 1 / 3;
                tmp /= 10;
        }
        for (var i = 0; i != 1000; ++i) {
                // Make string with 11 characters that would take
                // (11 + 1) * 2 bytes or sizeof(JSAtom) so eventually
                // malloc will ovewrite just freed atoms. 
                var tmp2 = Array(12).join(' ');
	}
});


a.shift();

var expected = 0;
var actual = a[JSVAL_INT_MAX];
if (expected !== actual)
   print("BAD");

Comment 1

[Igor Bukanov]

Attachment: Fix

Given this bug I would *appreciate* if somebody scan jsarray.c one more time for this type of hazards.

Comment 2

[Igor Bukanov]

Attachment: Fix v2

This version uses
  if (length > JSVAL_INT_MAX && !IndexToId(cx, length, &id))
and not equivalent but bug-looking 
  if (i > JSVAL_INT_MAX && !IndexToId(cx, length, &id))

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 230728 [details] [diff] [review]
Fix v2

sr=me, nominating for branches.

/be

Comment 4

[Igor Bukanov]

I committed to the trunk the patch from comment 2.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-345967.js

On a 32bit machine, the browser test has repeated out of memory errors on the shift(). Shouldn't the memory error cause the script to terminate?

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> On a 32bit machine, the browser test has repeated out of memory errors on the
> shift(). Shouldn't the memory error cause the script to terminate?

With the test case this should always happen on 32-bit computer due to bug 345961.  On 64-bit one you need 24GB(!) of RAM to finish the test. So this is why the bug is mostly theoretical unless the bug 345961 is fixed.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (In reply to comment #5)
> > On a 32bit machine, the browser test has repeated out of memory errors on the
> > shift(). Shouldn't the memory error cause the script to terminate?
> 
> With the test case this should always happen on 32-bit computer due to bug
> 345961.  On 64-bit one you need 24GB(!) of RAM to finish the test. So this is
> why the bug is mostly theoretical unless the bug 345961 is fixed.

But why isn't OOM stopping the script?  I don't see a failure to propagate false/null along the way.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

Filed bug 346077 on the out of memory doesn't terminate script issue.

Comment 9

[Igor Bukanov]

I committed the fix to MOZILLA_1_8_BRANCH

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 230728 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

[Igor Bukanov]

I committed the patch from comment 2 to MOZILLA_1_8_0_BRANCH

Comment 12

[Bob Clary [:bc] (inactive)]

Apart from a SIGABRT on macppc opt browser due to out of memory, I see no crashes. Marking verified fixed 1.8.0.7, 1.8, 1.9 20060818 windows/mac*/linux

Comment 13

[Bob Clary [:bc] (inactive)]

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