563125 - "Assertion failure: m != TT_INT32 || isInt32(*vp)" with array.length

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

var array = new Array(4294967295);
for (var j = 0; j < 2; ++j) { '' + array.length; }

Assertion failure: m != TT_INT32 || isInt32(*vp), at ../jstracer.cpp:3859

Opt does not crash, but I'm filing this as security-sensitive anyway because tracer bugs scare me.

Comment 1

[Jesse Ruderman]

This assertion failure happens in both 64-bit and 32-bit builds, fwiw.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

this feels pretty lame but i don't have any better ideas.

i dont think this is exploitable since the worst that can happen is the tracer interpreters a uint32 as an int32 (leads to correctness bugs) - but andreas should confirm.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This should have been asserting ever since tracing was enabled. I assert with -j as far back as Nov 2008 - http://hg.mozilla.org/tracemonkey/rev/04c360f123e5

Comment 4

[Daniel Veditz [:dveditz]]

Unless this is exploitable it's not going to block stable branch releases.

Comment 5

[David Mandelin [:dmandelin]]

WFM in f1a334334d13.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

You need to up the post-JM loop count.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: refreshed

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Not exploitable.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 492018 [details] [diff] [review]
refreshed

>-        v_ins = w.i2d(w.lduiObjPrivate(obj_ins));
>+        v_ins = w.lduiObjPrivate(obj_ins);
>+        if (obj->getArrayLength() <= JSVAL_INT_MAX) {
>+            guard(true, w.leui(v_ins, w.immi(JSVAL_INT_MAX)), BRANCH_EXIT);
>+            v_ins = w.i2d(v_ins);
>+        } else {
>+            v_ins = w.ui2d(v_ins);
>+        }

Wouldn't this suffice?

          v_ins = w.ui2d(w.lduiObjPrivate(obj_ins));

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Then the value wouldn't be demoted since IsPromoteInt() would return false.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to comment #10)
> Then the value wouldn't be demoted since IsPromoteInt() would return false.

Which IsPromoteInt() call are you referring to?  Should it be IsPromote() or isPromoteUint() instead?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yes and no... IsPromoteInt is called in the forward pipeline to demote float math to integer math (and float stores to integer stores). It can't be IsPromoteUint because there is no TT_* for an unsigned integer, so when leaving trace the very large value would be boxed incorrectly as a signed integer.

Code that checks IsPromoteInt() but not IsPromoteUint() should probably be looking at both, but that's a separate bug. Here I just want to keep performance the same, that is (x < array.length) should use integer math not float.

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 492018 [details] [diff] [review]
refreshed

(In reply to comment #12)
> 
> Here I just want to keep performance the same, that is (x < array.length)
> should use integer math not float.

There's an extra guard, but that's probably better than using float math.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/e020c8f1099d

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e020c8f1099d

Comment 16

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug563125.js.