[wasm] always allocate memory when guard regions are needed

When using trap handlers, memory references do not get any checks inserted. This
means there is no check for a null memory as happens when the memory size is
0. Normally this would be correctly caught as an out of bounds access, since the
low memory addresses are not normally mapped. However, if they were mapped for
some reason, we would not catch the out of bounds access.

The fix is to ensure WebAssembly instances always have a guard region even if
the memory is size 0.

Bug: chromium:769637
Change-Id: I2d0f8c107563236c3780eb7746c2f820e319c65f
Reviewed-on: https://chromium-review.googlesource.com/693137Reviewed-by: Mircea Trofin <mtrofin@chromium.org>
Commit-Queue: Eric Holk <eholk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48240}

src/wasm/module-compiler.cc

@@ -1806,7 +1806,10 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
 
     DCHECK_IMPLIES(EnableGuardRegions(),
                    module_->is_asm_js() || memory->has_guard_region());
-  } else if (initial_pages > 0) {
+  } else if (initial_pages > 0 || EnableGuardRegions()) {
+    // We need to unconditionally create a guard region if using trap handlers,
+    // even when the size is zero to prevent null-derence issues
+    // (e.g. https://crbug.com/769637).
     memory_ = AllocateMemory(initial_pages);
     if (memory_.is_null()) return {};  // failed to allocate memory
   }

src/wasm/wasm-module.cc

@@ -138,11 +138,11 @@ Handle<JSArrayBuffer> NewArrayBuffer(Isolate* isolate, size_t size,
 
   void* allocation_base = nullptr;  // Set by TryAllocateBackingStore
   size_t allocation_length = 0;     // Set by TryAllocateBackingStore
-  // Do not reserve memory till non zero memory is encountered.
-  void* memory =
-      (size == 0) ? nullptr
-                  : TryAllocateBackingStore(isolate, size, enable_guard_regions,
-                                            allocation_base, allocation_length);
+  // Normally we would avoid calling TryAllocateBackingStore at all for
+  // zero-sized memories. This is tricky with guard pages. Instead, this logic
+  // for when to allocate lives inside TryAllocateBackingStore.
+  void* memory = TryAllocateBackingStore(isolate, size, enable_guard_regions,
+                                         allocation_base, allocation_length);
 
   if (size > 0 && memory == nullptr) {
     return Handle<JSArrayBuffer>::null();

test/mjsunit/regress/wasm/regression-769637.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+let builder = new WasmModuleBuilder();
+builder
+    .addMemory()
+    .addFunction("main", kSig_v_v)
+    .addBody([kExprI32Const, 4,
+              kExprI32Const, 8,
+              kExprI32StoreMem, 0, 16])
+    .exportAs("main");
+let instance = builder.instantiate();
+assertTraps(kTrapMemOutOfBounds, instance.exports.main);