Last Comment Bug 345967 - Yet another unrooted atom in jsarray.c
: Yet another unrooted atom in jsarray.c
Status: VERIFIED FIXED
[sg:critical]
: verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-26 04:26 PDT by Igor Bukanov
Modified: 2006-10-10 03:48 PDT (History)
3 users (show)
mconnor: blocking1.8.1+
dveditz: blocking1.8.0.7+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (1.02 KB, patch)
2006-07-26 04:30 PDT, Igor Bukanov
no flags Details | Diff | Review
Fix v2 (1.02 KB, patch)
2006-07-26 04:35 PDT, Igor Bukanov
mrbkap: review+
brendan: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Review
js1_5/GC/regress-345967.js (3.24 KB, text/plain)
2006-07-27 01:54 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2006-07-26 04:26:17 PDT
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 2006-07-26 04:30:08 PDT
Created attachment 230727 [details] [diff] [review]
Fix

Given this bug I would *appreciate* if somebody scan jsarray.c one more time for this type of hazards.
Comment 2 Igor Bukanov 2006-07-26 04:35:18 PDT
Created attachment 230728 [details] [diff] [review]
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] 2006-07-26 13:22:04 PDT
Comment on attachment 230728 [details] [diff] [review]
Fix v2

sr=me, nominating for branches.

/be
Comment 4 Igor Bukanov 2006-07-26 14:35:15 PDT
I committed to the trunk the patch from comment 2.
Comment 5 Bob Clary [:bc:] 2006-07-27 01:54:44 PDT
Created attachment 230864 [details]
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 2006-07-27 02:05:45 PDT
(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] 2006-07-27 02:34:32 PDT
(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:] 2006-07-27 04:29:27 PDT
Filed bug 346077 on the out of memory doesn't terminate script issue.
Comment 9 Igor Bukanov 2006-07-27 12:07:38 PDT
I committed the fix to MOZILLA_1_8_BRANCH
Comment 10 Daniel Veditz [:dveditz] 2006-08-09 14:20:32 PDT
Comment on attachment 230728 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers
Comment 11 Igor Bukanov 2006-08-10 09:34:23 PDT
I committed the patch from comment 2 to MOZILLA_1_8_0_BRANCH
Comment 12 Bob Clary [:bc:] 2006-08-20 11:52:08 PDT
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:] 2006-10-10 03:48:35 PDT
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

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