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]

VERIFIED FIXED in Firefox 23

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: djvj)

Tracking

(Blocks: 2 bugs, 5 keywords)

Trunk
mozilla24
x86
Linux
assertion, crash, regression, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 unaffected, firefox22 unaffected, firefox23+ fixed, firefox24+ verified, firefox-esr17 unaffected, b2g18 unaffected)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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);
(Reporter)

Comment 1

5 years ago
Created attachment 747049 [details]
[crash-signature] Machine-readable crash signature
(Reporter)

Comment 2

5 years ago
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: 724444
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 3

5 years ago
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.
Keywords: regression, sec-high
Marking this as Firefox 22 and higher based on comment 3.
status-firefox21: --- → unaffected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?

Comment 5

5 years ago
Naveed - this is a sec-critical regression in FF22 (releasing in ~6wk). Can we get an assignee here?
Assignee: general → nihsanullah
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +

Comment 6

5 years ago
Nick's out, so I can take it.
Assignee: nihsanullah → luke

Comment 7

5 years ago
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?

Updated

5 years ago
Assignee: luke → general
Flags: needinfo?(jdemooij)
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,testComment=7,bisect]
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update,testComment=7,bisect] → [jsbugmon:testComment=7,bisect]
(Reporter)

Comment 8

5 years ago
JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:testComment=7,bisect] → [jsbugmon:update,testComment=7,origRev=41ff3b67b692,bisect]
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?
Flags: needinfo?(jdemooij) → needinfo?(luke)
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update,testComment=7,origRev=41ff3b67b692,bisect] → [jsbugmon:update,testComment=7,origRev=41ff3b67b692]
(Reporter)

Comment 10

5 years ago
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

5 years ago
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.
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).
status-firefox22: affected → unaffected
tracking-firefox22: + → ---
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)
(Assignee)

Comment 16

5 years ago
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)
(Assignee)

Comment 17

5 years ago
Created attachment 758652 [details] [diff] [review]
Fix.

See previous comment for issue description.
Attachment #758652 - Flags: review?(bhackett1024)
Assignee: general → kvijayan
(Assignee)

Comment 18

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

changeset:   127099:420892dc7b4d
user:        Jan de Mooij <jdemooij@mozilla.com>
date:        Fri Nov 16 13:20:18 2012 +0100
summary:     Bug 811875 part 1 - Support calls. r=djvj
(Assignee)

Comment 19

5 years ago
Try run looks green on Linux 32 and 64-bit:
https://tbpl.mozilla.org/?tree=Try&rev=75833d00c545
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
Attachment #758652 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/be065b3be5fe
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox24: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 23

5 years ago
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Comment 24

5 years ago
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.
Attachment #758652 - Flags: approval-mozilla-aurora?
Attachment #758652 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox-esr17: --- → unaffected
(Assignee)

Updated

5 years ago
Flags: in-testsuite? → in-testsuite+
status-firefox24: fixed → verified
Whiteboard: [jsbugmon:update,testComment=7,origRev=41ff3b67b692] → [jsbugmon:update,testComment=7,origRev=41ff3b67b692][adv-main23-]
status-b2g18: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.