Reland "[wasm] always allocate memory when guard regions are needed"

This reverts commit 7cf29d8d.

Original change's description:
> [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: I09fdaea92b7ccb3a6cc9e28392171ec098538a00
Reviewed-on: https://chromium-review.googlesource.com/695812
Commit-Queue: Eric Holk <eholk@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48293}

src/heap/array-buffer-tracker-inl.h

@@ -17,7 +17,7 @@ void ArrayBufferTracker::RegisterNew(Heap* heap, JSArrayBuffer* buffer) {
   void* data = buffer->backing_store();
   if (!data) return;
 
-  size_t length = buffer->allocation_length();
+  size_t length = static_cast<size_t>(buffer->byte_length()->Number());
   Page* page = Page::FromAddress(buffer->address());
   {
     base::LockGuard<base::RecursiveMutex> guard(page->mutex());
@@ -40,7 +40,7 @@ void ArrayBufferTracker::Unregister(Heap* heap, JSArrayBuffer* buffer) {
   if (!data) return;
 
   Page* page = Page::FromAddress(buffer->address());
-  size_t length = buffer->allocation_length();
+  size_t length = static_cast<size_t>(buffer->byte_length()->Number());
   {
     base::LockGuard<base::RecursiveMutex> guard(page->mutex());
     LocalArrayBufferTracker* tracker = page->local_tracker();
@@ -50,6 +50,22 @@ void ArrayBufferTracker::Unregister(Heap* heap, JSArrayBuffer* buffer) {
   heap->update_external_memory(-static_cast<intptr_t>(length));
 }
 
+void ArrayBufferTracker::IncreaseArrayBufferSize(Heap* heap,
+                                                 JSArrayBuffer* buffer,
+                                                 size_t delta) {
+  DCHECK_NOT_NULL(buffer->backing_store());
+
+  Page* const page = Page::FromAddress(buffer->address());
+  {
+    base::LockGuard<base::RecursiveMutex> guard(page->mutex());
+    LocalArrayBufferTracker* tracker = page->local_tracker();
+    DCHECK_NOT_NULL(tracker);
+    DCHECK(tracker->IsTracked(buffer));
+    tracker->IncreaseRetainedSize(delta);
+  }
+  heap->update_external_memory(delta);
+}
+
 template <typename Callback>
 void LocalArrayBufferTracker::Free(Callback should_free) {
   size_t freed_memory = 0;
@@ -57,7 +73,7 @@ void LocalArrayBufferTracker::Free(Callback should_free) {
   for (TrackingData::iterator it = array_buffers_.begin();
        it != array_buffers_.end();) {
     JSArrayBuffer* buffer = reinterpret_cast<JSArrayBuffer*>(*it);
-    const size_t length = buffer->allocation_length();
+    const size_t length = static_cast<size_t>(buffer->byte_length()->Number());
     if (should_free(buffer)) {
       freed_memory += length;
       buffer->FreeBackingStore();
@@ -106,6 +122,11 @@ void LocalArrayBufferTracker::Remove(JSArrayBuffer* buffer, size_t length) {
   array_buffers_.erase(it);
 }
 
+void LocalArrayBufferTracker::IncreaseRetainedSize(size_t delta) {
+  DCHECK_GE(retained_size_ + delta, retained_size_);
+  retained_size_ += delta;
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/heap/array-buffer-tracker.h

@@ -33,6 +33,10 @@ class ArrayBufferTracker : public AllStatic {
   // access to the tracker by taking the page lock for the corresponding page.
   inline static void RegisterNew(Heap* heap, JSArrayBuffer* buffer);
   inline static void Unregister(Heap* heap, JSArrayBuffer* buffer);
+  // Tells the tracker that the array buffer has increased in size. The buffer
+  // must already be tracked.
+  inline static void IncreaseArrayBufferSize(Heap* heap, JSArrayBuffer* buffer,
+                                             size_t delta);
 
   // Frees all backing store pointers for dead JSArrayBuffers in new space.
   // Does not take any locks and can only be called during Scavenge.
@@ -73,6 +77,7 @@ class LocalArrayBufferTracker {
 
   inline void Add(JSArrayBuffer* buffer, size_t length);
   inline void Remove(JSArrayBuffer* buffer, size_t length);
+  inline void IncreaseRetainedSize(size_t delta);
 
   // Frees up array buffers.
   //

src/heap/heap.cc

@@ -2341,6 +2341,10 @@ void Heap::UnregisterArrayBuffer(JSArrayBuffer* buffer) {
   ArrayBufferTracker::Unregister(this, buffer);
 }
 
+void Heap::TrackIncreasedArrayBufferSize(JSArrayBuffer* buffer, size_t delta) {
+  ArrayBufferTracker::IncreaseArrayBufferSize(this, buffer, delta);
+}
+
 void Heap::ConfigureInitialOldGenerationSize() {
   if (!old_generation_size_configured_ && tracer()->SurvivalEventsRecorded()) {
     old_generation_allocation_limit_ =

src/heap/heap.h

@@ -1474,6 +1474,7 @@ class Heap {
   // unregistered buffer, too, and the name is confusing.
   void RegisterNewArrayBuffer(JSArrayBuffer* buffer);
   void UnregisterArrayBuffer(JSArrayBuffer* buffer);
+  void TrackIncreasedArrayBufferSize(JSArrayBuffer* buffer, size_t delta);
 
   // ===========================================================================
   // Allocation site tracking. =================================================

src/wasm/module-compiler.cc

@@ -1807,7 +1807,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-memory.cc

@@ -88,11 +88,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();

src/wasm/wasm-objects.cc

@@ -331,8 +331,8 @@ Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
   const bool enable_guard_regions = old_buffer.is_null()
                                         ? wasm::EnableGuardRegions()
                                         : old_buffer->has_guard_region();
-  size_t new_size =
-      static_cast<size_t>(old_pages + pages) * WasmModule::kPageSize;
+  const uint32_t size_increase = pages * WasmModule::kPageSize;
+  const uint32_t new_size = old_size + size_increase;
   if (enable_guard_regions && old_size != 0) {
     DCHECK_NOT_NULL(old_buffer->backing_store());
     if (new_size > FLAG_wasm_max_mem_pages * WasmModule::kPageSize ||
@@ -346,6 +346,10 @@ Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
         ->AdjustAmountOfExternalAllocatedMemory(pages * WasmModule::kPageSize);
     Handle<Object> length_obj = isolate->factory()->NewNumberFromSize(new_size);
     old_buffer->set_byte_length(*length_obj);
+    if (!old_buffer->is_external()) {
+      isolate->heap()->TrackIncreasedArrayBufferSize(*old_buffer,
+                                                     size_increase);
+    }
     return old_buffer;
   } else {
     Handle<JSArrayBuffer> new_buffer;

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);