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

VERIFIED FIXED in Firefox 38, Firefox OS master

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: decoder, Assigned: bbouvier)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla38
x86
Linux
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 unaffected, firefox38 verified, firefox39 verified, firefox-esr31 unaffected, b2g-master fixed)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
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.
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 2

3 years ago
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.
Blocks: 1127929
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.
Flags: needinfo?(nicolas.b.pierron) → needinfo?(benj)
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
Created attachment 8563406 [details] [diff] [review]
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?
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(benj)
Attachment #8563406 - Flags: review?(sunfish)
(Assignee)

Comment 6

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c20f5895ec11
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).
Attachment #8563406 - Flags: review?(sunfish) → review+
(Assignee)

Comment 8

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68b192d02d94
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3341a0bc3296
Blocks: 1131811
https://hg.mozilla.org/mozilla-central/rev/3341a0bc3296
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
status-firefox38: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Follow-up patch to fix test on branches which lack SIMD:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e529daa34f7
Merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/3e529daa34f7
(Reporter)

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: --- → verified
(Reporter)

Comment 13

3 years ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx38
status-firefox37: --- → unaffected
status-firefox-esr31: --- → unaffected

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.