1645414 - Assertion failure: prev->isWasmCall() || prev->isIonToWasmCall(), at js/src/jit/InstructionReordering.cpp:193

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200612-fea1e502ea28 (debug build, run with --fuzzing-safe --no-threads test.js):

See attachment.

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555556665369 in js::jit::ReorderInstructions(js::jit::MIRGraph&) ()
#0  0x0000555556665369 in js::jit::ReorderInstructions(js::jit::MIRGraph&) ()
#1  0x000055555666c93a in js::jit::OptimizeMIR(js::jit::MIRGenerator*) ()
#2  0x000055555692849e in js::wasm::IonCompileFunctions(js::wasm::ModuleEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*) ()
#3  0x000055555691365d in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) ()
#4  0x0000555556914457 in js::wasm::ModuleGenerator::finishFuncDefs() ()
#5  0x000055555689e2ae in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) ()
#6  0x000055555689de49 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*) ()
#7  0x000055555697c8f4 in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) ()
#8  0x0000555555942e92 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#20 0x00005555557b9d6d in main ()
rax	0x55555714783c	93825021540412
rbx	0x7ffff5799aa8	140737311775400
rcx	0x5555583d8980	93825041009024
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffff8650	140737488324176
rsp	0x7fffffff8540	140737488323904
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9bd40	140737353727296
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0x7ffff579a678	140737311778424
r13	0x7fffffff89d0	140737488325072
r14	0x7ffff579a4b8	140737311777976
r15	0x1d	29
rip	0x555556665369 <js::jit::ReorderInstructions(js::jit::MIRGraph&)+2793>
=> 0x555556665369 <_ZN2js3jit19ReorderInstructionsERNS0_8MIRGraphE+2793>:	movl   $0xc1,0x0
   0x555556665374 <_ZN2js3jit19ReorderInstructionsERNS0_8MIRGraphE+2804>:	callq  0x5555558494ce <abort>

Marking s-s because the impact of this JIT assertion is unclear until triaged.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200612145655-9388507a8b1f.
The bug appears to have been introduced in the following build range:
> Start: 6331429d856033aaf757ebf2863ee36ae2b8a29d (20200407094413)
> End: d850c01b531f5ec42d195cfb4ef92fcdb272c2d5 (20200407101236)
> Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6331429d856033aaf757ebf2863ee36ae2b8a29d&tochange=d850c01b531f5ec42d195cfb4ef92fcdb272c2d5

Comment 3

[Lars T Hansen [:lth]]

Severity unknown at this time, leaving it as "critical".

Comment 4

[Lars T Hansen [:lth]]

Multi-value is implicated:

(module
  (type (;0;) (func (result i32 i32 i32)))
  (func $main (type 0) (result i32 i32 i32)
    call $main
    call $main
    br_if 0 (;@0;)
    i32.shr_s
    call $main
    i32.shr_u
    call $main
    i32.ctz
    nop
    drop
    nop
    drop
    i32.shr_s
    table.get 0
    unreachable
    i32.shr_s)
  (table (;0;) 8 255 externref)
  (export "t1" (table 0)))

Comment 5

[Lars T Hansen [:lth]]

Attachment: test.js

Non-binary test case, not yet minimized.

Comment 6

[Lars T Hansen [:lth]]

Attachment: test.js

Smaller. Additionally the first call $main can be replaced by i32.const 0 and the shr_r can be add, but I don't know if that clarifies anything.

Comment 7

[Andy Wingo [:wingo]]

Setting ni? to myself; I have repro'd and will take a look this afternoon.

Comment 8

[Andy Wingo [:wingo]]

Fix is in bug 1645820. Pretty sure it's s-s so not mentioning details or a test case in that patch.

Comment 9

[Andy Wingo [:wingo]]

For context, see bug 1609887.

Comment 10

[Andy Wingo [:wingo]]

Attachment: bug1645414.patch

Hello release managers, I would like to request this patch for uplift. It combines the patches for bug 1645819 and bug 1645820.

[Feature/Bug causing the regression]:
Regressed by enabling the WebAssembly multi-value feature: bug 1625927

[User impact if declined]:
Information leak or possibly arbitrary code execution

[Is this code covered by automated tests?]:
The bug was found while fuzzing. There is a test case attached to the bug but which hasn't been applied yet due to security reasons.

[Has the fix been verified in Nightly?]:
Not formally. Patch landed though and I verified the fix locally.

[Needs manual test from QE? If yes, steps to reproduce]:
I don't think so -- fix is verified on nightly and the code hasn't changed in beta. Otherwise if you want to, build dist/bin/js with debugging assertions enabled, and run it on the test.js attached to this bug. If it completes without segfaulting and printing an assertion, we're all good.

[Why is the change risky/not risky?]:
It's a localized fix that only affects compilation of WebAssembly functions that return multiple values; this feature was not in earlier Firefox versions.

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9156917 [details] [diff] [review]
bug1645414.patch

I'll flag each of the bugs, seems easier that way.

Comment 12

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200616154959-89a54069f124.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.