1216099 - Crash [@ ??] with asm.js and SIMD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision d1a89632277f (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --enable-debug, run with --fuzzing-safe --no-threads):

function asmCompile() {
    try {
        Function.apply(null, arguments);
    } catch (e) { }
}
function asmLink(f) {}
var code = `
    "use asm";
    var f4 = global.SIMD.Float32x4;
    var f4sub = f4.sub;
    const zerox4 = f4(0.0,0.0,0.0,0.0);
    function declareHeapSize() {
        var newVelx4 = f4(0.0,0.0,0.0,0.0);
        var newVelTruex4 = f4(0.0,0.0,0.0,0.0);
        newVelTruex4 = f4sub(zerox4, newVelx4);
    }
`;
var fbirds = asmLink(asmCompile('global', 'imp', 'buffer', code), this, ffi, buffer);



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x208976d0 in ?? ()
#0  0x208976d0 in ?? ()
#1  0x09889ff4 in ?? ()
#2  0x081a68d1 in EmitBinarySimdGuts<js::jit::MSimdBinaryArith::Operation> (f=..., type=type@entry=Float32x4, op=js::jit::MSimdBinaryArith::Op_sub, def=def@entry=0xffffa1bc) at js/src/asmjs/AsmJSCompile.cpp:1824
#3  0x0814e68e in EmitSimdBinaryArith (f=..., type=type@entry=Float32x4, def=def@entry=0xffffa1bc) at js/src/asmjs/AsmJSCompile.cpp:1837
#4  0x0814fb4f in EmitF32X4Expr (f=..., def=0xffffa1bc) at js/src/asmjs/AsmJSCompile.cpp:3054
#5  0x0814dde1 in EmitSetLoc (f=..., type=type@entry=Float32x4, def=def@entry=0xffffa22c) at js/src/asmjs/AsmJSCompile.cpp:1524
#6  0x0814fc57 in EmitF32X4Expr (f=..., def=def@entry=0xffffa22c) at js/src/asmjs/AsmJSCompile.cpp:3032
#7  0x0814bac5 in EmitStatement (f=..., stmt=stmt@entry=js::wasm::F32X4Expr, maybeLabels=0x0) at js/src/asmjs/AsmJSCompile.cpp:2655
#8  0x08150b05 in EmitStatement (stmt=js::wasm::F32X4Expr, f=..., maybeLabels=<optimized out>) at js/src/asmjs/AsmJSValidate.cpp:8274
#9  EmitStatement (f=..., maybeLabels=<optimized out>) at js/src/asmjs/AsmJSCompile.cpp:2674
#10 js::GenerateAsmFunctionMIR (m=..., lifo=..., func=..., mir=mir@entry=0xffffa4b8) at js/src/asmjs/AsmJSCompile.cpp:3096
#11 0x081750cd in CheckFunctionsSequential (m=..., compileResults=compileResults@entry=0xffffa618) at js/src/asmjs/AsmJSValidate.cpp:6506
#12 0x08175a17 in CheckFunctions (m=..., results=results@entry=0xffffa618) at js/src/asmjs/AsmJSValidate.cpp:6773
#13 0x08164f2e in CheckModule (cx=cx@entry=0xf7a74020, parser=..., stmtList=stmtList@entry=0xf7a80130, moduleOut=moduleOut@entry=0xffffa8c0, compilationTimeReport=compilationTimeReport@entry=0xffffa8b8) at js/src/asmjs/AsmJSValidate.cpp:8144
#14 0x081658a3 in js::ValidateAsmJS (cx=0xf7a74020, parser=..., stmtList=stmtList@entry=0xf7a80130, validated=validated@entry=0xffffa91c) at js/src/asmjs/AsmJSValidate.cpp:8238
#15 0x080bb5c5 in js::frontend::Parser<js::frontend::FullParseHandler>::asmJS (this=0xffffb1e4, list=0xf7a80130) at js/src/frontend/Parser.cpp:3094
#16 0x080d1ec7 in js::frontend::Parser<js::frontend::FullParseHandler>::maybeParseDirective (this=this@entry=0xffffb1e4, list=list@entry=0xf7a80130, pn=pn@entry=0xf7a80180, cont=cont@entry=0xffffa97c) at js/src/frontend/Parser.cpp:3169
#17 0x080f0f90 in js::frontend::Parser<js::frontend::FullParseHandler>::statements (this=this@entry=0xffffb1e4, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:3235
#18 0x080f1c7f in js::frontend::Parser<js::frontend::FullParseHandler>::functionBody (this=this@entry=0xffffb1e4, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, kind=kind@entry=js::frontend::Statement, type=type@entry=js::frontend::Parser<js::frontend::FullParseHandler>::StatementListBody) at js/src/frontend/Parser.cpp:1227
#19 0x080c61dc in js::frontend::Parser<js::frontend::FullParseHandler>::standaloneFunctionBody (this=this@entry=0xffffb1e4, fun=..., formals=formals@entry=..., generatorKind=generatorKind@entry=js::NotGenerator, inheritedDirectives=..., newDirectives=newDirectives@entry=0xffffaccc, enclosingStaticScope=enclosingStaticScope@entry=...) at js/src/frontend/Parser.cpp:1057
#20 0x0868291b in BytecodeCompiler::compileFunctionBody (this=this@entry=0xffffad18, fun=..., formals=..., generatorKind=generatorKind@entry=js::NotGenerator) at js/src/frontend/BytecodeCompiler.cpp:635
#21 0x08682bb1 in CompileFunctionBody (cx=<optimized out>, fun=..., options=..., formals=..., srcBuf=..., enclosingStaticScope=..., generatorKind=js::NotGenerator) at js/src/frontend/BytecodeCompiler.cpp:849
#22 0x08682d85 in js::frontend::CompileFunctionBody (cx=0xf7a74020, fun=fun@entry=..., options=..., formals=formals@entry=..., srcBuf=...) at js/src/frontend/BytecodeCompiler.cpp:868
#23 0x08530699 in FunctionConstructor (cx=0xf7a74020, argc=4294949340, vp=0xffffc0ac, generatorKind=js::NotGenerator) at js/src/jsfun.cpp:1939
#24 0x08665ddc in js::CallJSNative (cx=0xf7a74020, native=0x8530890 <js::Function(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#25 0x0865df97 in js::Invoke (cx=cx@entry=0xf7a74020, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:784
#26 0x085183bb in js::fun_apply (cx=0xf7a74020, argc=2, vp=0xf64e5100) at js/src/jsfun.cpp:1263
#27 0x08665ddc in js::CallJSNative (cx=0xf7a74020, native=0x8518140 <js::fun_apply(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#28 0x0865df97 in js::Invoke (cx=0xf7a74020, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:784
#29 0x08650320 in Interpret (cx=0xf7a74020, state=...) at js/src/vm/Interpreter.cpp:3105
#30 0x0865d360 in js::RunScript (cx=cx@entry=0xf7a74020, state=...) at js/src/vm/Interpreter.cpp:725
#31 0x0865d61f in js::ExecuteKernel (cx=cx@entry=0xf7a74020, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=js::EXECUTE_GLOBAL, evalInFrame=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:1000
#32 0x0865dcbc in js::Execute (cx=cx@entry=0xf7a74020, script=..., scopeChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:1035
#33 0x084a8de6 in ExecuteScript (cx=cx@entry=0xf7a74020, scope=scope@entry=..., script=..., rval=rval@entry=0x0) at js/src/jsapi.cpp:4598
#34 0x084a8f3c in JS_ExecuteScript (cx=cx@entry=0xf7a74020, scriptArg=scriptArg@entry=...) at js/src/jsapi.cpp:4631
#35 0x080807c5 in RunFile (compileOnly=false, file=0xf632b9e0, filename=0xffffd1b5 "min.js", cx=0xf7a74020) at js/src/shell/js.cpp:509
#36 Process (cx=cx@entry=0xf7a74020, filename=0xffffd1b5 "min.js", forceTTY=false, forceTTY@entry=true) at js/src/shell/js.cpp:628
#37 0x0808e72e in ProcessArgs (op=0xffffce6c, cx=0xf7a74020) at js/src/shell/js.cpp:5994
#38 Shell (op=0xffffce6c, cx=0xf7a74020, envp=<optimized out>) at js/src/shell/js.cpp:6297
#39 main (argc=4, argv=0xffffcfe4, envp=0xffffcff8) at js/src/shell/js.cpp:6654
eax	0xffffa27c	-23940
ebx	0x9889ff4	159948788
ecx	0xffffa204	-24060
edx	0xffffa118	-24296
esi	0x208976d0	545879760
edi	0xffffa204	-24060
ebp	0xffffa0e8	4294942952
esp	0xffffa0e0	4294942944
eip	0x208976d0	545879760
=> 0x208976d0:	



This bug only reproduces for me on the builds made on an Ubuntu 12.04 LTS machine with GCC 4.7.3. I was not able to reproduce it on my regular builds which use a newer GCC. This might be coincidence (newer GCC compiles code such that it is less likely to crash in this case) or some miscompilation happening. Marking s-s and sec-critical based on crash trace, showing an invalid jump to random memory.

Comment 1

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

[Christian Holler (:decoder)]

Needinfo from :bbouvier based on IRC conversation.

Comment 3

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]:

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as it's sec-crit.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Couldn't reproduce with either gcc-4.7/g++-4.7 compiling the 32 bits shell with the same configure parameters, nor the 64 bits version.

It really looks like a compiler bug, as newer versions of gcc/g++ don't reproduce as well. My guesses are either that one of the switches in EmitExpr (inlined in EmitBinarySimdGuts) or in MIRTypeFromAsmType gets wrong, or the template call to binarySimd gets messed up.

If I could have access to a machine on which the bug reproduces, I could allocate some time to get more insight, if that's useful. I even wonder if we are still using gcc-4.7 on our CI platforms, just asked on #developers. If we're not, maybe we could just close this bug as worksforme/wontfix.

decoder, any way to get a machine and/or any way to know whether we're still using gcc-4.7 in production?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

From IRC:

