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)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: gkw, Assigned: lth)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(5 files)

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 ).
Run:

./js --fuzzing-safe --no-baseline --no-ion --test-wasm-await-tier2 w2-out.wrapper w2-out.wasm
This seems 32-bit only.
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)
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)
No longer blocks: 1486027
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
Component: JavaScript Engine → Javascript: Web Assembly
What sec rating should this have?
Flags: needinfo?(lhansen)
Do sec-high for now, it's hard to tell how it can be abused (if at all).
Flags: needinfo?(lhansen)
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.
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.
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)
Group: javascript-core-security
Flags: needinfo?(ryanvm)
Keywords: sec-high
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+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33c57870ca18
Make an overbroad assertion narrower. r=jseward
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 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+
https://hg.mozilla.org/mozilla-central/rev/33c57870ca18
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: