Bug 1532285 - Handle zero-length operations at the edge of memory/table properly. r=jseward
authorLars T Hansen <lhansen@mozilla.com>
Mon, 04 Mar 2019 15:59:06 +0100
changeset 520275 97b4af9b538e1e93da307dbcf4adeb757bc4cd87
parent 520274 2d7395290c187ee80403a08267ea8c26eca2df8c
child 520276 c950aab1619a8c1e6f60b3e4badffc2745acc81a
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjseward
bugs1532285
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1532285 - Handle zero-length operations at the edge of memory/table properly. r=jseward Our bounds checking is a little too stringent - we disallow zero-length operations at the edge of the memory, table, or segment, but the spec allows that, a behavior inherited (from active segments) from the MVP. It's also a natural consequence of how we handle bounds checks for non-zero length operations. So loosen the tests slightly and adapt the test cases, testing both at the edge of the table/memory/segment and one past that. In the former case we allow the operation, in the latter not. Differential Revision: https://phabricator.services.mozilla.com/D21929
js/src/jit-test/tests/wasm/passive-segs-boundary.js
js/src/jit-test/tests/wasm/passive-segs-nonboundary.js
js/src/wasm/WasmInstance.cpp
--- a/js/src/jit-test/tests/wasm/passive-segs-boundary.js
+++ b/js/src/jit-test/tests/wasm/passive-segs-boundary.js
@@ -153,24 +153,34 @@ mem_test("",
          "(memory.init 1 (i32.const 1234) (i32.const 2) (i32.const 3))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
 // init: seg ix is valid passive, but implies copying beyond end of dst
 mem_test("",
          "(memory.init 1 (i32.const 0xFFFE) (i32.const 1) (i32.const 3))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
-// init: seg ix is valid passive, zero len, but src offset out of bounds
+// init: seg ix is valid passive, zero len, but src offset out of bounds.
+// At edge of segment is OK
 mem_test("",
-         "(memory.init 1 (i32.const 1234) (i32.const 4) (i32.const 0))",
+         "(memory.init 1 (i32.const 1234) (i32.const 4) (i32.const 0))");
+
+// One past end of segment is not OK
+mem_test("",
+         "(memory.init 1 (i32.const 1234) (i32.const 5) (i32.const 0))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
-// init: seg ix is valid passive, zero len, but dst offset out of bounds
+// init: seg ix is valid passive, zero len, but dst offset out of bounds.
+// At edge of memory is OK.
 mem_test("",
-         "(memory.init 1 (i32.const 0x10000) (i32.const 2) (i32.const 0))",
+         "(memory.init 1 (i32.const 0x10000) (i32.const 2) (i32.const 0))");
+
+// One past end of memory is not OK.
+mem_test("",
+         "(memory.init 1 (i32.const 0x10001) (i32.const 2) (i32.const 0))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
 // drop: too many args
 mem_test("data.drop 1 (i32.const 42)", "",
          WebAssembly.CompileError,
          /unused values not explicitly dropped by end of block/);
 
 // init: too many args
@@ -251,24 +261,34 @@ tab_test("",
          "(table.init 1 (i32.const 12) (i32.const 2) (i32.const 3))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
 // init: seg ix is valid passive, but implies copying beyond end of dst
 tab_test("",
          "(table.init 1 (i32.const 28) (i32.const 1) (i32.const 3))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
-// init: seg ix is valid passive, zero len, but src offset out of bounds
+// init: seg ix is valid passive, zero len, but src offset out of bounds.
+// At edge of segment is OK.
 tab_test("",
-         "(table.init 1 (i32.const 12) (i32.const 4) (i32.const 0))",
+         "(table.init 1 (i32.const 12) (i32.const 4) (i32.const 0))");
+
+// One past edge of segment is not OK.
+tab_test("",
+         "(table.init 1 (i32.const 12) (i32.const 5) (i32.const 0))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
-// init: seg ix is valid passive, zero len, but dst offset out of bounds
+// init: seg ix is valid passive, zero len, but dst offset out of bounds.
+// At edge of table is OK.
 tab_test("",
-         "(table.init 1 (i32.const 30) (i32.const 2) (i32.const 0))",
+         "(table.init 1 (i32.const 30) (i32.const 2) (i32.const 0))");
+
+// One past edge of table is not OK.
+tab_test("",
+         "(table.init 1 (i32.const 31) (i32.const 2) (i32.const 0))",
          WebAssembly.RuntimeError, /index out of bounds/);
 
 // drop: too many args
 tab_test("elem.drop 1 (i32.const 42)", "",
          WebAssembly.CompileError,
          /unused values not explicitly dropped by end of block/);
 
 // init: too many args
@@ -327,17 +347,27 @@ tab_test("(table.copy (i32.const 15) (i3
          "",
          WebAssembly.RuntimeError, /index out of bounds/);
 
 // copy: zero length with both offsets in-bounds is OK
 tab_test_nofail(
     "(table.copy (i32.const 15) (i32.const 25) (i32.const 0))",
     "");
 
-// copy: zero length with dst offset out of bounds
+// copy: zero length with dst offset out of bounds.
+// At edge of table is OK.
 tab_test("(table.copy (i32.const 30) (i32.const 15) (i32.const 0))",
+         "");
+
+// One past edge of table is not OK.
+tab_test("(table.copy (i32.const 31) (i32.const 15) (i32.const 0))",
          "",
          WebAssembly.RuntimeError, /index out of bounds/);
 
 // copy: zero length with src offset out of bounds
+// At edge of table is OK.
 tab_test("(table.copy (i32.const 15) (i32.const 30) (i32.const 0))",
+         "");
+
+// One past edge of table is not OK.
+tab_test("(table.copy (i32.const 15) (i32.const 31) (i32.const 0))",
          "",
          WebAssembly.RuntimeError, /index out of bounds/);
--- a/js/src/jit-test/tests/wasm/passive-segs-nonboundary.js
+++ b/js/src/jit-test/tests/wasm/passive-segs-nonboundary.js
@@ -542,26 +542,39 @@ function checkRange(arr, minIx, maxIxPlu
        )
      )`
     );
     inst.exports.testfn();
     let b = new Uint8Array(inst.exports.memory.buffer);
     checkRange(b, 0x00000, 0x10000, 0x00);
 }
 
-// Zero len with offset out-of-bounds gets an exception
+// Zero len with offset out-of-bounds is OK if it's at the edge of the
+// memory, but not if it is one past that.
 {
     let inst = wasmEvalText(
     `(module
        (memory (export "memory") 1 1)
        (func (export "testfn")
          (memory.fill (i32.const 0x10000) (i32.const 0x55) (i32.const 0))
        )
      )`
     );
+    inst.exports.testfn();
+}
+
+{
+    let inst = wasmEvalText(
+    `(module
+       (memory (export "memory") 1 1)
+       (func (export "testfn")
+         (memory.fill (i32.const 0x10001) (i32.const 0x55) (i32.const 0))
+       )
+     )`
+    );
     assertErrorMessage(() => inst.exports.testfn(),
                        WebAssembly.RuntimeError, /index out of bounds/);
 }
 
 // Very large range
 {
     let inst = wasmEvalText(
     `(module
@@ -710,40 +723,66 @@ function checkRange(arr, minIx, maxIxPlu
      )`
     );
     inst.exports.testfn();
     let b = new Uint8Array(inst.exports.memory.buffer);
     checkRange(b, 0x00000, 0x08000, 0x55);
     checkRange(b, 0x08000, 0x10000, 0xAA);
 }
 
-// Zero len with dest offset out-of-bounds is an exception
+// Zero len with dest offset out-of-bounds but at the edge of memory is OK
 {
     let inst = wasmEvalText(
     `(module
        (memory (export "memory") 1 1)
        (func (export "testfn")
          (memory.copy (i32.const 0x10000) (i32.const 0x7000) (i32.const 0))
        )
      )`
     );
+    inst.exports.testfn();
+}
+
+// Ditto, but one further out is not OK.
+{
+    let inst = wasmEvalText(
+    `(module
+       (memory (export "memory") 1 1)
+       (func (export "testfn")
+         (memory.copy (i32.const 0x10001) (i32.const 0x7000) (i32.const 0))
+       )
+     )`
+    );
     assertErrorMessage(() => inst.exports.testfn(),
                        WebAssembly.RuntimeError, /index out of bounds/);
 }
 
-// Zero len with src offset out-of-bounds is an exception
+// Zero len with src offset out-of-bounds but at the edge of memory is OK
 {
     let inst = wasmEvalText(
     `(module
        (memory (export "memory") 1 1)
        (func (export "testfn")
          (memory.copy (i32.const 0x9000) (i32.const 0x10000) (i32.const 0))
        )
      )`
     );
+    inst.exports.testfn();
+}
+
+// Ditto, but one element further out is not OK.
+{
+    let inst = wasmEvalText(
+    `(module
+       (memory (export "memory") 1 1)
+       (func (export "testfn")
+         (memory.copy (i32.const 0x9000) (i32.const 0x10001) (i32.const 0))
+       )
+     )`
+    );
     assertErrorMessage(() => inst.exports.testfn(),
                        WebAssembly.RuntimeError, /index out of bounds/);
 }
 
 // 100 random fills followed by 100 random copies, in a single-page buffer,
 // followed by verification of the (now heavily mashed-around) buffer.
 {
     let inst = wasmEvalText(
--- a/js/src/wasm/WasmInstance.cpp
+++ b/js/src/wasm/WasmInstance.cpp
@@ -422,18 +422,19 @@ Instance::wake(Instance* instance, uint3
 
 /* static */ int32_t /* -1 to signal trap; 0 for ok */
 Instance::memCopy(Instance* instance, uint32_t dstByteOffset,
                   uint32_t srcByteOffset, uint32_t len) {
   WasmMemoryObject* mem = instance->memory();
   uint32_t memLen = mem->volatileMemoryLength();
 
   if (len == 0) {
-    // Even though the length is zero, we must check for a valid offset.
-    if (dstByteOffset < memLen && srcByteOffset < memLen) {
+    // Even though the length is zero, we must check for a valid offset.  But
+    // zero-length operations at the edge of the memory are allowed.
+    if (dstByteOffset <= memLen && srcByteOffset <= memLen) {
       return 0;
     }
   } else {
     // Here, we know that |len - 1| cannot underflow.
     bool mustTrap = false;
 
     // As we're supposed to write data until we trap we have to deal with
     // arithmetic overflow in the limit calculation.
@@ -508,18 +509,19 @@ Instance::dataDrop(Instance* instance, u
 
 /* static */ int32_t /* -1 to signal trap; 0 for ok */
 Instance::memFill(Instance* instance, uint32_t byteOffset, uint32_t value,
                   uint32_t len) {
   WasmMemoryObject* mem = instance->memory();
   uint32_t memLen = mem->volatileMemoryLength();
 
   if (len == 0) {
-    // Even though the length is zero, we must check for a valid offset.
-    if (byteOffset < memLen) {
+    // Even though the length is zero, we must check for a valid offset.  But
+    // zero-length operations at the edge of the memory are allowed.
+    if (byteOffset <= memLen) {
       return 0;
     }
   } else {
     // Here, we know that |len - 1| cannot underflow.
 
     bool mustTrap = false;
 
     // We must write data until we trap, so we have to deal with arithmetic
@@ -580,18 +582,20 @@ Instance::memInit(Instance* instance, ui
 
   // We are proposing to copy
   //
   //   seg.bytes.begin()[ srcOffset .. srcOffset + len - 1 ]
   // to
   //   memoryBase[ dstOffset .. dstOffset + len - 1 ]
 
   if (len == 0) {
-    // Even though the length is zero, we must check for valid offsets.
-    if (dstOffset < memLen && srcOffset < segLen) {
+    // Even though the length is zero, we must check for valid offsets.  But
+    // zero-length operations at the edge of the memory or the segment are
+    // allowed.
+    if (dstOffset <= memLen && srcOffset <= segLen) {
       return 0;
     }
   } else {
     // Here, we know that |len - 1| cannot underflow.
 
     bool mustTrap = false;
 
     // As we're supposed to write data until we trap we have to deal with
@@ -640,19 +644,20 @@ Instance::tableCopy(Instance* instance, 
                     uint32_t srcTableIndex) {
   const SharedTable& srcTable = instance->tables()[srcTableIndex];
   uint32_t srcTableLen = srcTable->length();
 
   const SharedTable& dstTable = instance->tables()[dstTableIndex];
   uint32_t dstTableLen = dstTable->length();
 
   if (len == 0) {
-    // Even though the number of items to copy is zero, we must check
-    // for valid offsets.
-    if (dstOffset < dstTableLen && srcOffset < srcTableLen) {
+    // Even though the number of items to copy is zero, we must check for valid
+    // offsets.  But zero-length operations at the edge of the table are
+    // allowed.
+    if (dstOffset <= dstTableLen && srcOffset <= srcTableLen) {
       return 0;
     }
   } else {
     // Here, we know that |len - 1| cannot underflow.
     bool mustTrap = false;
 
     // As we're supposed to write data until we trap we have to deal with
     // arithmetic overflow in the limit calculation.
@@ -804,18 +809,19 @@ Instance::tableInit(Instance* instance, 
 
   // We are proposing to copy
   //
   //   seg[ srcOffset .. srcOffset + len - 1 ]
   // to
   //   tableBase[ dstOffset .. dstOffset + len - 1 ]
 
   if (len == 0) {
-    // Even though the length is zero, we must check for valid offsets.
-    if (dstOffset < tableLen && srcOffset < segLen) {
+    // Even though the length is zero, we must check for valid offsets.  But
+    // zero-length operations at the edge of the table or segment are allowed.
+    if (dstOffset <= tableLen && srcOffset <= segLen) {
       return 0;
     }
   } else {
     // Here, we know that |len - 1| cannot underflow.
     bool mustTrap = false;
 
     // As we're supposed to write data until we trap we have to deal with
     // arithmetic overflow in the limit calculation.