Closed Bug 814552 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, major)

ARM
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox19 - wontfix
firefox20 + wontfix
firefox21 + fixed
firefox22 + fixed
firefox23 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g18-v1.0.0 --- unaffected
b2g18-v1.0.1 --- unaffected

People

(Reporter: decoder, Assigned: mjrosenb)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:ignore][fuzzblocker][adv-main21+])

Crash Data

Attachments

(3 files)

Attached file Testcase for shell
The attached testcase asserts on mozilla-central revision bc69705c162d (run with --ion-eager).
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
Assignee: general → mrosenberg
Crash Signature: [@ MarkInternal<js::ion::IonCode>]
Keywords: crash
Whiteboard: [jsbugmon:ignore]
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?
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
Assignee: mrosenberg → general
This needs a security rating, Marty, can you judge the severity here?
Flags: needinfo?(mrosenberg)
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 });
This keeps triggering, also in various release crashes.
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][fuzzblocker]
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.
Assignee: general → nihsanullah
Assignee: nihsanullah → mrosenberg
Status: NEW → ASSIGNED
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.
Attachment #718716 - Flags: review?(dvander)
Flags: needinfo?(mrosenberg)
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.)
Attachment #718716 - Flags: review?(dvander) → review+
Al - do you suspect this is a new or easily exploited regression in FF19 and something we should consider fixing before FF20?
Flags: needinfo?(abillings)
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.
Flags: needinfo?(abillings)
I don't know how easily this can be exploited, but it should be ARM only.
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.
Needs a sec-approval nomination first.
(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 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.
Attachment #718716 - Flags: review+
Emailed Marty about next steps here.
Haven't yet gotten a response.
Sorry for the delay, I was buys with odin.  This is the simpler scheme that dvander and I talked about.
Attachment #730171 - Flags: review?(dvander)
Attachment #730171 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/538ec52a9156
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Given this has baked on m-c for a few days can we uplift this on aurora/beta ?
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:
Attachment #730171 - Flags: approval-mozilla-beta?
Attachment #730171 - Flags: approval-mozilla-aurora?
Attachment #730171 - Flags: approval-mozilla-beta?
Attachment #730171 - Flags: approval-mozilla-beta+
Attachment #730171 - Flags: approval-mozilla-aurora?
Attachment #730171 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:ignore][fuzzblocker] → [jsbugmon:ignore][fuzzblocker][adv-main21+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.