1151333 - Crash [@ ??] with SIMD swizzle

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 421548077f12 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --ion-offthread-compile=off --ion-eager):

function assertEqX4(vec, arr, ...opts) {}
var i4 = SIMD.int32x4(1, 2, 3, 4);
for (var i = 0; i < 150; i++) {
        assertEqX4(SIMD.int32x4.swizzle(i4, i, 3, 2, 0), [1, 4, 3, 1]);
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000200000001 in ?? ()
#0  0x0000000200000001 in ?? ()
#1  0x0000000400000003 in ?? ()
#2  0x00007ffff4d5e12a in ?? ()
#3  0x0000000000000000 in ?? ()
rax	0x7fff00000000	140733193388032
rbx	0xfffc7ffff4d7ac40	-985162605679552
rcx	0xfffa00000000000d	-1688849860263923
rdx	0x0	0
rsi	0x40000000	1073741824
rdi	0x7fffffffc5c0	140737488340416
rbp	0x7fffffffd0d0	140737488343248
rsp	0x7fffffffd188	140737488343432
r8	0x7ffff698d108	140737330598152
r9	0x0	0
r10	0x2	2
r11	0x7ffff6958498	140737330381976
r12	0x8	8
r13	0x7fffffffd5a0	140737488344480
r14	0x7ffff4d7ac40	140737301163072
r15	0x7ffff7fe8640	140737354040896
rip	0x200000001	8589934593
=> 0x200000001:	


Marking sec-critical due to invalid jump.

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150320081439" and the hash "5dcff2ab60e6".
The "bad" changeset has the timestamp "20150320082235" and the hash "6abbe0f83479".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5dcff2ab60e6&tochange=6abbe0f83479

Comment 2

[Christian Holler (:decoder)]

Needinfo from Benjamin based on comment 1.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: General shuffle should reserve space for numVectors + 1 vectors

Duh, that's really weird we didn't catch it before, because it also affects correctness. Sorry for this silly issue, it probably appeared during the generalization of SimdGeneralShuffle...

So we were overwriting stack values, before this patch, which would lead to very undefined behavior. The only way I can think of, to avoid this in the future is, if that's doable, to have a RAII class that would mprotect the stack above the masm.framePushed() value at construct, and un-mprotect it at exit. Probably overkill for just this operation, but could be useful for other nodes...

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

This is related to the SIMD global object, which is enabled only on nightlies, so other versions aren't affected.

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8589117 [details] [diff] [review]
General shuffle should reserve space for numVectors + 1 vectors

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

::: js/src/jit-test/tests/SIMD/swizzle.js
@@ +60,5 @@
>      }
>  }
>  
> +function testInt32x4SwizzleBailout() {
> +    // Fuzz bug 1151333. Wrap function call in try/catch.

Instead of referencing a non-public bug here, I think we can just describe what this test is testing. "Test out-of-bounds non-constant indices. These are expected to throw."

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +2395,5 @@
>  
>      // This won't generate fast code, but it's fine because we expect users
>      // to have used constant indices (and thus MSimdGeneralShuffle to be fold
>      // into MSimdSwizzle/MSimdShuffle, which are fast).
> +    masm.reserveStack(Simd128DataSize * (numVectors + 1));

"Simd128DataSize * (numVectors + 1)" is an interesting enough expression that, with 4 uses (1 here and 3 below), should get its own variable so that we can give it a name and a comment like "we need space to store the input vectors, plus 1 to compute the result in".

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you for the review. I've used a temporary variable for Simd128DataSize*(numVectors+1) and added a comment about it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd0cd1e992a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4cd0cd1e992a

Comment 8

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.