The default bug view has changed. See this FAQ.

Yet another unrooted atom in jsarray.c

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.7, verified1.8.1})

Trunk
verified1.8.0.7, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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");
(Assignee)

Comment 1

11 years ago
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.
Attachment #230727 - Flags: superreview?(brendan)
Attachment #230727 - Flags: review?(mrbkap)
(Assignee)

Comment 2

11 years ago
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))
Attachment #230727 - Attachment is obsolete: true
Attachment #230728 - Flags: superreview?(brendan)
Attachment #230728 - Flags: review?(mrbkap)
Attachment #230727 - Flags: superreview?(brendan)
Attachment #230727 - Flags: review?(mrbkap)

Updated

11 years ago
Attachment #230728 - Flags: review?(mrbkap) → review+
Whiteboard: [sg:critical]
Flags: blocking1.8.1?
Flags: blocking1.8.0.6+

Updated

11 years ago
Status: NEW → ASSIGNED
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 230728 [details] [diff] [review]
Fix v2

sr=me, nominating for branches.

/be
Attachment #230728 - Flags: superreview?(brendan)
Attachment #230728 - Flags: superreview+
Attachment #230728 - Flags: approval1.8.1?
Attachment #230728 - Flags: approval1.8.0.6?
(Assignee)

Comment 4

11 years ago
I committed to the trunk the patch from comment 2.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 5

11 years ago
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?

Updated

11 years ago
Flags: in-testsuite+
(Assignee)

Comment 6

11 years ago
(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.
(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

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

Updated

11 years ago
Attachment #230728 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 9

11 years ago
I committed the fix to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Comment on attachment 230728 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #230728 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 11

11 years ago
I committed the patch from comment 2 to MOZILLA_1_8_0_BRANCH
Keywords: fixed1.8.0.7

Comment 12

11 years ago
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
Group: security

Comment 13

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