1681269 - Hit MOZ_CRASH(NYI) at wasm/WasmIonCompile.cpp:2514 in [@ EmitTry]

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20201208-1a164819adef (debug build, run with --no-threads --fuzzing-safe --baseline-warmup-threshold=0 --disable-oom-functions test.js).

Backtrace:

==2660==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55e4b6100a9b bp 0x7ffe26521ef0 sp 0x7ffe26521ec0 T2660)
==2660==The signal is caused by a WRITE memory access.
==2660==Hint: address points to the zero page.
    #0 0x55e4b6100a9b in EmitTry((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:2514:3
    #1 0x55e4b60f4b5f in EmitBodyExprs((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:4409:9
    #2 0x55e4b60efed6 in js::wasm::IonCompileFunctions(js::wasm::ModuleEnvironment const&, js::wasm::CompilerEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmIonCompile.cpp:5483:12
    #3 0x55e4b60d492c in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmGenerator.cpp:757:16
    #4 0x55e4b60d644b in locallyCompileCurrentTask js/src/wasm/WasmGenerator.cpp:819:8
    #5 0x55e4b60d644b in js::wasm::ModuleGenerator::finishFuncDefs() js/src/wasm/WasmGenerator.cpp:957:24
    #6 0x55e4b602bc6a in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:569:13
    #7 0x55e4b602b6f3 in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>*, JS::OptimizedEncodingListener*, JSTelemetrySender) js/src/wasm/WasmCompile.cpp:592:8
    #8 0x55e4b6169576 in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1704:25
    #9 0x55e4b51dbd81 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) js/src/vm/Interpreter.cpp:503:13
    #10 0x55e4b51eb5c9 in CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:519:8
    #11 0x55e4b51dd1dd in InternalConstruct(JSContext*, js::AnyConstructArgs const&) js/src/vm/Interpreter.cpp:691:14
    #12 0x55e4b5baed49 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:1896:10
    #13 0x117bc070d872  (<unknown module>)

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

This needs to be fixed ASAP, and most definitely before we branch for 85 next week. The instruction decoders must never crash - when an instruction is recognized but not supported, it must always throw an unrecognized opcode error. The correct way to do this is shown eg for Op::SimdPrefix in WasmBaselineCompile.cpp. WasmIonCompile.cpp and WasmBaselineCompile.cpp are both affected.

(The error should reproduce easily on any branch with any settings if the test case uses one of the exception opcodes.)

Comment 3

[Lars T Hansen [:lth]]

I guess if exceptions are disabled completely on non-nightly it's not crucial that we fix this before we branch. But we should fix it ASAP anyway.

Comment 4

[Asumu Takikawa]

Attachment: Bug 1681269 - Fix exception instruction decoders crash.

Emit unrecognized opcode for unimplemented exception instructions.

Comment 5

[Asumu Takikawa]

(In reply to Lars T Hansen [:lth] from comment #2)

This needs to be fixed ASAP, and most definitely before we branch for 85 next week. The instruction decoders must never crash - when an instruction is recognized but not supported, it must always throw an unrecognized opcode error.

Sorry for the breakage, I've attached a patch now that adds an exception flag in FeatureArgs (for now always set to false) and emits unrecognized opcode if the flag isn't set (via ModuleEnvironment).

Should I add you as reviewer Lars, or add :rhunt as with the other patches?

Comment 6

[Lars T Hansen [:lth]]

Thanks! You may as well ask Ryan; I'll just steal the review if I have time before he gets to it.

(I know for a fact we have test cases for unrecognized opcodes in wasm/binary.js already, and I wonder why those test cases did not catch this problem. Probably they only run the validator, they don't try to compile.)

Comment 7

[Lars T Hansen [:lth]]

Actually, I'll take a look myself.

In re my comment:

(I know for a fact we have test cases for unrecognized opcodes in wasm/binary.js already, and I wonder why those test cases did not catch this problem. Probably they only run the validator, they don't try to compile.)

I see that this is because wasm-binary.js was changed to allow the exception opcodes... That's not totally desirable. Consider this instead:

diff --git a/js/src/jit-test/lib/wasm-binary.js b/js/src/jit-test/lib/wasm-binary.js
--- a/js/src/jit-test/lib/wasm-binary.js
+++ b/js/src/jit-test/lib/wasm-binary.js
@@ -145,7 +145,7 @@ const MozPrefix = 0xff;
 
 const definedOpcodes =
     [0x00, 0x01, 0x02, 0x03, 0x04, 0x05,
-     0x06, 0x07, 0x08, 0x09, 0x0a, // Exception handling opcodes
+     ...(wasmExceptionsEnabled() ? [0x06, 0x07, 0x08, 0x09, 0x0a] : []),
      0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
      0x10, 0x11,
      0x1a, 0x1b, 0x1c,

With this patch, the wasmEval test in wasm/binary.js (line 233) passes with and without --wasm-exceptions, but the subsequent validate test (line 234) fails without that flag, because the validator unconditionally accepts these opcodes. It should not, so the patch must be fixed to handle that.

I'll comment more on the patch.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91568b766813
Fix exception instruction decoders crash. r=lth

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/91568b766813