Closed Bug 1595466 Opened 5 years ago Closed 5 years ago

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


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




Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- unaffected
firefox72 --- fixed


(Reporter: decoder, Assigned: wingo)




(4 keywords, Whiteboard: [post-critsmash-triage])


(2 files)

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).


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
Attached file Testcase

Andy, can you look?

Flags: needinfo?(wingo)
Priority: -- → P1

On it, thanks.

Assignee: nobody → wingo
Flags: needinfo?(wingo)

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

var data = wasmTextToBinary(`
  (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)
module = new WebAssembly.Module(data.buffer);

Expected result would be a failure to validate.

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

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

var data = wasmTextToBinary(`
  (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)
module = new WebAssembly.Module(data.buffer);

An even better test!

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

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

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

Flags: needinfo?(wingo)

(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!

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
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?

Regressed by: 1586207

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.

Flags: needinfo?(wingo)

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.

Keywords: sec-moderate
Group: javascript-core-security → core-security-release
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.