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

VERIFIED FIXED in Firefox 27

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: bhackett)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla29
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:koi+, 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)

Details

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

Attachments

(2 attachments)

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.
Posted 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.
No longer blocks: 349611
> 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.
https://hg.mozilla.org/mozilla-central/rev/0c2a1f9f76a0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.