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

VERIFIED FIXED in Firefox 29

Status

()

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

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 wontfix, firefox29+ fixed, firefox30+ fixed, firefox31+ verified, firefox-esr24+ fixed, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [jsbugmon:][adv-main29+][adv-esr24.5+])

Attachments

(2 attachments)

Reporter

Description

5 years ago
Posted file stack
function g(x) {
    return a = Math ? x : 0
}
function f() {
    ({} > g()), g(Math.round())
}
try {
    f()
} catch (e) {}
g = (function() {
    return 0
})
f()
f()

asserts js debug shell on m-c changeset 199e65efd08b with --ion-parallel-compile=off --ion-eager at Assertion failure: MIR instruction returned value with unexpected type, at jit/IonMacroAssembler.cpp

s-s because this seems to be a Mid-Level Intermediate Representation issue, js folks say keep it s-s by default for now.

My configure flags are:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

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

Jan, perhaps bug 949475 caused a latent bug to show up?
Flags: needinfo?(jdemooij)
Assignee

Comment 1

5 years ago
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Jan, perhaps bug 949475 caused a latent bug to show up?

Yup, looks like it. Will take a look this week.
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Assignee

Comment 3

5 years ago
Posted patch PatchSplinter Review
Consider this write:

    return a = (Math ? x : 0);

In TryAddTypeBarrierForWrite, we have a value with TypeSet {undefined, int32} and propertyType == double.

We add a fallible MUnbox to double (which will always bail), but because SETPROP leaves this MUnbox on the stack, it flows into an MPhi after inlining and confuses the MPhi's resultType/resultTypeSet.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8396686 - Flags: review?(bhackett1024)
Flags: needinfo?(jdemooij)
Attachment #8396686 - Flags: review?(bhackett1024) → review+
Hi Jan how should this be sec rated? (How bad/exploitable is it)
Flags: needinfo?(jdemooij)
Please nominate this for sec-approval so we can get this into trunk and then make and nominate Aurora and Beta patches.
Keywords: sec-high
Assignee

Comment 6

5 years ago
Comment on attachment 8396686 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easy.. Maybe with a lot of work.

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? 24+.

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

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

How likely is this patch to cause regressions; how much testing does it need? Pretty unlikely.
Attachment #8396686 - Flags: sec-approval?
Flags: needinfo?(jdemooij)
Comment on attachment 8396686 [details] [diff] [review]
Patch

sec-approval+ for trunk. Let's get Aurora, Beta, and ESR24 patches created and nominated too, please.
Attachment #8396686 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/fdf61269816b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee

Comment 10

5 years ago
Comment on attachment 8396686 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 804676.
User impact if declined: Crashes, possible security bugs.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

[Approval Request Comment]
Fix Landed on Version: On m-c (31), will likely be backported.
Attachment #8396686 - Flags: approval-mozilla-esr24?
Attachment #8396686 - Flags: approval-mozilla-beta?
Attachment #8396686 - Flags: approval-mozilla-aurora?
Attachment #8396686 - Flags: approval-mozilla-esr24?
Attachment #8396686 - Flags: approval-mozilla-esr24+
Attachment #8396686 - Flags: approval-mozilla-beta?
Attachment #8396686 - Flags: approval-mozilla-beta+
Attachment #8396686 - Flags: approval-mozilla-aurora?
Attachment #8396686 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main29+][adv-esr24.5+]
Hi Gary - can you confirm that this has been fixed? Thanks!
Flags: needinfo?(gary)
Reporter

Comment 13

5 years ago
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/fdf61269816b
user:        Jan de Mooij
date:        Fri Apr 04 19:38:10 2014 +0200
summary:     Bug 986678 - Fix type check in TryAddTypeBarrierForWrite. r=bhackett

Yes, this is definitely fixed by the patch here.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gary)
Group: core-security
You need to log in before you can comment on or make changes to this bug.