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

RESOLVED FIXED in Firefox -esr52

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks 1 bug, 6 keywords)

Trunk
mozilla56
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [jsbugmon:][adv-main55+][adv-esr52.3+][post-critsmash-triage])

Attachments

(2 attachments)

Reporter

Description

2 years ago
The following testcase crashes on mozilla-central revision 0ed0fd886134 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --disable-oom-functions --wasm-always-baseline --ion-eager --baseline-eager --ion-extra-checks):

See attachment (and description below).


Backtrace:

Program terminated with signal SIGTRAP, Trace/breakpoint trap.
#0  0x0000183a1dbff1fa in ?? ()
[...]
#13 0x0000000000000000 in ?? ()
rax	0x7f14391bc760	139724834195296
rbx	0x7f14391b20c0	139724834152640
rcx	0x7f1439200480	139724834473088
rdx	0x0	0
rsi	0x20	32
rdi	0x7ffdedbe72c0	140728592134848
rbp	0x7f1438d08340	139724829262656
rsp	0x7ffdedbe7368	140728592135016
r8	0x1a	26
r9	0x1a	26
r10	0x7f1438d80008	139724829753352
r11	0x0	0
r12	0x0	0
r13	0x0	0
r14	0x7f14391acc00	139724834130944
r15	0x7f1439125640	139724833576512
rip	0x183a1dbff1fa	26637886288378
=> 0x183a1dbff1fa:	push   %r10
   0x183a1dbff1fc:	push   %r9


This bug is highly intermittent. The attached testcase only reproduces for me in about 0.2% (!) of all runs on the specified revision. Furthermore, you need to create a file on your disk with the path/name "/home/ubuntu/wasm/6.wasm" with the following contents:

(module (import $imp "" "inc") (func) (func $start (call $imp)) (start $start) (export "" $start))

I was not able to inline this into the testcase. I suspect there is some very fragile timing going on. It would be good if we could also make this more reliable so we can find similar bugs more easily.

Marking s-s because the abort message does not sound safe at all.
Reporter

Comment 1

2 years ago
Posted file Testcase
Reporter

Comment 2

2 years ago
Setting NI for Benjamin since this seems related to WebAssembly.
Flags: needinfo?(bbouvier)

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 3

2 years ago
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Updated

2 years ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]

Comment 4

2 years ago
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
Actually looking at the runtime command line, there's --wasm-always-baseline, so Baldr doesn't use Ion, thus it doesn't use the CodeGenerator, which causes this runtime assertion (in generated code): http://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#5088

Digging a bit, the likely top caller we can identify that generates this check could be http://searchfox.org/mozilla-central/source/js/src/jit/shared/Lowering-shared-inl.h#409 (the other ones can be eliminated because the test doesn't run with fullDebugChecks (see also http://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#5403 )).

So the issue is in Ion somehow; I'll try to reproduce under rr tomorrow, but at this point I think wasm isn't the obvious culprit.
Weird, after a thousand runs under rr chaos mode, I don't get a single failure.

However, I had to modify the test case: when run in the revision from comment 0, I get the following error:
/tmp/test.js:85:17 TypeError: class constructors must be invoked with |new|

I commented the line which redefines the parseInt class, so as to make the test succeed.

Christian, did you have to update the test case as well? Or is the revision number in comment 0 incorrect?
Flags: needinfo?(bbouvier) → needinfo?(choller)
(cc'ing jit people, because I think the root cause is not in wasm, as explained in comment 5)
Reporter

Comment 8

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Weird, after a thousand runs under rr chaos mode, I don't get a single
> failure.
> 
> However, I had to modify the test case: when run in the revision from
> comment 0, I get the following error:
> /tmp/test.js:85:17 TypeError: class constructors must be invoked with |new|
> 

This is expected. The test redefines parseInt and that's fine, I got the error as well. But about 2 out of 1000 runs crashed on the EC2 machine I was using to reproduce this. The revision comes straight from the machine, so it should be correct, as is the test. I was not able to reproduce this on a newer revision on another machine unfortunately. I was also not able to reproduce this inside GDB (also not on the original machine). This error seems to be highly sensitive to all sorts of things.
Flags: needinfo?(choller)
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jdemooij)
Assignee

Comment 9

2 years ago
I managed to catch this in rr after a few thousand recordings. The problem seems to be that InitGlobalLexicalOperation uses setSlot instead of setSlotWithType, so initializing the slot does not properly update type information.
Keywords: sec-moderatesec-high
See Also: → 1373094
Is this a duplicate of bug 1373094? Are we waiting for it to be resolved to be sure?
Reporter

Comment 11

2 years ago
I doubt it because this test uses wasm while the other bug doesn't, but only Jan can confirm that for sure.
Assignee

Comment 12

2 years ago
Posted patch PatchSplinter Review
OK so this oneliner to use setSlotWithType instead of setSlot indeed fixes this locally - I get about 1 crash in 1,000-2,000 runs with rr chaos mode without the patch and with the patch it finished > 10,000 runs without crashing.

It's very hard to write a reliable testcase for this because type information has to be just right to both trigger the Ion optimization and hit the crash.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jdemooij)
Attachment #8884785 - Flags: review?(shu)
Assignee

Comment 13

2 years ago
This goes back years. It's definitely unrelated to bug 1373094.
sec-high, tracking for 55/56
Comment on attachment 8884785 [details] [diff] [review]
Patch

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

Sorry about the delay.
Attachment #8884785 - Flags: review?(shu) → review+
Assignee

Comment 16

2 years ago
Comment on attachment 8884785 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Very difficult. I was unable to write a testcase that fails reliably.

> 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?
All.

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

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely, very small/local change.
Attachment #8884785 - Flags: sec-approval?
Comment on attachment 8884785 [details] [diff] [review]
Patch

sec-approval+ for trunk.
We'll want a patch nominated for Beta and ESR52 so it can be landed after trunk.
Attachment #8884785 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/9315eabc7fb0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee

Comment 20

2 years ago
Comment on attachment 8884785 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Security issues, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet, bug is hard to repro.
[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?]: No.
[Why is the change risky/not risky?]: Very small/local/trivial fix.
[String changes made/needed]: None.
Attachment #8884785 - Flags: approval-mozilla-esr52?
Attachment #8884785 - Flags: approval-mozilla-beta?
Attachment #8884785 - Flags: approval-mozilla-esr52?
Attachment #8884785 - Flags: approval-mozilla-esr52+
Attachment #8884785 - Flags: approval-mozilla-beta?
Attachment #8884785 - Flags: approval-mozilla-beta+
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main55+][adv-esr52.3+]
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:][adv-main55+][adv-esr52.3+] → [jsbugmon:][adv-main55+][adv-esr52.3+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.