1595466 - Assertion failure: destOffset >= bytes, at js/src/wasm/WasmBaselineCompile.cpp:1845 or Assertion failure: srcOffset >= bytes, at js/src/wasm/WasmBaselineCompile.cpp:1810

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 72c52c0101cf (build with --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --no-threads test.js).

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::wasm::BaseStackFrame::shuffleStackResultsTowardSP (temp=..., bytes=8, destHeight=<optimized out>, srcHeight=24, this=0x7fffffffa548) at js/src/wasm/WasmBaselineCompile.cpp:1845
#1  js::wasm::BaseCompiler::popStackResults (this=this@entry=0x7fffffffa140, iter=..., stackBase=..., stackBase@entry=...) at js/src/wasm/WasmBaselineCompile.cpp:4100
#2  0x000055555664f18c in js::wasm::BaseCompiler::popBlockResults (kind=js::wasm::BaseCompiler::ContinuationKind::Fallthrough, stackBase=..., type=..., this=0x7fffffffa140) at js/src/wasm/WasmBaselineCompile.cpp:4161
#3  js::wasm::BaseCompiler::topBlockResults (this=this@entry=0x7fffffffa140, type=...) at js/src/wasm/WasmBaselineCompile.cpp:4243
#4  0x0000555556600bc7 in js::wasm::BaseCompiler::emitIf (this=this@entry=0x7fffffffa140) at js/src/wasm/WasmBaselineCompile.cpp:8645
#5  0x00005555566053be in js::wasm::BaseCompiler::emitBody (this=this@entry=0x7fffffffa140) at js/src/wasm/WasmBaselineCompile.cpp:11385
#6  0x000055555660a728 in js::wasm::BaseCompiler::emitFunction (this=0x7fffffffa140) at js/src/wasm/WasmBaselineCompile.cpp:12278
#7  js::wasm::BaselineCompileFunctions (env=..., lifo=..., inputs=..., code=code@entry=0x7ffff5f5f3a8, error=error@entry=0x7fffffffc5a8) at js/src/wasm/WasmBaselineCompile.cpp:12443
#8  0x00005555566a1af5 in ExecuteCompileTask (task=0x7ffff5f5f000, error=0x7fffffffc5a8) at js/src/wasm/WasmGenerator.cpp:743
#9  0x00005555566a71bc in js::wasm::ModuleGenerator::locallyCompileCurrentTask (this=0x7fffffffb770) at js/src/wasm/WasmGenerator.cpp:776
#10 js::wasm::ModuleGenerator::finishFuncDefs (this=this@entry=0x7fffffffb770) at js/src/wasm/WasmGenerator.cpp:914
#11 0x00005555565e2584 in DecodeCodeSection<js::wasm::Decoder> (env=..., d=..., mg=...) at js/src/wasm/WasmCompile.cpp:570
#12 0x00005555565e3136 in DecodeCodeSection<js::wasm::Decoder> (mg=..., d=..., env=...) at js/src/wasm/WasmCompile.cpp:547
#13 js::wasm::CompileBuffer (args=..., bytecode=..., error=error@entry=0x7fffffffc5a8, warnings=warnings@entry=0x7fffffffc610, listener=listener@entry=0x0) at js/src/wasm/WasmCompile.cpp:593
#14 0x00005555566dfa75 in js::WasmModuleObject::construct (cx=<optimized out>, cx@entry=0x7ffff5f27000, argc=<optimized out>, vp=<optimized out>) at js/src/wasm/WasmJS.cpp:1180
#15 0x00005555559cc7f0 in CallJSNative (cx=0x7ffff5f27000, native=native@entry=0x5555566df8a0 <js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*)>, reason=reason@entry=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:456
[...]
#28 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11535
rax	0x5555580b3fa0	93825037713312
rbx	0x555556fa6130	93825019830576
rcx	0x7ffff6c1c2dd	140737333281501
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffff9b60	140737488329568
rsp	0x7fffffff9aa0	140737488329376
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x0	0
r11	0x0	0
r12	0x7fffffffa140	140737488331072
r13	0x7fffffff9ba8	140737488329640
r14	0x18	24
r15	0x7fffffff9b90	140737488329616
rip	0x55555664eda9 <js::wasm::BaseCompiler::popStackResults(js::wasm::ABIResultIter&, js::wasm::StackHeight)+2377>
=> 0x55555664eda9 <js::wasm::BaseCompiler::popStackResults(js::wasm::ABIResultIter&, js::wasm::StackHeight)+2377>:	movl   $0x0,0x0
   0x55555664edb4 <js::wasm::BaseCompiler::popStackResults(js::wasm::ABIResultIter&, js::wasm::StackHeight)+2388>:	ud2

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

