950452 - Assertion failure: MIR instruction returned value with unexpected type, at jit/IonMacroAssembler.cpp:1219

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision c049cb230d77 (run with --fuzzing-safe --ion-eager):


var ff = new f("foo")
function f(y) {
  y = +y;
};

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Jan de Mooij [:jandem]]

function f(y) {
  y = +y;
}

We add an MMul for the +y. Then in MBinaryArithInstruction::inferFallback, we give this MMul an empty resultTypeSet, because the argument has an empty resultTypeSet.

Then in IonBuilder::jsop_setarg, we detect this argument coercion pattern, but the MMul still has its empty TypeSet, and the type check fails.

Brian, can you take a look? It looks like this could also be a perf issue (the MMul has unknown type).

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:   http://hg.mozilla.org/mozilla-central/rev/1b91cf5c8407
user:        Jan de Mooij
date:        Sat Dec 14 14:32:35 2013 +0100
summary:     Bug 949475 - Add some debug-only sanity checks. r=bhackett

This iteration took 337.377 seconds to run.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 8348155 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not at all.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

aurora through release

If not all supported branches, which bug introduced the flaw?

Bug 902508

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

trivial

How likely is this patch to cause regressions; how much testing does it need?

not at all

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 902508
User impact if declined: potential, difficult to trigger exploit
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): none

Comment 6

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

We need to get a security rating on this issue before it can be approved. What are the implications of this as far as exploits, etc?

Comment 7

[Brian Hackett [Laid off!]]

This should probably be critical, it would require a lot of knowledge about the engine's internals to exploit but could probably be done.

Comment 8

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

Comment on attachment 8348155 [details] [diff] [review]
patch

Approve all things!

sec-approval+ granted, along with Aurora and Beta.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1035869d1819

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1035869d1819

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ab713e6d552d for apparently introducing a jit-test failure: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=32255377&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=32255930&tree=Mozilla-Inbound

Comment 14

[Jan de Mooij [:jandem]]

Brian, maybe we need to set the MMul's specialization as well.

Comment 15

[Brian Hackett [Laid off!]]

OK, yes (it would be nice if binary arithmetic instructions were more robust here).

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2a1f9f76a0

Comment 17

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

setJitCompilerOption('ion.usecount.trigger', 5)
for (var i = 0; i < 99; i++) {
    (function g(x) {
        x = +x;
    })()
}

Testcase generated by jsfunfuzz, run with --ion-parallel-compile=off and use m-c rev c049cb230d77

Comment 18

[Christian Holler (:decoder)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #17)

> Testcase generated by jsfunfuzz, run with --ion-parallel-compile=off and use
> m-c rev c049cb230d77

Almost identical test is in comment 0, for the same revision. Please don't just add duplicate tests to the bugs, it doesn't serve a purpose here.

Comment 19

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

> Almost identical test is in comment 0, for the same revision. Please don't
> just add duplicate tests to the bugs, it doesn't serve a purpose here.

We clarified our strategy going forward - there was a bunch of confusion by folks around here (including me).

Comment 20

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

(In reply to Brian Hackett (:bhackett) from comment #15)
> OK, yes (it would be nice if binary arithmetic instructions were more robust
> here).
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2a1f9f76a0

Using TBPL builds, I've verified that the testcase in comment 17 was indeed fixed by this revision.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c2a1f9f76a0

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db7f78ab4562
https://hg.mozilla.org/releases/mozilla-beta/rev/486381f7cf34

Comment 24

[Jason Smith [:jsmith]]

Sec critical = blocker for 1.2.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please provide a branch-specific patch for b2g26 uplift.

Comment 26

[Brian Hackett [Laid off!]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #25)
> Please provide a branch-specific patch for b2g26 uplift.

I don't have a copy of the b2g tree but in IonBuilder.cpp this paragraph:

if (!otherUses) {
    types::TypeSet *argTypes = types::TypeScript::ArgTypes(script(), arg);

    // Update both the original and cloned type set.
    argTypes->addType(cx, types::Type::UnknownType());
    op->resultTypeSet()->addType(cx, types::Type::UnknownType());
}

Should be changed to:

if (!otherUses) {
    types::TypeSet *argTypes = types::TypeScript::ArgTypes(script(), arg);

    // Update both the original and cloned type set.
    argTypes->addType(cx, types::Type::UnknownType());
    op->resultTypeSet()->addType(cx, types::Type::UnknownType());

    if (value->isMul()) {
        value->setResultType(MIRType_Double);
        value->toMul()->setSpecialization(MIRType_Double);
    } else {
        JS_ASSERT(value->type() == MIRType_Int32);
    }
    value->setResultTypeSet(nullptr);
}

Comment 27

[Ryan VanderMeulen [:RyanVM]]

That works, thanks :P

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/349cceaaab42