Closed
Bug 1516720
Opened 5 years ago
Closed 5 years ago
Assertion failure: byteRegs.has(r), at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1013
Categories
(Core :: JavaScript: WebAssembly, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla66
People
(Reporter: gkw, Assigned: lth)
Details
(4 keywords, Whiteboard: [jsbugmon:update])
Attachments
(5 files)
26.16 KB,
text/plain
|
Details | |
199.86 KB,
application/octet-stream
|
Details | |
9.00 MB,
application/octet-stream
|
Details | |
4.19 MB,
application/octet-stream
|
Details | |
1.46 KB,
patch
|
jseward
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-beta revision 0e5689df129b (build with --32 --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-baseline --no-ion --test-wasm-await-tier2 w2-out.wrapper): See attachment. Backtrace: #0 CheckBytereg (r=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1013 #1 0x56e64595 in CheckBytereg (r=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1172 #2 AtomicFetchOp<js::jit::Address, js::jit::Register> (masm=..., access=0xf5d048f8, arrayType=js::Scalar::Uint8, op=js::jit::AtomicFetchXorOp, value=..., mem=..., temp=..., output=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1177 #3 0x56e64a55 in js::jit::MacroAssembler::wasmAtomicFetchOp (this=<optimized out>, access=..., op=<optimized out>, value=..., mem=..., temp=..., output=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1282 #4 0x57153163 in js::wasm::BaseCompiler::atomicRMW32<js::jit::Address> (temps=..., rd=..., rv=..., op=js::jit::AtomicFetchXorOp, srcAddr=..., access=..., this=0xf5d04ad4) at js/src/wasm/WasmBaselineCompile.cpp:4936 /snip For detailed crash information, see attachment. Setting s-s as I'm not sure how bad this is yet. This is the first result of hooking jsfunfuzz together with the binaryen fuzzer (see https://github.com/WebAssembly/binaryen/wiki/Fuzzing ).
Reporter | ||
Comment 1•5 years ago
|
||
Reporter | ||
Comment 2•5 years ago
|
||
Reporter | ||
Comment 3•5 years ago
|
||
Reporter | ||
Comment 4•5 years ago
|
||
Reporter | ||
Comment 5•5 years ago
|
||
Run: ./js --fuzzing-safe --no-baseline --no-ion --test-wasm-await-tier2 w2-out.wrapper w2-out.wasm
Reporter | ||
Comment 6•5 years ago
|
||
This seems 32-bit only.
Reporter | ||
Comment 7•5 years ago
|
||
autobisectjs shows this is probably related to the following changeset: The first bad revision is: changeset: b48d2197a9a0 user: Luke Wagner date: Fri Aug 24 13:41:13 2018 -0500 summary: Bug 1486027 - don't oom on failed ModuleGenerator memory reservations (r=bbouvier) Luke, is bug 1486027 a likely regressor?
Blocks: 1486027
Flags: needinfo?(luke)
Comment 8•5 years ago
|
||
Rather unlikely; before bug 1486027 the testcase's failure would be masked by an OOM failure. OOM in wasm baseline, so perhaps Lars could take a look when he's back from PTO.
Flags: needinfo?(luke)
Assignee | ||
Comment 9•5 years ago
|
||
Same problem as bug 1516738 really, failing to properly use a register with a byte personality. This one is a little more curious because I thought I had handled it properly, but I guess not...
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Updated•5 years ago
|
Component: JavaScript Engine → Javascript: Web Assembly
Comment 10•5 years ago
|
||
What sec rating should this have?
status-firefox64:
--- → wontfix
status-firefox66:
--- → affected
status-firefox-esr60:
--- → affected
Flags: needinfo?(lhansen)
Assignee | ||
Comment 11•5 years ago
|
||
Do sec-high for now, it's hard to tell how it can be abused (if at all).
Flags: needinfo?(lhansen)
Assignee | ||
Comment 12•5 years ago
|
||
The reason this is failing is that I remembered to add byte register support to cmpxchg and xchg, but not to the rmw operations. Go figure... This is probably not sec-high because we don't yet ship atomics on-by-default, but I'd still like to uplift the eventual fix to beta so that there will be no surprises even for users that turn atomics on via about:config.
Assignee | ||
Comment 13•5 years ago
|
||
Repro instructions in full should be: cat the .001 and .002 files together into w2-out-wasm.zip unzip that file to produce w2-out.wasm append w2-out.wasm as an additional command line argument when running the tests it's sufficient to run with --no-wasm-ion I'm having a hard time producing a reduced test case so far. Anyway, I think this is not a real bug - I think it's an incorrect MOZ_ASSERT in the macroassembler, there's no reason why the register in question need be a byte register for the bitwise operations.
Assignee | ||
Comment 14•5 years ago
|
||
Move the assertion to where it belongs, see commit comment for more info. Ryan, can you open this bug? It's not s-s at all. (We maybe should uplift to beta just because it reduces the risk of false test failures; will ponder.)
Flags: needinfo?(ryanvm)
Attachment #9033922 -
Flags: review?(jseward)
Comment 15•5 years ago
|
||
Comment on attachment 9033922 [details] [diff] [review] bug1516720-move-overbroad-assertion.patch Review of attachment 9033922 [details] [diff] [review]: ----------------------------------------------------------------- Loosk OK.
Attachment #9033922 -
Flags: review?(jseward) → review+
Comment 16•5 years ago
|
||
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/33c57870ca18 Make an overbroad assertion narrower. r=jseward
Assignee | ||
Comment 17•5 years ago
|
||
Comment on attachment 9033922 [details] [diff] [review] bug1516720-move-overbroad-assertion.patch [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: unknown User impact if declined: No user impact. We may run into this during testing however. Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): It restricts the scope of an assertion, making it less aggressive. Also, the assertion is only active in debug builds. String changes made/needed:
Attachment #9033922 -
Flags: approval-mozilla-beta?
Comment 18•5 years ago
|
||
Comment on attachment 9033922 [details] [diff] [review] bug1516720-move-overbroad-assertion.patch [Triage Comment] Fixes an overly-aggressive assertion. Approved for 65.0b8.
Attachment #9033922 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/35ba72605a9a
Flags: in-testsuite-
Comment 20•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/33c57870ca18
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Updated•5 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•