2042287 - memory.init on a dropped/active data segment fails to trap on out-of-bounds dst when len == 0

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Summary

memory.init with len == 0 on a dropped or active data segment fails to trap when the destination offset is out of bounds. Per the WebAssembly spec the dst check (d + n > |mem|) applies regardless of n; SpiderMonkey's dropped-segment fast path returns success without checking it.

Case (mem size 0x10000, args dst=0x10001, src=0, len=0)	Expected	SpiderMonkey	V8	JSC
non-dropped passive segment	trap	trap	trap	trap
dropped passive (data.drop then memory.init)	trap	no trap	trap	trap
active segment (auto-dropped after instantiation)	trap	no trap	trap	trap
dropped + in-bounds dst (dst=0, src=0, len=0) — PR #123 case	no trap	no trap	no trap	no trap

SpiderMonkey is the only engine that fails to trap on the OOB-dst variant.

Root cause

The bulk-memory spec algorithm for memory.init is:

1. If s + n > len(data), trap.
2. If d + n > len(mem), trap. ← still applies when n == 0
3. If n == 0, return.

bulk-memory PR #123 clarified that a dropped/active segment behaves as an empty data sequence, so step 1 stops trapping for s = 0, n = 0. Step 2 was untouched. js/src/wasm/WasmInstance.cpp:826-837 collapses both steps into one shortcut:

if (!maybeSeg) {
  if (len == 0 && srcOffset == 0) {
    return 0;            // returns without inspecting dstOffset
  }
  ReportTrapError(cx, JSMSG_WASM_OUT_OF_BOUNDS);
  return -1;
}

const DataSegment& seg = *maybeSeg;
...
if (BoundsCheckInit(dstOffset, srcOffset, len, memLen, segLen)) {   // non-dropped path: correct
  ReportTrapError(cx, JSMSG_WASM_OUT_OF_BOUNDS);
  return -1;
}

maybeSeg == nullptr exactly when the segment is unavailable: either dropped via Instance::dataDrop (WasmInstance.cpp:1424, sets passiveDataSegments_[segIndex] = nullptr) or active (active segments are never stored into passiveDataSegments_, Instance::init lines 2921-2925; they behave as already-dropped). BoundsCheckInit (WasmInstance.cpp:817) on the non-dropped branch correctly traps when dstOffset > memLen even with len == 0.

The in-tree spec test js/src/jit-test/tests/wasm/spec/spec/memory_init.wast.js:576 (case $13) does require a trap for dst=0x10001, src=0, len=0, but only exercises it on a non-dropped passive segment; the drop-then-init cases $5, $6 use len == 1. The zero-length × dropped combination is the gap.

Test case

function trapped(fn) { try { fn(); return false; } catch (e) { return true; } }
function check(name, wat, args, expectTrap) {
  let inst = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(wat)));
  let didTrap = trapped(() => inst.exports.test.apply(null, args));
  print((didTrap === expectTrap ? "[PASS] " : "[BUG ] ") + name +
        ": expected trap=" + expectTrap + ", got trap=" + didTrap);
}

// Control: PR #123 relaxation. Dropped + in-bounds dst + len=0 must NOT trap.
check("dropped, in-bounds dst, len=0",
  `(module (memory 1) (data "\\37")
     (func (export "test") (param i32 i32 i32)
       (data.drop 0)
       (memory.init 0 (local.get 0) (local.get 1) (local.get 2))))`,
  [0, 0, 0], false);

// Bug: dropped + OOB dst + len=0 must trap (spec step 2). SM: no trap.
check("dropped, OOB dst, len=0",
  `(module (memory 1) (data "\\37")
     (func (export "test") (param i32 i32 i32)
       (data.drop 0)
       (memory.init 0 (local.get 0) (local.get 1) (local.get 2))))`,
  [0x10001, 0, 0], true);

// Bug: active (auto-dropped) + OOB dst + len=0 must trap. SM: no trap.
check("active, OOB dst, len=0",
  `(module (memory 1) (data (i32.const 0) "\\37")
     (func (export "test") (param i32 i32 i32)
       (memory.init 0 (local.get 0) (local.get 1) (local.get 2))))`,
  [0x10001, 0, 0], true);

Suggested fix

Run the dst bounds check before short-circuiting on len == 0. Equivalent to BoundsCheckInit with segLen == 0, preserving PR #123's relaxation:

   if (!maybeSeg) {
-    if (len == 0 && srcOffset == 0) {
-      return 0;
-    }
-
-    ReportTrapError(cx, JSMSG_WASM_OUT_OF_BOUNDS);
-    return -1;
+    // A dropped or active data segment behaves as an empty data sequence
+    // (bulk-memory PR #123 / Bug 1594561). Spec step 2 still requires the
+    // dst check: trap when d + n > |mem|, even when n == 0.
+    WasmMemoryObject* mem = instance->memory(memoryIndex);
+    const size_t memLen = mem->volatileMemoryLength();
+    uint64_t dstOffsetLimit = uint64_t(dstOffset) + uint64_t(len);
+    if (dstOffsetLimit < dstOffset || dstOffsetLimit > memLen ||
+        srcOffset != 0 || len != 0) {
+      ReportTrapError(cx, JSMSG_WASM_OUT_OF_BOUNDS);
+      return -1;
+    }
+    return 0;
   }

Impact

Spec-conformance / interop bug, observable divergence from V8 and JSC. No memory-safety implication: len == 0 reads and writes nothing. The defect is purely a missing trap.

Comment 1

[Ben Visness [:bvisness]]

Fun that this was not covered by spec tests. I have opened a PR: https://github.com/WebAssembly/spec/pull/2176

Comment 2

[Ben Visness [:bvisness]]

Attachment: Bug 2042287: Add missing trap on dropped data segments. r=rhunt

We failed to trap if the destination index was out of bounds when doing
memory.init on a dropped data segment. The bounds checks still apply
as if the data segment had length zero, so they should always be done up
front regardless of droppedness.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c9177a0e966b
https://hg.mozilla.org/integration/autoland/rev/1a372843a5c2
Add missing trap on dropped data segments. r=rhunt

Comment 5

[Goloman Adrian[:agoloman]]

https://hg.mozilla.org/mozilla-central/rev/1a372843a5c2