Closed
Bug 1373094
Opened 7 years ago
Closed 7 years ago
Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/MacroAssembler.cpp:1695
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | + | fixed |
firefox56 | + | fixed |
People
(Reporter: bc, Assigned: jandem)
References
()
Details
(4 keywords, Whiteboard: [post-critsmash-triage])
Attachments
(5 files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
application/x-javascript
|
Details | |
(deleted),
patch
|
tcampbell
:
review+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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•7 years ago
|
Version: 55 Branch → 56 Branch
Comment 2•7 years ago
|
||
Bug 1368362 is hitting the same assertion. Maybe this is a dupe with better STR?
Group: core-security → javascript-core-security
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 3•7 years 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•7 years ago
|
||
This fails with --no-threads.
Assignee | ||
Comment 5•7 years 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•7 years ago
|
||
Thanks Gary! That's very useful because these TI failures are not trivial to debug. I'll investigate soon.
Assignee | ||
Comment 9•7 years ago
|
||
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•7 years ago
|
||
Attachment #8880309 -
Flags: review?(tcampbell)
Assignee | ||
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox55:
--- → ?
tracking-firefox56:
--- → ?
Keywords: sec-high → sec-critical
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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+
Comment 13•7 years ago
|
||
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•7 years 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 15•7 years ago
|
||
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•7 years ago
|
Comment 16•7 years ago
|
||
jan, still need to land and also need beta approval
Updated•7 years ago
|
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99aaef31a9a33ebac977a82db23480441f49468
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 18•7 years 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•7 years 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•7 years ago
|
Attachment #8880306 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/a99aaef31a9a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 21•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ebda0666d66f
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•