1130845 - Assertion failure: from->toStackSlot()->slot() % SimdMemoryAlignment == 0, at jit/LIR.cpp:557 or Crash [@ ??] with SIMD

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision be65d1fde126 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-debug, run with --ion-offthread-compile=off --ion-eager):

var int32x4 = SIMD.int32x4;
function test() {
  var a = int32x4(  ) ;
  var b = int32x4(10, 20, 30, 40);
  var c = SIMD.int32x4.and(a, b);
  assertEq(c.x, 0);
}
test();
for (var j=0; j<u.length; ++j)
    v[test()] = t;



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0xf55f2cae in ?? ()
#0  0xf55f2cae in ?? ()
#1  0xf7fc87a6 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax	0xf6300230	-164625872
ebx	0x301	769
ecx	0xf5858660	-175798688
edx	0x9369824	154572836
esi	0xf7fd03d3	-134413357
edi	0x936fb68	154598248
ebp	0xffffd37c	4294955900
esp	0xffffd300	4294955776
eip	0xf55f2cae	4116655278
=> 0xf55f2cae:	movdqa %xmm0,0x78(%esp)
   0xf55f2cb4:	mov    $0xf5844040,%ecx


Marking s-s because the crash in the optimized build does not look safe.

Comment 1

[Nicolas B. Pierron [:nbp]]

This is probably related to the addition of SIMD into Ion.
This crash is only on nightlies, and it is safe because this is a miss-alignment issue.

(In reply to Christian Holler (:decoder) from comment #0)
> Program received signal SIGSEGV, Segmentation fault.
> => 0xf55f2cae:	movdqa %xmm0,0x78(%esp)

movdqa  expects the address to be something like  0x.....0
esp is the stack pointer.
0x78 is greater than 0.

(In reply to Christian Holler (:decoder) from comment #0)
> esp	0xffffd300	4294955776

esp + 0x78  == 0xffffd378  and does not match   0x.....0

So this is a miss-alignment issue, and this cannot be use to make any exploit.

I will investigate this issue later.

Comment 2

[Christian Holler (:decoder)]

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/ed0a31f8bdc6
user:        Victor Carlquist
date:        Thu Feb 05 15:13:14 2015 +0100
summary:     Bug 1127929 - IonMonkey: Inline SIMD.int32x4.and calls. r=nbp

This iteration took 539.840 seconds to run.

Comment 3

[Nicolas B. Pierron [:nbp]]

Benjamin would be looking at this issue,  I guess this issue is not directly related to Victor's patch but just highlighted by it as it is adding more surface to an existing issue.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

In this case, there's a SimdUnbox in the block right before the loop header, that gets spilled onto the stack because there are too many live registers, and then it gets reloaded at the end of the block.  Moves implied by regalloc assume that the stack is correctly aligned, and thus that we can do aligned moves, but that's not the case here.

Testing with nbp's folded alignment patch, it still crashes, so there might be something wrong with one of the alignment patches, or the assertion is somehow wrong? Investigating.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simd-stackslots.patch

So the issue was that on a call to spill(), regalloc was re-using another already allocated, but available stack slot (available since the corresponding definition was dead).  The issue in this particular example is that the stack slot had a size of 32 bits and no alignment requirement, while SIMD expect 128 bits and do have alignment requirements.

Odd enough, this was never hit with backtracking, while the code for allocating stack slots looks to be shared among the two.  Any idea why?

I've just copied the same logic as for the doubles slots list, and it works fine.

About the security: this is nightly-only, but this could be the source of stack corruption, I suppose (in the case where the former 32-bits slot is aligned, we could clobber up to 3 * 32 = 96 bits on the stack).  Should I ask for security rating?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c20f5895ec11

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 8563406 [details] [diff] [review]
simd-stackslots.patch

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

Looks right to me. Backtracking already knows about this (quadSlots).

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68b192d02d94

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3341a0bc3296

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3341a0bc3296

Comment 11

[Dan Gohman [:sunfish]]

Follow-up patch to fix test on branches which lack SIMD:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e529daa34f7

Comment 12

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

Merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/3e529daa34f7

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx38