814552 - IonMonkey: Assertion failure: masm.framePushed() == frameSize(), at ion/CodeGenerator.cpp:342 or Crash [@ MarkInternal<js::ion::IonCode>]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase asserts on mozilla-central revision bc69705c162d (run with --ion-eager).

Comment 1

[Christian Holler (:decoder)]

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
0x40a08140 in ?? ()
(gdb) bt
#0  0x40a08140 in ?? ()
Cannot access memory at address 0xffffff82
#1  0x0015739e in MarkInternal<js::ion::IonCode> (trc=0x40a11150, thing=<value optimized out>, name=<value optimized out>) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:125
#2  Mark<js::ion::IonCode> (trc=0x40a11150, thing=<value optimized out>, name=<value optimized out>) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:151
#3  js::gc::MarkIonCode (trc=0x40a11150, thing=<value optimized out>, name=<value optimized out>) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:240
#4  0x001d0ba6 in trace (trc=0x40a11150, script=0x1c2c320) at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:537
#5  js::ion::IonScript::Trace (trc=0x40a11150, script=0x1c2c320) at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:713
#6  0x00233d84 in js::ion::IonBailoutIterator::IonBailoutIterator (this=0x40a11150, activations=<value optimized out>, bailout=0x40a11150) at /srv/repos/mozilla-central/js/src/ion/arm/Bailouts-arm.cpp:159
#7  0x00233d84 in js::ion::IonBailoutIterator::IonBailoutIterator (this=0xbee0d1c4, activations=<value optimized out>, bailout=0xbee0d290) at /srv/repos/mozilla-central/js/src/ion/arm/Bailouts-arm.cpp:159
#8  0x001ce054 in js::ion::InvalidationBailout (sp=0xbee0d290, frameSizeOut=0xbee0d288) at /srv/repos/mozilla-central/js/src/ion/Bailouts.cpp:391
#9  0x400abf4c in ?? ()
Cannot access memory at address 0xffffff82
#10 0x400abf4c in ?? ()
Cannot access memory at address 0xffffff82

Comment 2

[Nicolas B. Pierron [:nbp]]

I cannot reproduce it with a recent build of mozilla-central on Linux x64, CC marty.
This assertion is raised by the codegen. Can you provide the backtrace of the debug version?

Comment 3

[Christian Holler (:decoder)]

The bug is likely ARM only, that's why assigned Marty already. Here is the backtrace of the debug version as requested:


Program received signal SIGSEGV, Segmentation fault.
0x004f09a4 in js::ion::CodeGenerator::visitOsiPoint (this=0xd55d48, lir=0xd6b0f0) at /srv/repos/mozilla-central/js/src/ion/CodeGenerator.cpp:342
342         JS_ASSERT(masm.framePushed() == frameSize());
(gdb) bt
#0  0x004f09a4 in js::ion::CodeGenerator::visitOsiPoint (this=0xd55d48, lir=0xd6b0f0) at /srv/repos/mozilla-central/js/src/ion/CodeGenerator.cpp:342
#1  0x00445cdc in js::ion::LOsiPoint::accept (this=0xd6b0f0, visitor=0xd55d48) at ../ion/LIR-Common.h:72
#2  0x004f411e in js::ion::CodeGenerator::generateBody (this=0xd55d48) at /srv/repos/mozilla-central/js/src/ion/CodeGenerator.cpp:1472
#3  0x004f9a86 in js::ion::CodeGenerator::generate (this=0xd55d48) at /srv/repos/mozilla-central/js/src/ion/CodeGenerator.cpp:3079
#4  0x003c19c0 in js::ion::CompileBackEnd (mir=0xd5eb88) at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:957
#5  0x003c1cd4 in js::ion::SequentialCompileContext::compile (this=0xbee18ba4, builder=0xd5eb88, graph=0xd5eb20, autoDestroy=...) at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:1142
#6  0x003c625a in js::ion::IonCompile<js::ion::SequentialCompileContext> (cx=0xce4868, script=0x40a0f280, fun=0x40a17020, osrPc=0xd28ba8 "\343V", constructing=false, compileContext=...)
    at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:1104
#7  0x003c20f6 in js::ion::Compile (cx=0xce4868, script=0x40a0f280, fun=0x40a17020, osrPc=0xd28ba8 "\343V", constructing=false) at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:1281
#8  0x003c22b8 in js::ion::CanEnterAtBranch (cx=0xce4868, script=..., fp=0x403d80d8, pc=0xd28ba8 "\343V") at /srv/repos/mozilla-central/js/src/ion/Ion.cpp:1323
#9  0x0011b104 in js::Interpret (cx=0xce4868, entryFrame=0x403d8088, interpMode=js::JSINTERP_NORMAL) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:1394
#10 0x00117ab0 in js::RunScript (cx=0xce4868, script=..., fp=0x403d8088) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:326
#11 0x00118736 in js::ExecuteKernel (cx=0xce4868, script=..., scopeChain=..., thisv=..., type=js::EXECUTE_GLOBAL, evalInFrame=0x0, result=0x403d8060) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:512
#12 0x00118938 in js::Execute (cx=0xce4868, script=..., scopeChainArg=..., rval=0x403d8060) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:550
#13 0x000532bc in JS_ExecuteScript (cx=0xce4868, objArg=0x40a0b040, scriptArg=0x40a0f200, rval=0x403d8060) at /srv/repos/mozilla-central/js/src/jsapi.cpp:5537
#14 0x00011bc0 in Evaluate (cx=0xce4868, argc=1, vp=0x403d8060) at /srv/repos/mozilla-central/js/src/shell/js.cpp:927

Comment 4

[Christian Holler (:decoder)]

This needs a security rating, Marty, can you judge the severity here?

Comment 5

[Christian Holler (:decoder)]

The original test doesn't reproduce anymore, but I got a very similar looking test that still repros this (revision 8f818068da09):


var assertEq = {};
evaluate("var oa = f2(0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,\
              0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,\
              0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,\
              0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,\
              0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,\
              0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,\
              0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9);\
var ob = {};\
assertEq(ob.length, 1);\
", { noScriptRval : true });

Comment 6

[Christian Holler (:decoder)]

This keeps triggering, also in various release crashes.

Comment 7

[Daniel Veditz [:dveditz]]

Naveed: please find an assignee for this one. In addition to being a security bug in its own right it's a "fuzzblocker" that is preventing us from effectively testing Ionmonkey on ARM.

Comment 8

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/alignAllTheStacks-r0.patch

I'll leave it to dvander to dictate the break-up of the mega-line that is created here.  Due to this line being in an initializer list, I can't use temps or anything quite that fancy.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 718716 [details] [diff] [review]
/home/mjrosenb/patches/alignAllTheStacks-r0.patch

>     JS_ASSERT((framePushed() & 3) == 0);
>+    //    ma_callIonHalfPush(callee);
>     if ((framePushed() & 7) == 4) {
>+        //        fprintf(stderr, "half push\n");
>         ma_callIonHalfPush(callee);
>     } else {
>+        //fprintf(stderr, "full push\n");
>         adjustFrame(sizeof(void*));
>         ma_callIon(callee);
>     }
> }

nit: don't forget to remove these and the other commented debug statements


>-    frameDepth_(graph->localSlotCount() * sizeof(STACK_SLOT_SIZE) +
>+    frameDepth_(((graph->localSlotCount() + ComputeByteAlignment(-graph->localSlotCount(), StackAlignment / sizeof(STACK_SLOT_SIZE)))) * sizeof(STACK_SLOT_SIZE) +
>                 graph->argumentSlotCount() * sizeof(Value))
> {

You could store stuff in temporaries and assign frameDepth_ in the constructor body, if that would be clearer. 

(As discussed in IRC you can get rid of the negate.)

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking based on comment 7.

Comment 11

[Alex Keybl [:akeybl]]

Al - do you suspect this is a new or easily exploited regression in FF19 and something we should consider fixing before FF20?

Comment 12

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

Alex, I don't know. Decoder might be able to say if this is a regression or not. He did think it was ARM only.

Comment 13

[Christian Holler (:decoder)]

I don't know how easily this can be exploited, but it should be ARM only.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like this needs to be landed, verified, and nominated for uplift - would be great to get that happening prior to Tuesday when we go to build with our next beta.

Comment 15

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

Needs a sec-approval nomination first.

Comment 16

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

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #15)
> Needs a sec-approval nomination first.

Marty, can you do that and fill out the template that comes up for the patch?

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 718716 [details] [diff] [review]
/home/mjrosenb/patches/alignAllTheStacks-r0.patch

We talked in person last week, posting here for posterity: I don't fully understand this fix, and am worried about the mismatch between localSlotCount() and the actual number of slots being allocated.

If we need to, we should align localSlotCount at its source, but I'd like to know why it's crashing.

Comment 18

[Alex Keybl [:akeybl]]

Emailed Marty about next steps here.

Comment 19

[Alex Keybl [:akeybl]]

Haven't yet gotten a response.

Comment 20

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/alignAllTheStacks-r1.patch

Sorry for the delay, I was buys with odin.  This is the simpler scheme that dvander and I talked about.

Comment 21

[Marty Rosenberg [:mjrosenb]]

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/538ec52a9156

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/538ec52a9156

Comment 23

[bhavana bajaj [:bajaj]]

Given this has baked on m-c for a few days can we uplift this on aurora/beta ?

Comment 24

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 730171 [details] [diff] [review]
/home/mjrosenb/patches/alignAllTheStacks-r1.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: (Mobile only) debug builds can throw an assertion, opt builds will most likely crash.  It is possible for some compiler-controlled data to leak places in the stack reconstructor that it should not.
Testing completed (on m-c, etc.): on m-c for about a week now
Risk to taking this patch (and alternatives if risky): this may be the safest patch I have ever written.
String or IDL/UUID changes made by this patch:

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73986f0ec459
https://hg.mozilla.org/releases/mozilla-beta/rev/92ebf138fc39