870034 - 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]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


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

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

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

Comment 3

[Christian Holler (:decoder)]

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.

Comment 4

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

Marking this as Firefox 22 and higher based on comment 3.

Comment 5

[Alex Keybl [:akeybl]]

Naveed - this is a sec-critical regression in FF22 (releasing in ~6wk). Can we get an assignee here?

Comment 6

[Luke Wagner [:luke]]

Nick's out, so I can take it.

Comment 7

[Luke Wagner [: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)
        f(false);
    else
        g = { apply:parseInt };
    g.apply(null, a);
}
f(true);

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?

Comment 8

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test

Comment 9

[Hannes Verschore [:h4writer]]

I'm starting to think comment 7 is incorrect?!

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

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?

Comment 10

[Christian Holler (:decoder)]

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.

Comment 11

[Luke Wagner [:luke]]

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:
  http://hg.mozilla.org/releases/mozilla-beta/rev/3a535bd50a23
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.

Comment 12

[Hannes Verschore [:h4writer]]

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).

Comment 13

[Nicholas Nethercote [inactive]]

So is my patch from bug 851421 off the hook?

Comment 14

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

Setting needinfo from Brian as per comment 10, and he can confirm comment 13.

Comment 15

[Brian Hackett [Laid off!]]

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.

Comment 16

[Kannan Vijayan [:djvj]]

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.

Comment 17

[Kannan Vijayan [:djvj]]

Attachment: Fix.

See previous comment for issue description.

Comment 18

[Kannan Vijayan [:djvj]]

I think the most appropriate candidate bug for the regression would be bug 811875, patch:

changeset:   127099:420892dc7b4d
user:        Jan de Mooij <[e-mail]>
date:        Fri Nov 16 13:20:18 2012 +0100
summary:     Bug 811875 part 1 - Support calls. r=djvj

Comment 19

[Kannan Vijayan [:djvj]]

Try run looks green on Linux 32 and 64-bit:
https://tbpl.mozilla.org/?tree=Try&rev=75833d00c545

Comment 20

[Brian Hackett [Laid off!]]

Comment on attachment 758652 [details] [diff] [review]
Fix.

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.

grammar

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

whitespace

Comment 21

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be065b3be5fe

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/be065b3be5fe

Comment 23

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 24

[Kannan Vijayan [:djvj]]

Comment on attachment 758652 [details] [diff] [review]
Fix.

[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:
None.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/713a74635e1c

Comment 26

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98b3cca0fe06

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98b3cca0fe06