[wasm] Fix bounds check for zero initial memory.

Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype.

R=titzer@chromium.org, bradnelson@chromium.org

Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290
Review-Url: https://codereview.chromium.org/2416543002
Cr-Original-Commit-Position: refs/heads/master@{#40326}
Cr-Commit-Position: refs/heads/master@{#40329}

src/assembler.cc

@@ -352,9 +352,8 @@ void RelocInfo::update_wasm_memory_reference(
   } else if (IsWasmMemorySizeReference(rmode_)) {
     uint32_t current_size_reference = wasm_memory_size_reference();
     DCHECK(old_size == 0 || current_size_reference <= old_size);
-    uint32_t offset = old_size - current_size_reference;
-    DCHECK_GE(new_size, offset);
-    uint32_t updated_size_reference = new_size - offset;
+    uint32_t updated_size_reference =
+        new_size + (current_size_reference - old_size);
     unchecked_update_wasm_memory_size(updated_size_reference,
                                       icache_flush_mode);
   } else {

src/compiler/wasm-compiler.cc

@@ -2859,25 +2859,22 @@ void WasmGraphBuilder::BoundsCheckMem(MachineType memtype, Node* index,
   uint32_t size = module_->instance->mem_size;
   byte memsize = wasm::WasmOpcodes::MemSize(memtype);
 
-  // Check against the effective size.
   size_t effective_size;
-  if (size == 0) {
-    effective_size = 0;
-  } else if (offset >= size ||
-             (static_cast<uint64_t>(offset) + memsize) > size) {
+  if (size <= offset || size < (static_cast<uint64_t>(offset) + memsize)) {
     // Two checks are needed in the case where the offset is statically
     // out of bounds; one check for the offset being in bounds, and the next for
     // the offset + index being out of bounds for code to be patched correctly
     // on relocation.
-    effective_size = size - memsize + 1;
+    size_t effective_offset = offset + memsize - 1;
     Node* cond = graph()->NewNode(jsgraph()->machine()->Uint32LessThan(),
-                                  jsgraph()->IntPtrConstant(offset),
+                                  jsgraph()->IntPtrConstant(effective_offset),
                                   jsgraph()->RelocatableInt32Constant(
-                                      static_cast<uint32_t>(effective_size),
+                                      static_cast<uint32_t>(size),
                                       RelocInfo::WASM_MEMORY_SIZE_REFERENCE));
     trap_->AddTrapIfFalse(wasm::kTrapMemOutOfBounds, cond, position);
-    DCHECK(offset >= effective_size);
-    effective_size = offset - effective_size;
+    // For offset > effective size, this relies on check above to fail and
+    // effective size can be negative, relies on wrap around.
+    effective_size = size - offset - memsize + 1;
   } else {
     effective_size = size - offset - memsize + 1;
     CHECK(effective_size <= kMaxUInt32);

src/wasm/wasm-module.cc

@@ -2056,38 +2056,6 @@ Handle<WasmDebugInfo> wasm::GetDebugInfo(Handle<JSObject> wasm) {
   return new_info;
 }
 
-bool wasm::UpdateWasmModuleMemory(Handle<JSObject> object, Address old_start,
-                                  Address new_start, uint32_t old_size,
-                                  uint32_t new_size) {
-  DisallowHeapAllocation no_allocation;
-  if (!IsWasmObject(*object)) {
-    return false;
-  }
-
-  // Get code table associated with the module js_object
-  Object* obj = object->GetInternalField(kWasmModuleCodeTable);
-  Handle<FixedArray> code_table(FixedArray::cast(obj));
-
-  // Iterate through the code objects in the code table and update relocation
-  // information
-  for (int i = 0; i < code_table->length(); ++i) {
-    obj = code_table->get(i);
-    Handle<Code> code(Code::cast(obj));
-
-    int mode_mask = RelocInfo::ModeMask(RelocInfo::WASM_MEMORY_REFERENCE) |
-                    RelocInfo::ModeMask(RelocInfo::WASM_MEMORY_SIZE_REFERENCE);
-    for (RelocIterator it(*code, mode_mask); !it.done(); it.next()) {
-      RelocInfo::Mode mode = it.rinfo()->rmode();
-      if (RelocInfo::IsWasmMemoryReference(mode) ||
-          RelocInfo::IsWasmMemorySizeReference(mode)) {
-        it.rinfo()->update_wasm_memory_reference(old_start, new_start, old_size,
-                                                 new_size);
-      }
-    }
-  }
-  return true;
-}
-
 Handle<FixedArray> wasm::BuildFunctionTable(Isolate* isolate, uint32_t index,
                                             const WasmModule* module) {
   const WasmIndirectFunctionTable* table = &module->function_tables[index];
@@ -2239,9 +2207,9 @@ int32_t wasm::GetInstanceMemorySize(Isolate* isolate,
 
 int32_t wasm::GrowInstanceMemory(Isolate* isolate, Handle<JSObject> instance,
                                  uint32_t pages) {
-  if (pages == 0) {
-    return GetInstanceMemorySize(isolate, instance);
-  }
+  if (!IsWasmObject(*instance)) return false;
+  if (pages == 0) return GetInstanceMemorySize(isolate, instance);
+
   Address old_mem_start = nullptr;
   uint32_t old_size = 0, new_size = 0;
 
@@ -2278,10 +2246,8 @@ int32_t wasm::GrowInstanceMemory(Isolate* isolate, Handle<JSObject> instance,
     memcpy(new_mem_start, old_mem_start, old_size);
   }
   SetInstanceMemory(instance, *buffer);
-  if (!UpdateWasmModuleMemory(instance, old_mem_start, new_mem_start, old_size,
-                              new_size)) {
-    return -1;
-  }
+  RelocateInstanceCode(instance, old_mem_start, new_mem_start, old_size,
+                       new_size);
   DCHECK(old_size % WasmModule::kPageSize == 0);
   return (old_size / WasmModule::kPageSize);
 }

src/wasm/wasm-module.h

@@ -515,11 +515,6 @@ Handle<Script> GetAsmWasmScript(Handle<JSObject> wasm);
 int GetAsmWasmSourcePosition(Handle<JSObject> wasm, int func_index,
                              int byte_offset);
 
-// Update memory references of code objects associated with the module
-bool UpdateWasmModuleMemory(Handle<JSObject> object, Address old_start,
-                            Address new_start, uint32_t old_size,
-                            uint32_t new_size);
-
 // Constructs a single function table as a FixedArray of double size,
 // populating it with function signature indices and function indices.
 Handle<FixedArray> BuildFunctionTable(Isolate* isolate, uint32_t index,

test/mjsunit/wasm/grow-memory.js

@@ -38,6 +38,8 @@ function genGrowMemoryBuilder() {
   return builder;
 }
 
+// TODO(gdeepti): Generate tests programatically for all the sizes instead of
+// current implementation.
 function testGrowMemoryReadWrite32() {
   var builder = genGrowMemoryBuilder();
   builder.addMemory(1, 1, false);
@@ -197,15 +199,96 @@ function testGrowMemoryZeroInitialSize() {
     assertEquals(20, peek());
   }
 
-  //TODO(gdeepti): Fix tests with correct write boundaries
-  //when runtime function is fixed.
-  for(offset = kPageSize; offset <= kPageSize + 5; offset++) {
+  for(offset = kPageSize - 3; offset <= kPageSize + 5; offset++) {
     assertTraps(kTrapMemOutOfBounds, peek);
   }
+
+  offset = 3*kPageSize;
+  for (var i = 1; i < 4; i++) {
+    assertTraps(kTrapMemOutOfBounds, poke);
+    assertEquals(i, growMem(1));
+  }
+  poke(20);
+  assertEquals(20, peek());
 }
 
 testGrowMemoryZeroInitialSize();
 
+function testGrowMemoryZeroInitialSize32() {
+  var builder = genGrowMemoryBuilder();
+  var module = builder.instantiate();
+  var offset;
+  function peek() { return module.exports.load(offset); }
+  function poke(value) { return module.exports.store(offset, value); }
+  function growMem(pages) { return module.exports.grow_memory(pages); }
+
+  assertTraps(kTrapMemOutOfBounds, peek);
+  assertTraps(kTrapMemOutOfBounds, poke);
+
+  assertEquals(0, growMem(1));
+
+  for(offset = 0; offset <= kPageSize - 4; offset++) {
+    poke(20);
+    assertEquals(20, peek());
+  }
+
+  for(offset = kPageSize - 3; offset <= kPageSize + 5; offset++) {
+    assertTraps(kTrapMemOutOfBounds, peek);
+  }
+}
+
+testGrowMemoryZeroInitialSize32();
+
+function testGrowMemoryZeroInitialSize16() {
+  var builder = genGrowMemoryBuilder();
+  var module = builder.instantiate();
+  var offset;
+  function peek() { return module.exports.load16(offset); }
+  function poke(value) { return module.exports.store16(offset, value); }
+  function growMem(pages) { return module.exports.grow_memory(pages); }
+
+  assertTraps(kTrapMemOutOfBounds, peek);
+  assertTraps(kTrapMemOutOfBounds, poke);
+
+  assertEquals(0, growMem(1));
+
+  for(offset = 0; offset <= kPageSize - 2; offset++) {
+    poke(20);
+    assertEquals(20, peek());
+  }
+
+  for(offset = kPageSize - 1; offset <= kPageSize + 5; offset++) {
+    assertTraps(kTrapMemOutOfBounds, peek);
+  }
+}
+
+testGrowMemoryZeroInitialSize16();
+
+function testGrowMemoryZeroInitialSize8() {
+  var builder = genGrowMemoryBuilder();
+  var module = builder.instantiate();
+  var offset;
+  function peek() { return module.exports.load8(offset); }
+  function poke(value) { return module.exports.store8(offset, value); }
+  function growMem(pages) { return module.exports.grow_memory(pages); }
+
+  assertTraps(kTrapMemOutOfBounds, peek);
+  assertTraps(kTrapMemOutOfBounds, poke);
+
+  assertEquals(0, growMem(1));
+
+  for(offset = 0; offset <= kPageSize - 1; offset++) {
+    poke(20);
+    assertEquals(20, peek());
+  }
+
+  for(offset = kPageSize; offset <= kPageSize + 5; offset++) {
+    assertTraps(kTrapMemOutOfBounds, peek);
+  }
+}
+
+testGrowMemoryZeroInitialSize8();
+
 function testGrowMemoryTrapMaxPagesZeroInitialMemory() {
   var builder = genGrowMemoryBuilder();
   var module = builder.instantiate();

test/mjsunit/wasm/import-memory.js

@@ -80,3 +80,34 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
     }
   }
 })();
+
+(function ValidateBoundsCheck() {
+  print("Validate bounds check");
+  let memory = new WebAssembly.Memory({initial: 1, maximum: 5});
+  assertEquals(kPageSize, memory.buffer.byteLength);
+  let i32 = new Int32Array(memory.buffer);
+  let builder = new WasmModuleBuilder();
+  // builder.addImportedMemory("mine");
+  builder.addImportedMemory("mine");
+  builder.addFunction("load", kSig_i_i)
+      .addBody([kExprGetLocal, 0, kExprI32LoadMem, 0, 0])
+      .exportFunc();
+  builder.addFunction("store", kSig_i_ii)
+      .addBody([kExprGetLocal, 0, kExprGetLocal, 1, kExprI32StoreMem, 0, 0,
+                kExprGetLocal, 1])
+      .exportFunc();
+  var offset;
+  let instance = builder.instantiate({mine: memory});
+  function load() { return instance.exports.load(offset); }
+  function store(value) { return instance.exports.store(offset, value); }
+
+  for (offset = 0; offset < kPageSize -3; offset+=4) {
+    store(offset);
+  }
+  for (offset = 0; offset < kPageSize - 3; offset+=4) {
+    assertEquals(offset, load());
+  }
+  for (offset = kPageSize - 3; offset < kPageSize + 4; offset++) {
+    assertTraps(kTrapMemOutOfBounds, load);
+  }
+})();