Andy, can you look?

Comment 3

[Andy Wingo [:wingo]]

On it, thanks.

Comment 4

[Andy Wingo [:wingo]]

Here's a fairly small reproducer; run with --wasm-compiler=baseline --no-threads:

var data = wasmTextToBinary(`
(module
  (func $main (export "main") (param i32) (result i32)
    i32.const 0
    local.get 0
    local.get 0
    local.get 0
    if (param i32 i32 i32) (result i32)
      unreachable
    end))
`);
module = new WebAssembly.Module(data.buffer);

Expected result would be a failure to validate.

Comment 5

[Andy Wingo [:wingo]]

Update: it fails to validate because the "topBlockResults" that happens when entering the if fails. It would fail to validate if the compile continued.

Comment 6

[Andy Wingo [:wingo]]

Just for the record, here's one that would validate but which also fails with the baseline compiler:

var data = wasmTextToBinary(`
(module
  (func $main (export "main") (param i32) (result i32)
    i32.const 0
    local.get 0
    local.get 0
    local.get 0
    if (param i32 i32 i32) (result i32)
      drop
      drop
    else
      drop
      drop
    end))
`);
module = new WebAssembly.Module(data.buffer);

Comment 7

[Andy Wingo [:wingo]]

An even better test!

var data = wasmTextToBinary(`
(module
  (func $main (export "main") (param i32) (result i32)
    i32.const 0
    local.get 0
    local.get 0
    loop (param i32 i32 i32) (result i32)
      drop
      drop
    end))
`);
module = new WebAssembly.Module(data.buffer);

Comment 8

[Andy Wingo [:wingo]]

Attachment: Bug 1595466 - Fix multi-value shuffle of stack values toward SP

This commit fixes a bug whereby shuffling of stack results toward the
stack pointer was borked. Just for posterity, here's a wee ASCII art of
how this works in the baseline compiler. The error came in the last
transition, when shuffling B toward the newly-expanded SP. The previous
code was simply bogus.

      initial        (i32.const A)
       stack    -->  (local.get B)    -> popRegisterResults -> popStackResults
       state         (local.get C)
            _
  |     | | |            | |                | |                   | |
  |     | | stack        | |                | |                   | |
  |     | | height       | |                | |                   | |
  |     | | |            | |                | |                   | |
  |   sp+-+ v            +-+                +-+                   +-+
  |                                         |B|                   |A|
  |                                         +-+                   +-+
  |                                                               |B|
  |                                                               +-+
  |
  V                 nothing pushed       C in %rax;         B shuffled toward
stack               on machine stack;    A still on         %rsp; A materialized
grows               3 values on value    value stack
down                stack

Comment 9

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

Please confirm this only affects Nightly else get a security rating - and security-approval if necessary - for this bug.

Comment 10

[Christian Holler (:decoder)]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #9)

Please confirm this only affects Nightly else get a security rating - and security-approval if necessary - for this bug.

A security rating should actually be assigned independent of the branches affected (also for Nightly-only issues). Thanks!

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/0f3a32ae9e57
user: Andy Wingo
date: Fri Nov 08 14:16:57 2019 +0000
summary: Bug 1586207 - WebAssembly validation allows multi-param and multi-result blocks r=lth

Guessing this is likely due to bug 1586207?

Comment 12

[Lars T Hansen [:lth]]

I can confirm that this fixes a recent Nightly-only (actually JS-shell-only) regression, see discussion on the patch if necessary. Per policy it should be fine to land it.

Comment 13

[Lars T Hansen [:lth]]

sec-moderate since, if this had been shipped in Firefox, and reference types had been turned on (Nightly only atm), then there would have been a theoretical chance of some data leakage as a pointer could have misrepresented to the compiler as an integer and thus an attacker might have obtained an address.

Comment 14

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

https://hg.mozilla.org/integration/autoland/rev/4424f41acdb04b53e7efcf8d64fc9dd1a2c53275

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4424f41acdb0