Closed Bug 950452 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 + fixed
firefox28 + fixed
firefox29 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: decoder, Assigned: bhackett1024)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main27+])

Attachments

(2 files)

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;
};
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).
Flags: needinfo?(bhackett1024)
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:   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.
Attached patch patchSplinter Review
Assignee: general → bhackett1024
Attachment #8348155 - Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
Attachment #8348155 - Flags: review?(jdemooij) → review+
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
Attachment #8348155 - Flags: sec-approval?
Attachment #8348155 - Flags: approval-mozilla-beta?
Attachment #8348155 - Flags: approval-mozilla-aurora?
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?
This should probably be critical, it would require a lot of knowledge about the engine's internals to exploit but could probably be done.
Keywords: sec-critical
Comment on attachment 8348155 [details] [diff] [review]
patch

Approve all things!

sec-approval+ granted, along with Aurora and Beta.
Attachment #8348155 - Flags: sec-approval?
Attachment #8348155 - Flags: sec-approval+
Attachment #8348155 - Flags: approval-mozilla-beta?
Attachment #8348155 - Flags: approval-mozilla-beta+
Attachment #8348155 - Flags: approval-mozilla-aurora?
Attachment #8348155 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Target Milestone: --- → mozilla29
Brian, maybe we need to set the MMul's specialization as well.
Flags: needinfo?(bhackett1024)
OK, yes (it would be nice if binary arithmetic instructions were more robust here).

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2a1f9f76a0
Flags: needinfo?(bhackett1024)
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
(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.
> 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).
(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.
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Sec critical = blocker for 1.2.
blocking-b2g: koi? → koi+
Please provide a branch-specific patch for b2g26 uplift.
Flags: needinfo?(bhackett1024)
(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);
}
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main27+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.