15:14 	<RyanVM>	yes, for linux IIRC
15:14 	<RyanVM>	or was it android
15:15 	<RyanVM>	oh hey, we're using 4.9 on android now it appears
15:17 	<RyanVM>	bbouvier: so I *think* the answer is actually no now
15:17 	<RyanVM>	pretty sure our linux builds are 4.8, and android/b2g are 4.9
15:18 	<RyanVM>	b2g is still on 4.8
15:18 	<RyanVM>	well ICS is, JB+ are on 4.


So I suggest closing as wontfix.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

After some digging, it seems that gcc4.7.3 is actually still used for linux32. However, this is tested on treeherder and it doesn't seem to be an issue even there. Not sure what we should do here.

Comment 8

[Christian Holler (:decoder)]

What is tested on treeherder, the test in comment 0? If we still build something with GCC 4.7.x that we actually ship, then we need to fix this bug. If that isn't the case, then we can close this as wontfix.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Let's try-run this very specific test case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13052584ecd8

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: test.patch

I don't think there is any actual issue here. The try run shows that the test passed as expected on linux32 (without crashing), which is a platform that uses gcc version 4.7. This might be caused by the combination of --enable-debug / --enable-optimize, which is not tested on treeherder.

This code path is already very well tested. Let's be paranoid and make sure this doesn't happen in practice by landing the same test case. Unless I can get my hands on the same exact device that triggered the build issue, there is not much more we can do here.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8688434 [details] [diff] [review]
test.patch

General context: this patch contains only a single test, that I couldn't reproduce on any machine (developer machine or treeherder). We want to land this test just to make sure there isn't a (probable) compiler error when running this test case.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
You'd have to use gcc4.7 and have the exact same compilation conditions as on the original machine. Testing locally and on tbpl, this doesn't crash. Plus, comment 0 seems to show a crash at a random address. So it seems very unlikely that it can be crafted into an exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
No idea. Guessing bug 1157624 introduced the last modifications on the affected code, but this code was already present beforehand and should have triggered way before, if it were dangerous.

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No risks, it's just a test.

How likely is this patch to cause regressions; how much testing does it need?
No risks, it's just a test.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8688434 [details] [diff] [review]
test.patch

sec-approval+

Comment 13

[Daniel Veditz [:dveditz]]

If we think this does not happen in any release build then it's not sec-crit.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Another try run was all green.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a5885fca6ae

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a5885fca6ae

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Benjamin, do we need to uplift this to Aurora44?

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

This is a patch containing only a test, so I'd say no.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> This is a patch containing only a test, so I'd say no.

Interestingly, I don't think this was a code issue at all. Unless I am mistaken about this one, I will set status-firefox44 to unaffected.