1250198 - [wasm] Crash [@ writeH] with Unaligned unsigned halfword write

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision e1cf617a1f28 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-simulator=arm --enable-debug, run with --fuzzing-safe --no-threads --ion-eager):

testStore('i32', '16', 268435455 >> 25 || (this), 0, 0, 0x2345);
function wasmEvalText(str, imports) {
    return wasmEval(wasmTextToBinary(str), imports);
}
function testStore(type, ext, base, offset, align, value) {
    assertEq(wasmEvalText(
        '(module' +
        '  (memory 0x10000' +
        '  )' +
        '  (func (param i32) (param ' + type + ') (result ' + type + ')' +
        '    (' + type + '.store' + ext +
        '     (get_local 0)' +
        '     (get_local 1)' +
        '    )' +
        '  ) (export "" 0))'
    )(base, value), value);
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x084e4e84 in writeH (this=0xf7a1c000, instr=0xf7c7b030, value=<optimized out>, addr=-139812857) at js/src/jit/arm/Simulator-arm.cpp:1617
#0  0x084e4e84 in writeH (this=0xf7a1c000, instr=0xf7c7b030, value=<optimized out>, addr=-139812857) at js/src/jit/arm/Simulator-arm.cpp:1617
#1  js::jit::Simulator::decodeType01 (this=0xf7a1c000, instr=0xf7c7b030) at js/src/jit/arm/Simulator-arm.cpp:2882
#2  0x084e17dd in js::jit::Simulator::instructionDecode (this=this@entry=0xf7a1c000, instr=instr@entry=0xf7c7b030) at js/src/jit/arm/Simulator-arm.cpp:4386
#3  0x084e5674 in execute<false> (this=0xf7a1c000) at js/src/jit/arm/Simulator-arm.cpp:4459
#4  js::jit::Simulator::callInternal (this=this@entry=0xf7a1c000, entry=entry@entry=0xf7c7b05c "\004\340-\345\360\037-\351\020\212-\355\001\240\240\341\001\253\212\342\374\263", <incomplete sequence \345>) at js/src/jit/arm/Simulator-arm.cpp:4547
#5  0x084e5ba0 in js::jit::Simulator::call (this=0xf7a1c000, entry=entry@entry=0xf7c7b05c "\004\340-\345\360\037-\351\020\212-\355\001\240\240\341\001\253\212\342\374\263", <incomplete sequence \345>, argument_count=argument_count@entry=2) at js/src/jit/arm/Simulator-arm.cpp:4630
#6  0x081c8c8a in js::wasm::Module::callExport (this=this@entry=0xf61d5b80, cx=cx@entry=0xf7a72020, exportIndex=0, args=...) at js/src/asmjs/WasmModule.cpp:1337
#7  0x081c945b in WasmCall (cx=0xf7a72020, argc=2, vp=0xffffb1c0) at js/src/asmjs/WasmModule.cpp:1088
#8  0x086a1c2a in js::CallJSNative (cx=0xf7a72020, native=0x81c93a0 <WasmCall(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#41 main (argc=5, argv=0xffffcba4, envp=0xffffcbbc) at js/src/shell/js.cpp:7120
eax	0x0	0
ebx	0x98496d4	159684308
ecx	0xf7e3b88c	-136071028
edx	0x0	0
esi	0xf7aaa007	-139812857
edi	0xb	11
ebp	0xffffab08	4294945544
esp	0xffffaa90	4294945424
eip	0x84e4e84 <js::jit::Simulator::decodeType01(js::jit::SimInstruction*)+5668>
=> 0x84e4e84 <js::jit::Simulator::decodeType01(js::jit::SimInstruction*)+5668>:	movl   $0x651,0x0
   0x84e4e8e <js::jit::Simulator::decodeType01(js::jit::SimInstruction*)+5678>:	call   0x80fada0 <abort()>

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Probably worth interesting Dan who implemented loads/stores. I can have a look this week otherwise.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

This is just an unaligned store: (i32.store16 (i32.const 1) (i32.const 0x2345)). I see that these are NYI in the basic-memory.js tests, so that might be expected to crash here. We need a mitigation here.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

assertEq(wasmEvalText(`
    (module
      (memory 0x10000)
      (func (result i32)
        (i32.store16
         (i32.const 1)
         (i32.const 0x2345)
        )
      ) (export "" 0))
`)(), 0x2345);

Fuzzers might run into this until we implement unaligned loads/stores. Possible ways to move forward:
- implement dynamic checks to ensure the index is aligned (this is my least preferred; it's high cost for a workaround)
- shift the base in WasmIonCompile when we emit the opcode, for wasm only (so as not to penalize asm.js)
- implement unaligned memory accesses :-)

Any preference, Dan?

Comment 4

[Dan Gohman [:sunfish]]

asm.js doesn't ever generate unaligned accesses, so I think my preference would be to just make DecodeLoadStoreAddress in Wasm.cpp reject non-natural alignments.

Comment 5

[Dan Gohman [:sunfish]]

Ok, that doesn't actually work because code can do misaligned accesses where the declared alignment is natural, but the dynamic access is misaligned.

Implementing misaligned accesses is non-trivial because without dynamic checks, we'll need a signal handler. I think I'm leaning towards masking base addresses in wasm, as a temporary measure, until we teach the backend more about wasm-style loads and stores.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: masklowbits.patch

Masking the low bits, as suggested. It adds a new parameter to EmitLoadStoreAddress, but this should be temporary.

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 8724735 [details] [diff] [review]
masklowbits.patch

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

Looks good.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/758c1524bbd4

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/758c1524bbd4