1271959 - Crash [@ MaybeSimdTypeToMIRType] with bad address

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 674a55274378 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions --ion-extra-checks):

setJitCompilerOption("ion.warmup.trigger", 50);
var i4 = SIMD.Int32x4(1, 2, 3, 4);
for (var i = 0; i < 150; i++) 
    SIMD.Int32x4.swizzle(i4, 3, 2, 1, 0), [4, 3, 2, 1];



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x083b112a in MaybeSimdTypeToMIRType (mirType=<synthetic pointer>, type=<incomplete type>) at js/src/jit/MIR.h:76
#0  0x083b112a in MaybeSimdTypeToMIRType (mirType=<synthetic pointer>, type=<incomplete type>) at js/src/jit/MIR.h:76
#1  js::jit::SimdTypeToMIRType (type=<incomplete type>) at js/src/jit/MIR.h:99
#2  0x083dda0c in js::jit::IonBuilder::inlineSimdShuffle (this=this@entry=0xf7a9e158, callInfo=..., native=0x86a87a0 <js::simd_int32x4_swizzle(JSContext*, unsigned int, JS::Value*)>, type=type@entry=<incomplete type>, numVectors=1) at js/src/jit/MCallOptimize.cpp:3741
#3  0x083ed254 in js::jit::IonBuilder::inlineSimd (this=this@entry=0xf7a9e158, callInfo=..., target=target@entry=0xf656bb80, type=<incomplete type>) at js/src/jit/MCallOptimize.cpp:3227
#4  0x083f3ca1 in js::jit::IonBuilder::inlineNativeCall (this=0xf7a9e158, callInfo=..., target=0xf656bb80) at js/src/jit/MCallOptimize.cpp:218
#5  0x083734db in js::jit::IonBuilder::inlineSingleCall (this=0xf7a9e158, callInfo=..., targetArg=0xf656bb80) at js/src/jit/IonBuilder.cpp:5680
#6  0x08374bda in js::jit::IonBuilder::inlineCallsite (this=this@entry=0xf7a9e158, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:5744
#7  0x08374f5c in js::jit::IonBuilder::jsop_call (this=this@entry=0xf7a9e158, argc=5, constructing=false) at js/src/jit/IonBuilder.cpp:6683
#8  0x0836ed3d in js::jit::IonBuilder::inspectOpcode (this=this@entry=0xf7a9e158, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:1894
#9  0x0836f608 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0xf7a9e158) at js/src/jit/IonBuilder.cpp:1525
#10 0x0836fde1 in js::jit::IonBuilder::build (this=0xf7a9e158) at js/src/jit/IonBuilder.cpp:918
#11 0x0837cbbf in js::jit::IonCompile (cx=cx@entry=0xf7a72020, script=script@entry=0xf655b0d0, baselineFrame=baselineFrame@entry=0xffffbe38, osrPc=osrPc@entry=0xf63f969d "ず", constructing=constructing@entry=false, recompile=recompile@entry=false, optimizationLevel=js::jit::Normal) at js/src/jit/Ion.cpp:2175
#12 0x0837d4a3 in js::jit::Compile (cx=0xf7a72020, script=script@entry=..., osrFrame=osrFrame@entry=0xffffbe38, osrPc=osrPc@entry=0xf63f969d "ず", constructing=false, forceRecompile=false) at js/src/jit/Ion.cpp:2407
[...]
#31 main (argc=6, argv=0xffffcc74, envp=0xffffcc90) at js/src/shell/js.cpp:7465
eax	0x87ff802	142604290
ebx	0x98d4314	160252692
ecx	0x990c348	160482120
edx	0xffffb740	-18624
esi	0x5	5
edi	0x87ff802	142604290
ebp	0xffffb718	4294948632
esp	0xffffb710	4294948624
eip	0x83b112a <js::jit::SimdTypeToMIRType(js::SimdType)+26>
=> 0x83b112a <js::jit::SimdTypeToMIRType(js::SimdType)+26>:	mov    -0x15231dc(%ebx,%eax,4),%eax
   0x83b1131 <js::jit::SimdTypeToMIRType(js::SimdType)+33>:	mov    %ebx,%edx


