Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/MacroAssembler.cpp:1695

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bc, Assigned: jandem)

Tracking

(Blocks: 1 bug, 4 keywords)

56 Branch
mozilla56
assertion, crash, reproducible, sec-critical
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [post-critsmash-triage], URL)

Attachments

(5 attachments)

(Reporter)

Description

a year ago
Created attachment 8877819 [details]
windows assertion crash report @ js::jit::IonCompileScriptForBaseline with ebx = 0xdeadbeef

1. https://manishearth.github.io/rust-internals-docs/rustc/ty/adjustment/enum.Adjust.html?search=JJ

2. Assertion failure: MIR instruction returned object with unexpected type, at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jit/MacroAssembler.cpp:1695

Thread 0 (crashed)
 0  0x149ac691
    eip = 0x149ac691   esp = 0x006fd2ec   ebp = 0xffffff8c   ebx = 0xdeadbeef
    esi = 0x0c631378   edi = 0x1e9c3010   eax = 0x1e6a6ec0   ecx = 0x1e6b2670
    edx = 0x00000003   efl = 0x00200202
    Found by: given as instruction pointer in context
 1  xul.dll!js::jit::IonCompileScriptForBaseline(JSContext *,js::jit::BaselineFrame *,unsigned char *) [Ion.cpp:da66c4a05fda : 2705 + 0xc]
    eip = 0x54029c01   esp = 0x006fd2f8   ebp = 0x006fd318
    Found by: stack scanning

Even though this is an assert, I'm marking s-s since ebx = 0xdeadbeef.

reproduced with today's builds on Windows and Linux in Bughunter and also a local Linux build from 2017-06-07.
(Reporter)

Comment 1

a year ago
Created attachment 8877822 [details]
Windows assertion crash report @ EnterBaseline

Assertion failure: MIR instruction returned object with unexpected type, at c:/builds/moz2_slave/m-cen-w64-d-000000000000000000/build/src/js/src/jit/MacroAssembler.cpp:1695

Linux debug builds also assert with this same stack and Windows 7 opt builds has also crashed with this stack at least once.
(Reporter)

Updated

a year ago
Version: 55 Branch → 56 Branch
Bug 1368362 is hitting the same assertion. Maybe this is a dupe with better STR?
Group: core-security → javascript-core-security
Flags: needinfo?(jdemooij)
See Also: → bug 1368362
Keywords: sec-high
(Assignee)

Comment 3

a year ago
I have a shell testcase, reducing it now (it's pretty huge because it contains the Rust documentation search index of > 1.5 MB). Fortunately we can discard most of it.
(Assignee)

Comment 4

a year ago
Created attachment 8879624 [details]
Shell test

This fails with --no-threads.
(Assignee)

Comment 5

a year ago
Gary maybe you can run autoBisect on the attached shell test? It should repro with --no-threads.
Flags: needinfo?(gary)
I couldn't reproduce with the shell test in comment 4 using debug builds from m-c rev 464b2a3c25aa. What rev did you use, and what are the compilation/runtime parameters?
Flags: needinfo?(gary)
I managed to reproduce this with the testcase in comment 4 after all, with a build that does not have --enable-more-deterministic set.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ba5cee986655
user:        Jan de Mooij
date:        Wed May 31 09:43:16 2017 +0200
summary:     Bug 1368509 part 2 - Generalize Baseline unknown/unknownObject type update stubs. r=tcampbell

Jan, is bug 1368509 a likely regressor?
Blocks: 1368509
(Assignee)

Comment 8

a year ago
Thanks Gary! That's very useful because these TI failures are not trivial to debug. I'll investigate soon.
(Assignee)

Comment 9

a year ago
Created attachment 8880306 [details] [diff] [review]
Part 1 - Fix

Blegh, in addUpdateStubForValue we look at obj->group() but that's not guaranteed to be the group we care about if obj is an unboxed expando object - see DoTypeUpdateFallback. The unboxed expando object has unknown properties, so we'd incorrectly attach an AnyValue stub.

We can just pass the group to addUpdateStubForValue and use it.

I'm going to hide in a corner now because I wrote DoTypeUpdateFallback and should have realized this.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8880306 - Flags: review?(tcampbell)
(Assignee)

Comment 10

a year ago
Created attachment 8880309 [details] [diff] [review]
Part 2 - Test + comment
Attachment #8880309 - Flags: review?(tcampbell)
(Assignee)

Updated

a year ago
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
Keywords: sec-high → sec-critical
Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

Review of attachment 8880306 [details] [diff] [review]:
-----------------------------------------------------------------

Ah dang, I also should have noticed this too when we had to fix the last issue of mismatch with DoTypeUpdateFallback.

Looks good now.

::: js/src/jit/SharedIC.cpp
@@ +2557,1 @@
>          AddTypePropertyId(cx, obj, id, val);

Is there any material difference between this and the variant that DoTypeUpdateFallback uses?
Attachment #8880306 - Flags: review?(tcampbell) → review+
Comment on attachment 8880309 [details] [diff] [review]
Part 2 - Test + comment

Review of attachment 8880309 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Testcase seems a little fragile, so many a comment to help someone who trips over it later.

::: js/src/jit-test/tests/baseline/bug1373094.js
@@ +7,5 @@
> +    for (var i = 0; i < 100; i++) {
> +        arr.push({x: i});
> +    }
> +    for (var i = 0; i < 1600; i++) {
> +        assertEq(inIon() === true, false);

I imagine this test sneaking up on someone in the future when our optimizers change. Can we have a comment about how to address this? (eg. remove assertion)
Attachment #8880309 - Flags: review?(tcampbell) → review+
seems http://static.echonest.com/autocanonizer/go.html?trid=TRJQDYU14404EDA900 is another live url for this assertion, will check this url after this bug here is fixed
(Assignee)

Comment 14

a year ago
Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's not trivial but possible with some 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?
55+.

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

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

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8880306 - Flags: sec-approval?
Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

sec-approval+.
If it is 55+ only, please nominate a patch for Beta as well.
Attachment #8880306 - Flags: sec-approval? → sec-approval+

Updated

a year ago
tracking-firefox55: ? → +
tracking-firefox56: ? → +
jan, still need to land and also need beta approval
Flags: needinfo?(jdemooij)
(Assignee)

Comment 18

a year ago
(In reply to Ted Campbell [:tcampbell] [pto until July 11] from comment #11)
> Is there any material difference between this and the variant that
> DoTypeUpdateFallback uses?

Not really but I think we have to do this here after the EnsureTrackPropertyTypes call.
(Assignee)

Comment 19

a year ago
Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1368509.
[User impact if declined]: Security bugs, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Patch is pretty straight-forward.
[String changes made/needed]: None.
Attachment #8880306 - Flags: approval-mozilla-beta?

Updated

a year ago
Attachment #8880306 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/a99aaef31a9a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: javascript-core-security → core-security-release
Whiteboard: [post-critsmash-triage]

Updated

11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.