1346547 - Assertion failure: isInt32(), at dist/include/js/Value.h:605

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 4ceb9062ea8f (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads --ion-eager):

var lfLogBuffer = `
function test() {
    for (var i=0; i<20; i++) {
	var arr = [1];
	if (i > 5)
	  Object.defineProperty(arr, "length", { writable: true, value: -1 && 0x7fffffff });
	arr.push(2);
    }
}
test();
`;
var lfCodeBuffer = lfLogBuffer;
Object.defineProperty(this, "fuzzutils", { });
if (lfCodeBuffer) loadFile(lfCodeBuffer);
function loadFile(lfVarx) {
    evaluate(lfVarx); 
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000004f9794 in JS::Value::toInt32 (this=<optimized out>) at dist/include/js/Value.h:605
#0  0x00000000004f9794 in JS::Value::toInt32 (this=<optimized out>) at dist/include/js/Value.h:605
#1  0x0000000000827f2c in js::WrappedPtrOperations<JS::Value, JS::MutableHandle<JS::Value> >::toInt32 (this=<optimized out>) at dist/include/js/Value.h:1327
#2  js::jit::ArrayPushDense (cx=<optimized out>, obj=..., v=..., length=0x7fffffffa7cc) at js/src/jit/VMFunctions.cpp:378
#3  0x000024b1a70b79f4 in ?? ()
#4  0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffa7cc	140737488332748
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffa6f0	140737488332528
rsp	0x7fffffffa6f0	140737488332528
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x1	1
r13	0x7fffffffa7f8	140737488332792
r14	0x7fffffffa720	140737488332576
r15	0x7fffffffa7f0	140737488332784
rip	0x4f9794 <JS::Value::toInt32() const+52>
=> 0x4f9794 <JS::Value::toInt32() const+52>:	movl   $0x0,0x0
   0x4f979f <JS::Value::toInt32() const+63>:	ud2    


This assert looks potentially unsafe to me and the testcase behaved very strangely (plus it involves Arrays), so I'm marking this s-s until we can be sure this is entirely safe.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/55f700adddec
user:        Boris Zbarsky
date:        Fri Mar 20 21:34:19 2015 -0400
summary:     Bug 1145491 part 7.  Stop checking compileAndGo before emitting GNAME ops.  r=luke

This iteration took 178.920 seconds to run.

Comment 2

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

This seems unlikely to be the regressing changeset, so setting needinfo? from Jan as a fallback, as JIT is on the stack.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

Great find. This is a very old bug.

In ArrayPushDense we call array_push and expect it to return an int32 value. This fails when the array has length INT32_MAX because pushing the new value will return INT32_MAX + 1 and that's stored as double.

Fortunately, TI will set the OBJECT_FLAG_LENGTH_OVERFLOW flag in this case and we will invalidate the script, so all we have to do is use AutoDetectInvalidation to override the return value on bailout.

This is not s-s, the worst that can happen is that script will see a bogus return value from array.push in some edge cases.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8847081 [details] [diff] [review]
Patch

Review of attachment 8847081 [details] [diff] [review]:
-----------------------------------------------------------------

Good find!

::: js/src/jit/VMFunctions.cpp
@@ +381,5 @@
> +        return true;
> +    }
> +
> +    // array_push changed the length to be larger than INT32_MAX. In this case
> +    // we will have set the OBJECT_FLAG_LENGTH_OVERFLOW flag, TI invalidated

... In this case OBJECT_FLAG_LENGTH_OVERFLOW was set, ...

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71cb591837f2
Fix correctness bug with ArrayPushDense and INT32_MAX array length. r=h4writer

Comment 6

[Simon Lindholm]

Should
+        *length = argv[0].isInt32();
be
+        *length = argv[0].toInt32();
?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Simon Lindholm from comment #6)
> Should
> +        *length = argv[0].isInt32();
> be
> +        *length = argv[0].toInt32();
> ?

Yes, great catch! I'm surprised we don't have test coverage for this. I'll write a patch.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Follow-up fix

Actually it's hard to write a test for this, because ArrayPushDense has a fast path that handles most cases, and I think the slow path either throws an exception (non writable array length etc) or it returns a double (the bug we fixed here), so we don't get to the isInt32 case. I think we should keep this code in case that changes in the future.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8851544 [details] [diff] [review]
Follow-up fix

Review of attachment 8851544 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about missing that in the review.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/71cb591837f2

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3afa18fd5ef5
followup - Use toInt32 instead of isInt32 in ArrayPushDense. r=h4writer

Comment 12

[Jan de Mooij [:jandem]]

Very old bug and an edge case that's unlikely to happen in the wild, should just ride the trains.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3afa18fd5ef5