Marking s-s because this crashes at a bad address.

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/5de8e799eae3
user:        Jakob Olesen
date:        Mon May 09 16:48:30 2016 -0700
summary:     Bug 1136226 - Add 16x8 and 8x16 MIRTypes. r=sunfish

This iteration took 214.164 seconds to run.

Comment 2

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

Jakob, is bug 1136226 a likely regressor?

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

It seems likely that the crash was caused by that commit since it changes the MaybeSimdTypeToMIRType() function which crashes.

The disassembly at the crash site looks bonkers. Neither SimdTypeToMIRType() nor its callee MaybeSimdTypeToMIRType() load from memory, and certainly don't require such a crazy addressing mode.

The js::SimdType is an 'enum class X : uint8_t'. I seem to remember issues with GCC and enums with declared sizes.

This does not reproduce on OS X with clang. I am trying to reproduce on Linux.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Does not reproduce on linux with GCC-4.9, but it does reproduce with GCC-4.8.

The 'mov -0x15231dc(%ebx,%eax,4),%eax' instruction is loading an address from a jump table for the switch(type). It has the SimdType in %eax, but the high bits have not been cleared because of a compiler bug.

The range check before the jump table is only looking at %al which is 2 and in range:

   0x083a3e77 <+7>:	call   0x80ed530 <__x86.get_pc_thunk.bx>
   0x083a3e7c <+12>:	add    $0xac7490,%ebx
   0x083a3e82 <+18>:	cmp    $0xa,%al
   0x083a3e84 <+20>:	ja     0x83a3f08 <js::jit::SimdTypeToMIRType(js::SimdType)+152>
=> 0x083a3e8a <+26>:	mov    -0x25332c(%ebx,%eax,4),%eax

This compiler bug can be worked around by removing the ': uint8_t' size specifier.

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Don't specify a size for enum class SimdType.

There is a bug in GCC 4.8 that miscompiles some uses of an enum with uint8_t
size. Work around this compiler bug by not specifying a size for SimdType.

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

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

Comment 7

[Jan de Mooij [:jandem]]

The enum GCC bug was fixed in 4.8.5, are you both using an older 4.8 release? Maybe we should try bumping our minimum supported GCC from 4.8 to 4.8.5...

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64037

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1175546#c16 says we already use 4.8.5 in automation, fwiw.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8751577 [details] [diff] [review]
Don't specify a size for enum class SimdType.

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

Thanks for fixing so quickly.

::: js/src/jit/Recover.cpp
@@ +1334,5 @@
>  MSimdBox::writeRecoverData(CompactBufferWriter& writer) const
>  {
>      MOZ_ASSERT(canRecoverOnBailout());
>      writer.writeUnsigned(uint32_t(RInstruction::Recover_SimdBox));
> +    static_assert(unsigned(SimdType::Count) < 0x100, "assuming uint8 storage class for SimdType");

Can you update the comment here, please?

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> The enum GCC bug was fixed in 4.8.5, are you both using an older 4.8
> release? Maybe we should try bumping our minimum supported GCC from 4.8 to
> 4.8.5...
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64037

I was testing with gcc version 4.8.4 (Debian 4.8.4-1) which is what I got on my Debian install with the gcc-4.8 package.

I think it would be a good idea to bump our minimum GCC version to 4.8.5 since this isn't the only enum in our sources with a specified size.

I am still going to land this fix since the size of SimdType is not critical.

Comment 11

[Jakob Stoklund Olesen [:jolesen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76dab36ffb46f81ac6c0d93a20b91e2da4ee03a4
Bug 1271959 - Don't specify a size for enum class SimdType. r=bbouvier

Comment 12

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 5a2deb5a9b09).

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76dab36ffb46

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.