Closed Bug 1481171 Opened 2 years ago Closed 2 years ago

Wasm atomics must use standard bounds checking

Categories

(Core :: Javascript: WebAssembly, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(3 files, 2 obsolete files)

(Forked from bug 1432577)

Currently we do not emit metadata for atomic accesses, so when we get a segv in wasm code without metadata we "know" it was an atomic oob access.  This was acceptable because asm.js used the same mechanism for its simd and atomics, but those are gone now, and we should clean this up for wasm: atomics need to emit appropriate metadata for the first (temporally speaking) memory access that each atomic operation will do.

Since we are using native atomic operations for all atomic accesses on all platforms this should not be all that difficult.
See bug 1423577 comment 11.  Carrying Luke's f+, but note from that comment that more can be done here:

> I think you can delete LinkDataTier::outOfBoundsOffset too which will trigger another small transitive wave of removals.
Attachment #8997831 - Flags: feedback+
Note, atomic load and store appear already to work because the common wasmLoad / wasmStore infrastructure emits the correct metadata.
Depends on: 1343981
Attached patch bug1481171-wasm-atomics.patch (obsolete) — Splinter Review
For safekeeping, almost ready for review.  Need to rework the append() calls for ARM64, they should not use masm.size() but masm.currentOffset().  Every platform has a different mechanism here, sigh.
One note about this: On ARM64, this follows existing code in how it emits metadata for traps, namely, masm.append(access,masm.currentOffset()).  However, I suspect that that is incorrect, because currentOffset() calls nextOffset() which does *not* flush constant pools.  nextInstrOffset() does flush the pools though.  I will be investigating the matter immediately, but there's no particular reason to hold up this bug for that.
Attachment #8998513 - Attachment is obsolete: true
Attachment #8998811 - Flags: review?(luke)
Now with even more code removal.
Attachment #8997831 - Attachment is obsolete: true
Attachment #8998812 - Flags: review?(luke)
Blocks: resab
No longer blocks: 1444925
Duplicate of this bug: 1423496
(In reply to Lars T Hansen [:lth] from comment #4)
> Created attachment 8998811 [details] [diff] [review]
> bug1481171-wasm-atomics-v2.patch
> 
> One note about this: On ARM64, this follows existing code in how it emits
> metadata for traps, namely, masm.append(access,masm.currentOffset()).

I *thought* those cases were supposed all be guarded by AutoForbidPools; if they aren't, that seems like a bug.  I wasn't aware of nextInstrOffset(); that sounds better.  Either way, glad you'll be looking into next.
Comment on attachment 8998811 [details] [diff] [review]
bug1481171-wasm-atomics-v2.patch

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

Great!  Nice to see that passing around the MemoryAccessDesc actually tidies up a few places.
Attachment #8998811 - Flags: review?(luke) → review+
Comment on attachment 8998812 [details] [diff] [review]
bug1481171-rm-signal-handling-fallback-v2.patch

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

\o/
Attachment #8998812 - Flags: review?(luke) → review+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/413be91a0257
Wasm-specific atomics operations.  r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7bd965f86b
Remove failure path for lookupTrap().  r=luke
(In reply to Luke Wagner [:luke] from comment #7)
> (In reply to Lars T Hansen [:lth] from comment #4)
> > Created attachment 8998811 [details] [diff] [review]
> > bug1481171-wasm-atomics-v2.patch
> > 
> > One note about this: On ARM64, this follows existing code in how it emits
> > metadata for traps, namely, masm.append(access,masm.currentOffset()).
> 
> I *thought* those cases were supposed all be guarded by AutoForbidPools; if
> they aren't, that seems like a bug.  I wasn't aware of nextInstrOffset();
> that sounds better.  Either way, glad you'll be looking into next.

It's a bit of a mess, basically.  *One* instance uses AutoForbidPools in a reasonable way; *one* gets the instruction offset from the code emitter and append()s the metainfo afterward (ARM-style - I'm pretty sure I wrote this one because I tripped over a problem); existing wasmLoad and wasmStore just append() information before the instruction and hope for the best, as do the new atomic ops.

AutoForbidPools is a blunt instrument as one has to know a bit about the actual machine instructions emitted before it can be trusted; the MacroAssembler layers get in the way here.  But it looks doable to provide the necessary guarantees.
Keywords: leave-open
Attachment #8999135 - Flags: review?(luke)
Attachment #8999135 - Flags: review?(luke) → review+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9112406a54a2
Properly emit signal handling metadata for wasm on aarch64.  r=luke
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/9112406a54a2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1482932
Dragan, the first patch on this bug definitely affects MIPS, see the commit comments in the patch.
(In reply to Lars T Hansen [:lth] from comment #16)
> Dragan, the first patch on this bug definitely affects MIPS, see the commit
> comments in the patch.

Thanks for the heads up. Will look into it.
You need to log in before you can comment on or make changes to this bug.