Fix bounds check of a store instruction after a grow_memory instruction

 - Store instruction with an offset bigger than GrowMemory offset should handle out of bounds correctly
 - Refactor to separate runnning from compile so arguments can be passed in to module builder tests.

BUG=chromium:644670

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

Review-Url: https://codereview.chromium.org/2373613004
Cr-Commit-Position: refs/heads/master@{#39840}

src/compiler/wasm-compiler.cc

@@ -2770,19 +2770,34 @@ void WasmGraphBuilder::BoundsCheckMem(MachineType memtype, Node* index,
 
   // Check against the effective size.
   size_t effective_size;
-  if (offset >= size || (static_cast<uint64_t>(offset) + memsize) > size) {
+  if (size == 0) {
     effective_size = 0;
+  } else if (offset >= size ||
+             (static_cast<uint64_t>(offset) + memsize) > size) {
+    // 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;
+    Node* cond = graph()->NewNode(jsgraph()->machine()->Uint32LessThan(),
+                                  jsgraph()->IntPtrConstant(offset),
+                                  jsgraph()->RelocatableInt32Constant(
+                                      static_cast<uint32_t>(effective_size),
+                                      RelocInfo::WASM_MEMORY_SIZE_REFERENCE));
+    trap_->AddTrapIfFalse(wasm::kTrapMemOutOfBounds, cond, position);
+    DCHECK(offset >= effective_size);
+    effective_size = offset - effective_size;
   } else {
     effective_size = size - offset - memsize + 1;
-  }
-  CHECK(effective_size <= kMaxUInt32);
-
-  Uint32Matcher m(index);
-  if (m.HasValue()) {
-    uint32_t value = m.Value();
-    if (value < effective_size) {
-      // The bounds check will always succeed.
-      return;
+    CHECK(effective_size <= kMaxUInt32);
+
+    Uint32Matcher m(index);
+    if (m.HasValue()) {
+      uint32_t value = m.Value();
+      if (value < effective_size) {
+        // The bounds check will always succeed.
+        return;
+      }
     }
   }
 

test/cctest/wasm/test-run-wasm-module.cc

@@ -34,6 +34,20 @@ void TestModule(Zone* zone, WasmModuleBuilder* builder,
   CHECK_EQ(expected_result, result);
 }
 
+void TestModuleException(Zone* zone, WasmModuleBuilder* builder) {
+  ZoneBuffer buffer(zone);
+  builder->WriteTo(buffer);
+
+  Isolate* isolate = CcTest::InitIsolateOnce();
+  HandleScope scope(isolate);
+  testing::SetupIsolateForWasmModule(isolate);
+  v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
+  testing::CompileAndRunWasmModule(isolate, buffer.begin(), buffer.end(),
+                                   ModuleOrigin::kWasmOrigin);
+  CHECK(try_catch.HasCaught());
+  isolate->clear_pending_exception();
+}
+
 void ExportAs(WasmFunctionBuilder* f, const char* name) {
   f->SetExported();
   f->SetName(name, static_cast<int>(strlen(name)));
@@ -267,3 +281,122 @@ TEST(Run_WasmModule_GrowMemoryInIf) {
   f->EmitCode(code, sizeof(code));
   TestModule(&zone, builder, 12);
 }
+
+TEST(Run_WasmModule_GrowMemOobOffset) {
+  static const int kPageSize = 0x10000;
+  // Initial memory size = 16 + GrowMemory(10)
+  static const int index = kPageSize * 17 + 4;
+  int value = 0xaced;
+  TestSignatures sigs;
+  v8::internal::AccountingAllocator allocator;
+  Zone zone(&allocator);
+
+  WasmModuleBuilder* builder = new (&zone) WasmModuleBuilder(&zone);
+  WasmFunctionBuilder* f = builder->AddFunction(sigs.i_v());
+  ExportAsMain(f);
+  byte code[] = {
+      WASM_GROW_MEMORY(WASM_I8(1)),
+      WASM_STORE_MEM(MachineType::Int32(), WASM_I32V(index), WASM_I32V(value))};
+  f->EmitCode(code, sizeof(code));
+  TestModuleException(&zone, builder);
+}
+
+TEST(Run_WasmModule_GrowMemOobFixedIndex) {
+  static const int kPageSize = 0x10000;
+  // Initial memory size = 16 + GrowMemory(10)
+  static const int index = kPageSize * 26 + 4;
+  int value = 0xaced;
+  TestSignatures sigs;
+  Isolate* isolate = CcTest::InitIsolateOnce();
+  Zone zone(isolate->allocator());
+
+  WasmModuleBuilder* builder = new (&zone) WasmModuleBuilder(&zone);
+  WasmFunctionBuilder* f = builder->AddFunction(sigs.i_i());
+  ExportAsMain(f);
+  byte code[] = {
+      WASM_GROW_MEMORY(WASM_GET_LOCAL(0)), WASM_DROP,
+      WASM_STORE_MEM(MachineType::Int32(), WASM_I32V(index), WASM_I32V(value)),
+      WASM_LOAD_MEM(MachineType::Int32(), WASM_I32V(index))};
+  f->EmitCode(code, sizeof(code));
+
+  HandleScope scope(isolate);
+  ZoneBuffer buffer(&zone);
+  builder->WriteTo(buffer);
+  testing::SetupIsolateForWasmModule(isolate);
+
+  Handle<JSObject> instance = testing::CompileInstantiateWasmModuleForTesting(
+      isolate, &zone, buffer.begin(), buffer.end(), ModuleOrigin::kWasmOrigin);
+  CHECK(!instance.is_null());
+
+  // Initial memory size is 16 pages, should trap till index > MemSize on
+  // consecutive GrowMem calls
+  for (uint32_t i = 1; i < 5; i++) {
+    Handle<Object> params[1] = {Handle<Object>(Smi::FromInt(i), isolate)};
+    v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
+    testing::RunWasmModuleForTesting(isolate, instance, 1, params,
+                                     ModuleOrigin::kWasmOrigin);
+    CHECK(try_catch.HasCaught());
+    isolate->clear_pending_exception();
+  }
+
+  Handle<Object> params[1] = {Handle<Object>(Smi::FromInt(1), isolate)};
+  int32_t result = testing::RunWasmModuleForTesting(
+      isolate, instance, 1, params, ModuleOrigin::kWasmOrigin);
+  CHECK(result == 0xaced);
+}
+
+TEST(Run_WasmModule_GrowMemOobVariableIndex) {
+  static const int kPageSize = 0x10000;
+  int value = 0xaced;
+  TestSignatures sigs;
+  Isolate* isolate = CcTest::InitIsolateOnce();
+  v8::internal::AccountingAllocator allocator;
+  Zone zone(&allocator);
+
+  WasmModuleBuilder* builder = new (&zone) WasmModuleBuilder(&zone);
+  WasmFunctionBuilder* f = builder->AddFunction(sigs.i_i());
+  ExportAsMain(f);
+  byte code[] = {
+      WASM_GROW_MEMORY(WASM_I8(1)), WASM_DROP,
+      WASM_STORE_MEM(MachineType::Int32(), WASM_GET_LOCAL(0), WASM_I32V(value)),
+      WASM_LOAD_MEM(MachineType::Int32(), WASM_GET_LOCAL(0))};
+  f->EmitCode(code, sizeof(code));
+
+  HandleScope scope(isolate);
+  ZoneBuffer buffer(&zone);
+  builder->WriteTo(buffer);
+  testing::SetupIsolateForWasmModule(isolate);
+
+  Handle<JSObject> instance = testing::CompileInstantiateWasmModuleForTesting(
+      isolate, &zone, buffer.begin(), buffer.end(), ModuleOrigin::kWasmOrigin);
+
+  CHECK(!instance.is_null());
+
+  // Initial memory size is 16 pages, should trap till index > MemSize on
+  // consecutive GrowMem calls
+  for (int i = 1; i < 5; i++) {
+    Handle<Object> params[1] = {
+        Handle<Object>(Smi::FromInt((16 + i) * kPageSize - 3), isolate)};
+    v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
+    testing::RunWasmModuleForTesting(isolate, instance, 1, params,
+                                     ModuleOrigin::kWasmOrigin);
+    CHECK(try_catch.HasCaught());
+    isolate->clear_pending_exception();
+  }
+
+  for (int i = 1; i < 5; i++) {
+    Handle<Object> params[1] = {
+        Handle<Object>(Smi::FromInt((20 + i) * kPageSize - 4), isolate)};
+    int32_t result = testing::RunWasmModuleForTesting(
+        isolate, instance, 1, params, ModuleOrigin::kWasmOrigin);
+    CHECK(result == 0xaced);
+  }
+
+  v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
+  Handle<Object> params[1] = {
+      Handle<Object>(Smi::FromInt(25 * kPageSize), isolate)};
+  testing::RunWasmModuleForTesting(isolate, instance, 1, params,
+                                   ModuleOrigin::kWasmOrigin);
+  CHECK(try_catch.HasCaught());
+  isolate->clear_pending_exception();
+}

test/common/wasm/wasm-module-runner.cc

@@ -13,7 +13,6 @@
 #include "src/wasm/wasm-js.h"
 #include "src/wasm/wasm-module.h"
 #include "src/wasm/wasm-result.h"
-#include "src/zone/zone.h"
 
 namespace v8 {
 namespace internal {
@@ -57,7 +56,6 @@ const Handle<JSObject> InstantiateModuleForTesting(Isolate* isolate,
   if (module->export_table.size() == 0) {
     thrower->Error("Not supported: module has no exports.");
   }
-
   if (thrower->error()) return Handle<JSObject>::null();
 
   // Although we decoded the module for some pre-validation, run the bytes
@@ -65,7 +63,10 @@ const Handle<JSObject> InstantiateModuleForTesting(Isolate* isolate,
   MaybeHandle<JSObject> module_object = CreateModuleObjectFromBytes(
       isolate, module->module_start, module->module_end, thrower,
       ModuleOrigin::kWasmOrigin);
-  if (module_object.is_null()) return Handle<JSObject>::null();
+  if (module_object.is_null()) {
+    thrower->Error("Module pre-validation failed.");
+    return Handle<JSObject>::null();
+  }
   MaybeHandle<JSObject> maybe_instance = WasmModule::Instantiate(
       isolate, thrower, module_object.ToHandleChecked(),
       Handle<JSReceiver>::null(), Handle<JSArrayBuffer>::null());
@@ -76,26 +77,40 @@ const Handle<JSObject> InstantiateModuleForTesting(Isolate* isolate,
   return instance;
 }
 
+const Handle<JSObject> CompileInstantiateWasmModuleForTesting(
+    Isolate* isolate, Zone* zone, const byte* module_start,
+    const byte* module_end, ModuleOrigin origin) {
+  ErrorThrower thrower(isolate, "CompileInstantiateWasmModule");
+  std::unique_ptr<const WasmModule> module(DecodeWasmModuleForTesting(
+      isolate, zone, &thrower, module_start, module_end, origin));
+
+  if (module == nullptr) {
+    thrower.Error("Wasm module decode failed");
+    return Handle<JSObject>::null();
+  }
+  return InstantiateModuleForTesting(isolate, &thrower, module.get());
+}
+
+int32_t RunWasmModuleForTesting(Isolate* isolate, Handle<JSObject> instance,
+                                int argc, Handle<Object> argv[],
+                                ModuleOrigin origin) {
+  ErrorThrower thrower(isolate, "RunWasmModule");
+  const char* f_name = origin == ModuleOrigin::kAsmJsOrigin ? "caller" : "main";
+  return CallWasmFunctionForTesting(isolate, instance, &thrower, f_name, argc,
+                                    argv, origin);
+}
+
 int32_t CompileAndRunWasmModule(Isolate* isolate, const byte* module_start,
                                 const byte* module_end, ModuleOrigin origin) {
   HandleScope scope(isolate);
   Zone zone(isolate->allocator());
 
-  ErrorThrower thrower(isolate, "CompileAndRunWasmModule");
-  std::unique_ptr<const WasmModule> module(DecodeWasmModuleForTesting(
-      isolate, &zone, &thrower, module_start, module_end, origin));
-
-  if (module == nullptr) {
-    return -1;
-  }
-  Handle<JSObject> instance =
-      InstantiateModuleForTesting(isolate, &thrower, module.get());
+  Handle<JSObject> instance = CompileInstantiateWasmModuleForTesting(
+      isolate, &zone, module_start, module_end, origin);
   if (instance.is_null()) {
     return -1;
   }
-  const char* f_name = origin == ModuleOrigin::kAsmJsOrigin ? "caller" : "main";
-  return CallWasmFunctionForTesting(isolate, instance, &thrower, f_name, 0,
-                                    nullptr, origin);
+  return RunWasmModuleForTesting(isolate, instance, 0, nullptr, origin);
 }
 
 int32_t InterpretWasmModule(Isolate* isolate, ErrorThrower* thrower,

test/common/wasm/wasm-module-runner.h

@@ -47,6 +47,15 @@ int32_t InterpretWasmModule(Isolate* isolate, ErrorThrower* thrower,
                             const WasmModule* module, int function_index,
                             WasmVal* args);
 
+// Compiles WasmModule bytes and return an instance of the compiled module.
+const Handle<JSObject> CompileInstantiateWasmModuleForTesting(
+    Isolate* isolate, Zone* zone, const byte* module_start,
+    const byte* module_end, ModuleOrigin origin);
+
+// Runs the module instance with arguments.
+int32_t RunWasmModuleForTesting(Isolate* isolate, Handle<JSObject> instance,
+                                int argc, Handle<Object> argv[],
+                                ModuleOrigin origin);
 // Install function map, module symbol for testing
 void SetupIsolateForWasmModule(Isolate* isolate);
 }  // namespace testing

test/mjsunit/wasm/grow-memory.js

@@ -326,3 +326,35 @@ function testGrowMemoryPreservesDataMemOp8() {
 }
 
 testGrowMemoryPreservesDataMemOp8();
+
+function testGrowMemoryOutOfBoundsOffset() {
+  var builder = genGrowMemoryBuilder();
+  builder.addMemory(1, 1, false);
+  var module = builder.instantiate();
+  var offset, val;
+  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); }
+
+  offset = 3*kPageSize + 4;
+  assertTraps(kTrapMemOutOfBounds, poke);
+
+  assertEquals(1, growMem(1));
+  assertTraps(kTrapMemOutOfBounds, poke);
+
+  assertEquals(2, growMem(1));
+  assertTraps(kTrapMemOutOfBounds, poke);
+
+  assertEquals(3, growMem(1));
+
+  for (offset = 3*kPageSize; offset <= 4*kPageSize - 4; offset++) {
+    poke(0xaced);
+    assertEquals(0xaced, peek());
+  }
+
+  for (offset = 4*kPageSize - 3; offset <= 4*kPageSize + 4; offset++) {
+    assertTraps(kTrapMemOutOfBounds, poke);
+  }
+}
+
+testGrowMemoryOutOfBoundsOffset();