1699647 - Assertion failure: size_t(reg) < std::size(names), at /storagejs/src/jit/x86-shared/Constants-x86-shared.h:194

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision a96c49a026f6 (build with --target=i686-pc-linux-gnu --enable-gczeal --enable-optimize --enable-debug --enable-fuzzing, run with --no-threads --fuzzing-safe --baseline-warmup-threshold=0 --disable-oom-functions test.js).

Backtrace:

==29603==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x00000000 (pc 0x58a3fffc bp 0xffd59f98 sp 0xffd59f40 T29603)
    #0 0x58a3fffb in AnnotateMozCrashReason(char const*) js/src/debug32fuzzing/dist/include/mozilla/Assertions.h:42:19
    #1 0x58a3fffb in js::jit::X86Encoding::GPReg8Name(js::jit::X86Encoding::RegisterID) js/src/jit/x86-shared/Constants-x86-shared.h:194:3
    #2 0x58a3fffb in js::jit::X86Encoding::BaseAssembler::setCC_r(js::jit::X86Encoding::Condition, js::jit::X86Encoding::RegisterID) js/src/jit/x86-shared/BaseAssembler-x86-shared.h:2034:41
    #3 0x58a3fffb in js::jit::AssemblerX86Shared::setCC(js::jit::AssemblerX86Shared::Condition, js::jit::Register) js/src/jit/x86-shared/Assembler-x86-shared.h:1208:10
    #4 0x58a3fffb in js::jit::MacroAssembler::allTrueInt16x8(js::jit::FloatRegister, js::jit::Register) js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h:1407:3
    #5 0x58a3f5bb in js::jit::CodeGenerator::visitWasmReduceSimd128(js::jit::LWasmReduceSimd128*) js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp:3448:12
    #6 0x58cdaf3b in js::jit::CodeGenerator::generateBody() js/src/jit/CodeGenerator.cpp:6571:9
    #7 0x58d33b82 in js::jit::CodeGenerator::generateWasm(js::wasm::TypeIdDesc, js::wasm::BytecodeOffset, js::wasm::ArgTypeVector const&, js::jit::MachineState const&, unsigned int, js::wasm::FuncOffsets*, js::wasm::StackMaps*) js/src/jit/CodeGenerator.cpp:11359:8
    #8 0x59234ea4 in js::wasm::IonCompileFunctions(js::wasm::ModuleEnvironment const&, js::wasm::CompilerEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8u, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmIonCompile.cpp:5659:20
    #9 0x591f114b in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmGenerator.cpp:782:16
    #10 0x591f2c1c in js::wasm::ModuleGenerator::locallyCompileCurrentTask() js/src/wasm/WasmGenerator.cpp:831:8
    #11 0x591f2c1c in js::wasm::ModuleGenerator::finishFuncDefs() js/src/wasm/WasmGenerator.cpp:969:24
    #12 0x5912d858 in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:678:13
    #13 0x5912d2b1 in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0u, js::SystemAllocPolicy>*, JS::OptimizedEncodingListener*) js/src/wasm/WasmCompile.cpp:700:8
    #14 0x5926d4cf in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1598:7
    #15 0x57db361a in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) js/src/vm/Interpreter.cpp:435:13
    #16 0x57db77e9 in CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:451:8
    #17 0x57da79be in InternalConstruct(JSContext*, js::AnyConstructArgs const&) js/src/vm/Interpreter.cpp:624:14
    #18 0x57da75d6 in js::ConstructFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:670:10
    #19 0x588cba98 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:1821:10
    #20 0x326745b9  (<unknown module>)

This is x86 (32-bit) only and blocking further fuzzing.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

Sigh. setcc requires a register with an 8-bit personality on x86. Elsewhere I think we've punted on this by just choosing a fixed register such as eax. Yury, perhaps you can take a look?

Comment 3

[Yury Delendik (:yury)]

Compact test with pre-built wasm binary:

const module = new Uint8Array([
    0,97,115,109,1,0,0,0,1,4,1,96,0,0,3,2,1,0,
    5,4,1,1,0,0,7,10,1,6,109,101,109,111,114,121,2,0,10,49,
    1,47,0,65,16,253,1,1,1,253,253,1,253,131,1,65,158,232,
    68,253,1,1,1,253,100,65,16,253,1,1,1,253,11,0,0,253,1,1,
    1,65,158,232,68,253,1,1,0,0,11,0,14,4,110,97,109,101,1,
    7,1,0,4,109,97,105,110]);
new WebAssembly.Module(module)

Comment 4

[Julian Seward [:jseward]]

If you don't want to choose a fixed reg (eg eax), on 32-bit only, another kludgey
fix is to accept any register (that is, one of eax/ebx/ecx/edx/esi/edi). If it is
one of the first 4, then just use setcc as at present. If esi or edi, emit
swapl eax, reg ; setcc eax ; swapl eax, reg.
swapl eax, reg has a one-byte encoding in 32-bit mode, so it's only 2 bytes extra
code in total.

Comment 5

[Yury Delendik (:yury)]

Attachment: Bug 1699647 - Use setCC only when register has 8bit personality. r?lth

Comment 6

[Yury Delendik (:yury)]

Cool, for now I used the same logic as in emitSet. Let me know if swapl one is better.

Comment 7

[Lars T Hansen [:lth]]

Nightly-only code. Should be good to land after the freeze ends. Yury, it would be good if you could get it uplifted to Beta 88 ASAP so that it won't bite Bergamot.

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 9210362 [details]
Bug 1699647 - Use setCC only when register has 8bit personality. r?lth

Beta/Release Uplift Approval Request

• User impact if declined: Web application (e.g. Google Hangout), that use WebAssembly SIMD may crash.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: Test included
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): SIMD is new technology in WebAssembly but used in some apps now there is a small risk that similar crash can happen in the wild now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b683eaeb2a74d454af062bbe2f3c256bf5ca2cd3

• String changes made/needed:

Comment 9

[Yury Delendik (:yury)]

Limited test to the affected platforms via // |jit-test| skip-if: !wasmSimdEnabled() || !getBuildConfiguration().x86

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f00e6bc1bf83daed51797ca81077438e8a72f7

Comment 10

[Yury Delendik (:yury)]

Attachment: Bug 1699647 - Use setCC only when register has 8bit personality. r=lth

Comment 11

[Yury Delendik (:yury)]

Comment on attachment 9211040 [details]
Bug 1699647 - Use setCC only when register has 8bit personality. r=lth

Beta/Release Uplift Approval Request

• User impact if declined: Web application (e.g. Google Hangout), that use WebAssembly SIMD may crash.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: jit-test for affected platforms included
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): SIMD is new technology in WebAssembly but used in some apps now there is a small risk that similar crash can happen in the wild now.
• String changes made/needed: np

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Use setCC only when register has 8bit personality. r=lth
https://hg.mozilla.org/integration/autoland/rev/7b8bf5ab83a80ec1d60d9b570c4b3e27ed6442b1
Added lines at devs request r=fix CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/e60b4bcde4298d442e7e5be7b7a7c5bf7c742807

https://hg.mozilla.org/mozilla-central/rev/7b8bf5ab83a8
https://hg.mozilla.org/mozilla-central/rev/e60b4bcde429

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9211040 [details]
Bug 1699647 - Use setCC only when register has 8bit personality. r=lth

Approved for 88.0b3.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12a9c01cf2f2