Closed Bug 870034 Opened 11 years ago Closed 11 years ago

IonMonkey: Assertion failure: type == MIRType_Boolean || type == MIRType_Int32 || type == MIRType_Double || type == MIRType_String || type == MIRType_Object, at ion/MIR.h:1946 or Crash [@ LUnbox]


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 + fixed
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: decoder, Assigned: djvj)



(5 keywords, Whiteboard: [jsbugmon:update,testComment=7,origRev=41ff3b67b692][adv-main23-])


(2 files)

The following testcase asserts on mozilla-central revision 41ff3b67b692 (run with --ion-eager):

function f(b) {
    var a = arguments;
    if (b)
        g = {
            apply:function(x,y) { "use asm"; function g() {} return g }
    g.apply(null, a);
Marked s-s because these MIRType failures we're s-s sometimes. The crash looks like a null-crash though:

Program received signal SIGSEGV, Segmentation fault.
0x08423bbd in LUnbox (this=<optimized out>) at ../ion/x86/LIR-x86.h:44
44      class LUnbox : public LInstructionHelper<1, 2, 0>
#0  0x08423bbd in LUnbox (this=<optimized out>) at ../ion/x86/LIR-x86.h:44
#1  js::ion::LIRGeneratorX86::visitUnbox (this=0xffffb690, unbox=0x910e6d8) at js/src/ion/x86/Lowering-x86.cpp:99
#2  0x085197aa in js::ion::MUnbox::accept (this=0x910e6d8, visitor=0xffffb690) at js/src/ion/MIR.h:1959
#3  0x083cc425 in visitInstruction (ins=0x910e6d8, this=0xffffb690) at js/src/ion/Lowering.cpp:2702
#4  js::ion::LIRGenerator::visitBlock (this=0xffffb690, block=0x910e338) at js/src/ion/Lowering.cpp:2794
#5  0x083cc7eb in js::ion::LIRGenerator::generate (this=0xffffb690) at js/src/ion/Lowering.cpp:2870
#6  0x08344ffd in js::ion::GenerateLIR (mir=0x910dfd8) at js/src/ion/Ion.cpp:1135
#7  0x083469d4 in CompileBackEnd (mir=<optimized out>, maybeMasm=<optimized out>) at js/src/ion/Ion.cpp:1231
eax     0x90ae228       151708200
esi     0x0     0
=> 0x8423bbd <js::ion::LIRGeneratorX86::visitUnbox(js::ion::MUnbox*)+205>:      mov    %eax,(%esi)
   0x8423bbf <js::ion::LIRGeneratorX86::visitUnbox(js::ion::MUnbox*)+207>:      mov    (%edi),%eax
Blocks: IonFuzz
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   126041:30b977b2b911
user:        Nicholas Nethercote
date:        Thu Mar 14 18:44:03 2013 -0700
summary:     Bug 851421 (part 2) - Don't emit bytecode for asm.js functions unless linking fails.  r=luke.

This iteration took 123.983 seconds to run.
Marking this as Firefox 22 and higher based on comment 3.
Naveed - this is a sec-critical regression in FF22 (releasing in ~6wk). Can we get an assignee here?
Assignee: general → nihsanullah
Nick's out, so I can take it.
Assignee: nihsanullah → luke
The root of the problem is a bad interaction between the apply-arguments optimization and jit compilation of calls to natives: while ion::DoCallFallback does GuardFunApplyArgumentsOptimization, the compiled call to the js::Native does no such guard.

Here's a testcase that doesn't involve asm.js (run with --ion-eager):

function f(b) {
    var a = arguments;
    if (b)
        g = { apply:parseInt };
    g.apply(null, a);

I think the fix is pretty simple: don't compile native calls where op == JSOP_FUNAPPLY and the callee is not fun_apply.  It'd also be good to check that the same bug isn't present on the scripted call path.  As a side note, I think DoCallfallback could call the overload of GuardFunApplyArgumentsOptimization that takes an AbstractFramePtr (to avoid duplicating the args-probing logic).

Perhaps Jan or Hannes could take this?
Assignee: luke → general
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,testComment=7,bisect]
Whiteboard: [jsbugmon:update,testComment=7,bisect] → [jsbugmon:testComment=7,bisect]
JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test
Whiteboard: [jsbugmon:testComment=7,bisect] → [jsbugmon:update,testComment=7,origRev=41ff3b67b692,bisect]
I'm starting to think comment 7 is incorrect?!

function f(b) {
    var a = arguments;
    if (b)
        g = {
            apply:function(x,y) { "use asm"; function g() {} return g }
    g.apply(null, a);

Fails on FF22 shell build (has no baseline) with "--no-ion --no-jm".
So this even segfaults when there are no monkeys active! (The argument optimization I added is in ionmonkey/baseline only. So I don't think that is the problem).

Also the testcase provided in comment 7 fails only on baseline. Ionmonkey is smart enough to disable compilation ("fun.apply speculation failed"). So that would be FF23+. Fixing this issue wouldn't fix the original issue! (As sidenote I think baseline doesn't do the argument optimization)

Also the issue I see in the original testcase is caused because fun->name doesn't exist ... Why wouldn't that exist? Could it be because bytecode isn't emitted yet?
Flags: needinfo?(jdemooij) → needinfo?(luke)
Whiteboard: [jsbugmon:update,testComment=7,origRev=41ff3b67b692,bisect] → [jsbugmon:update,testComment=7,origRev=41ff3b67b692]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   127250:f21ddc17c570
user:        Brian Hackett
date:        Thu Feb 07 13:03:12 2013 -0700
summary:     Bug 839080 - Compile object initializer opcodes, r=djvj.

This iteration took 120.429 seconds to run.
I just pulled mozilla-beta (which should be FF22) and tested comment 9 and I didn't see any issue, in a debug shell, no matter which flags I pass.  A segfault at fun->name sounds exactly like bug 854448, but that is fixed on beta:
Can you confirm that you can repro on a clean pull of beta and, if so, give me more details on how to repro?

Regardless, the arguments-speculation failure pointed to by comment 7 is a separate bug and the cause of the MIRType assert in comment 0.  I can confirm it is only an issue on FF23 and up.
Flags: needinfo?(luke)
Thanks, that indeed explains it. I was taking a random revision from ff22. So I indeed got the issue mentioned (bug 854448).

Comment 7 has the testcase with the real problem. Just like luke said FF23+. Happens only in baseline. Baseline only landed in FF23. And bisect is also pointing to a revision in the ionmonkey branch. (was used for development of baseline).
So is my patch from bug 851421 off the hook?
Setting needinfo from Brian as per comment 10, and he can confirm comment 13.
Flags: needinfo?(bhackett1024)
I think I'm going to have to kick the can down the road some more.  The rev in comment 10 is a pretty boilerplate patch that allowed baseline to compile more scripts, so the problem is almost certainly that after that rev we were now baseline compiling this script when we weren't before.
Flags: needinfo?(kvijayan)
Flags: needinfo?(jdemooij)
Flags: needinfo?(bhackett1024)
Found the issue.  In the first call to |f|, we immediately re-enter |f| via the first branch of the conditional.  In the inner call to |f|, we run the second branch of the conditional and initialize the global |g| to the funky-apply-overridden object.

Still in the inner call to |f|, we run the last line, which works fine, but it attaches an optimized stub for the call (not a special "FUNAPPLY" optimized stub, just a regular optimized call stub for a 2-argument call).

Then the inner function exits its frame, and the outer function enters the FUNAPPLY's call IC.  But this time the IC contains an optimized stub that handles the funapply as a regular call, and doesn't check for a magic arguments.

Solution is just to disable generation of optimized "regular-call" ICs when compiling FUNAPPLY ops, since that can allow MagicArguments to escape the frame.
Flags: needinfo?(kvijayan)
Flags: needinfo?(jdemooij)
Attached patch Fix.Splinter Review
See previous comment for issue description.
Attachment #758652 - Flags: review?(bhackett1024)
Assignee: general → kvijayan
I think the most appropriate candidate bug for the regression would be bug 811875, patch:

changeset:   127099:420892dc7b4d
user:        Jan de Mooij <>
date:        Fri Nov 16 13:20:18 2012 +0100
summary:     Bug 811875 part 1 - Support calls. r=djvj
Try run looks green on Linux 32 and 64-bit:
Comment on attachment 758652 [details] [diff] [review]

Review of attachment 758652 [details] [diff] [review]:

::: js/src/ion/BaselineIC.cpp
@@ +6890,5 @@
> +                return TryAttachFunApplyStub(cx, stub, script, pc, thisv, argc, vp + 2);
> +
> +            // Aside from optimized FunApply stubs, never try to attach a "regular"
> +            // native-function optimized call stub for FUNAPPLY ops, since MagicArguments
> +            // may escape through those stub.


@@ +6895,3 @@
>              return true;
>          }
> +            

Attachment #758652 - Flags: review?(bhackett1024) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 758652 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 811875

User impact if declined: 
Incorrect behaviour, security vulnerability.  JS magic values can leak to outside world, and attackers can induce this behaviour by simple code engineering.

Testing completed (on m-c, etc.): 
Green on m-c for 1 day.

Risk to taking this patch (and alternatives if risky): 
Low.  Only alters compile logic to disable compilation of stubs in certain situations.

String or IDL/UUID changes made by this patch:
Attachment #758652 - Flags: approval-mozilla-aurora?
Attachment #758652 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite? → in-testsuite+
Whiteboard: [jsbugmon:update,testComment=7,origRev=41ff3b67b692] → [jsbugmon:update,testComment=7,origRev=41ff3b67b692][adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.