1684861 - Baseline compiler does not take offset into account for atomic wait or notify. (was: Different error messages with WebAssembly Memory and ion vs. baseline)

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Christian Holler (:decoder)]

The attached testcase produces different error messages on mozilla-central revision 20210103-89fef9703703 (build with --enable-debug, run with either --no-threads --wasm-compiler=baseline test.js or --no-threads --wasm-compiler=ion test.js).

debug64/dist/bin/js --no-threads --wasm-compiler=baseline test.js
test.js line 10 > WebAssembly.Module:43:1 RuntimeError: index out of bounds

vs.

debug64/dist/bin/js --no-threads --wasm-compiler=ion test.js
test.js line 10 > WebAssembly.Module:43:1 RuntimeError: unaligned memory access

Marking this s-s until :lth had time to investigate. We should be able to ignore this in the fuzzer if it turns out to be not an issue.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

Yeah, this is weird. The problem is a call to the wake() operation, and ion and baseline receive a different value here, which is why we see different error messages. But it's not clear yet why the values are different.

Comment 3

[Lars T Hansen [:lth]]

(module
  (type (;0;) (func))
  (func $main (type 0)
    i32.const -64
    i32.const -63
    memory.atomic.notify offset=1
    unreachable)
  (memory (;0;) 4 4)
  (export "main" (func $main)))

The bug is that the baseline compiler forgets to fold the offset into the address (-64). If it did, the effective address would be -65 which would give rise to the "unaligned memory access" error. As it is, the address is aligned when it comes from the baseline compiler and thus we fall through to the next check, which is a bounds check. ("index out of bounds" is a bit generic frankly.)

This is not a security problem because everything is properly bounds checked, we just don't compute the correct address. So we should open up. I'll retitle.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Unhiding per comment 3.

Comment 5

[Yury Delendik (:yury)]

Attachment: Bug 1684861 - wasm: add address folding for atomic wake/wait.

Comment 6

[Yury Delendik (:yury)]

So we have issue with Aarch64 backend too: https://treeherder.mozilla.org/logviewer?job_id=326004002&repo=try&lineNumber=11267

It is pretty much the same issue as in the baseline, but in the cranelift code: https://github.com/bytecodealliance/wasmtime/blob/4bee07d6f9651a5069463cce05f5036a3c99e468/cranelift/wasm/src/code_translator.rs#L1041-L1076

I sketched the solution https://github.com/yurydelendik/wasmtime/commit/9b96787267382b17f2675d138c99b94fa6459b57

Julian, can you look at it?

Comment 7

[Yury Delendik (:yury)]

There might be a need to redesign API for SymbolicAddress::Wake and co. functions to accept final and valid pointer instead of (memory +) 32bit-offset. Do we want to refactor these instead of patching only baseline compiler?

Comment 8

[Yury Delendik (:yury)]

Related patch for wasmtime repo https://github.com/bytecodealliance/wasmtime/pull/2556

Comment 9

[Lars T Hansen [:lth]]

Commented on the patch. I think the API is fine, we just need to generate simpler code in baseline, that folds the offset and does nothing else.

Comment 10

[Lars T Hansen [:lth]]

Sorry, did not mean to clear ni? for Julian!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf1b2cc8b927
wasm: add address folding for atomic wake/wait. r=lth

Comment 12

[Yury Delendik (:yury)]

Few potential TODOs:

• "... Positive tests of wait and notify that use a nonzero offset. [...] Basically for wait, it should be enough to have a value already in a location that we wait for so that the wait does not block. Notify is trickier, we may need to set something up with shell workers. Something in js/src/tests/shell/futex.js may serve as an inspiration."

• Cranelift update (for https://github.com/bytecodealliance/wasmtime/pull/2556) and enable the test

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bf1b2cc8b927

Comment 14

[Julian Seward [:jseward]]

The Cranelift components of this have been fixed upstream in
https://github.com/bytecodealliance/wasmtime/pull/2556/.
The fix has been pulled into nightly by the CL rebase in bug 1689950,
which is also now closed.

Comment 15

[Julian Seward [:jseward]]

Yury, have all the non-CL components of this landed now? If so can we close?

Comment 16

[Julian Seward [:jseward]]

If so can we close?

Uh, duh. It is closed already. Ignore ..

Comment 17

[Yury Delendik (:yury)]

Julian, we need to enable testing with js/src/jit-test/tests/wasm/regress/bug1684861.js though. These tests shall work with CL compiler.