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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Christian Holler (:decoder)]

Setting NI for Benjamin since this seems related to WebAssembly.

Comment 3

[Fuzzing Team]

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

Comment 4

[Fuzzing Team]

JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(cc'ing jit people, because I think the root cause is not in wasm, as explained in comment 5)

Comment 8

[Christian Holler (:decoder)]

(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.

Comment 9

[Jan de Mooij [:jandem]]

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.

Comment 10

[Frederik Braun [:freddy]]

Is this a duplicate of bug 1373094? Are we waiting for it to be resolved to be sure?

Comment 11

[Christian Holler (:decoder)]

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

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 13

[Jan de Mooij [:jandem]]

This goes back years. It's definitely unrelated to bug 1373094.

Comment 14

[Julien Cristau [:jcristau]]

sec-high, tracking for 55/56

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8884785 [details] [diff] [review]
Patch

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

Sorry about the delay.

Comment 16

[Jan de Mooij [:jandem]]

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.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9315eabc7fb0301e772359505bff5704fe894b99

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9315eabc7fb0

Comment 20

[Jan de Mooij [:jandem]]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6223be841811
https://hg.mozilla.org/releases/mozilla-esr52/rev/f0ab032fd674