Closed Bug 1368362 Opened 4 years ago Closed 4 years ago
Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/Macro
Assembler .cpp:1695 with wasm
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.
Setting NI for Benjamin since this seems related to WebAssembly.
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
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)
(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.
4 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.
Is this a duplicate of bug 1373094? Are we waiting for it to be resolved to be sure?
I doubt it because this test uses wasm while the other bug doesn't, but only Jan can confirm that for sure.
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
Attachment #8884785 - Flags: review?(shu)
This goes back years. It's definitely unrelated to bug 1373094.
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+
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+
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.
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main55+][adv-esr52.3+]
Whiteboard: [jsbugmon:][adv-main55+][adv-esr52.3+] → [jsbugmon:][adv-main55+][adv-esr52.3+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.