Closed Bug 986678 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
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

People

(Reporter: gkw, Assigned: jandem)

References

Details

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

Attachments

(2 files)

Attached 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)
(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.
Attached 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
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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)
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.