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

eholk · Commit Bot · commit 841ca52c816a · 2017-10-04T17:21:07.000Z
This reverts commit 5e76ff5. Reason for revert: tsan failures - https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/17574 Original change's description: > Reland "[wasm] always allocate memory when guard regions are needed" > > This reverts commit 7cf29d8. > > 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} TBR=gdeepti@chromium.org,mtrofin@chromium.org,mlippautz@chromium.org,eholk@chromium.org,eholk@google.com,clemensh@chromium.org Change-Id: I52d5354126158a92602b08c48703d562ac95075b No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/699599 Reviewed-by: Eric Holk <eholk@chromium.org> Commit-Queue: Eric Holk <eholk@chromium.org> Cr-Commit-Position: refs/heads/master@{#48294}
diff --git a/src/heap/array-buffer-tracker-inl.h b/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 = static_cast<size_t>(buffer->byte_length()->Number());
+  size_t length = buffer->allocation_length();
   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 = static_cast<size_t>(buffer->byte_length()->Number());
+  size_t length = buffer->allocation_length();
   {
     base::LockGuard<base::RecursiveMutex> guard(page->mutex());
     LocalArrayBufferTracker* tracker = page->local_tracker();
@@ -50,30 +50,14 @@ 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;
   size_t retained_size = 0;
   for (TrackingData::iterator it = array_buffers_.begin();
        it != array_buffers_.end();) {
     JSArrayBuffer* buffer = reinterpret_cast<JSArrayBuffer*>(*it);
-    const size_t length = static_cast<size_t>(buffer->byte_length()->Number());
+    const size_t length = buffer->allocation_length();
     if (should_free(buffer)) {
       freed_memory += length;
       buffer->FreeBackingStore();
@@ -122,11 +106,6 @@ 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
 
diff --git a/src/heap/array-buffer-tracker.h b/src/heap/array-buffer-tracker.h
@@ -33,10 +33,6 @@ 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.
@@ -77,7 +73,6 @@ 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.
   //
diff --git a/src/heap/heap.cc b/src/heap/heap.cc
@@ -2341,10 +2341,6 @@ 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_ =
diff --git a/src/heap/heap.h b/src/heap/heap.h
@@ -1474,7 +1474,6 @@ 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. =================================================
diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc
@@ -1807,10 +1807,7 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
 
     DCHECK_IMPLIES(EnableGuardRegions(),
                    module_->is_asm_js() || memory->has_guard_region());
-  } 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).
+  } else if (initial_pages > 0) {
     memory_ = AllocateMemory(initial_pages);
     if (memory_.is_null()) return {};  // failed to allocate memory
   }
diff --git a/src/wasm/wasm-memory.cc b/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
-  // 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);
+  // 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);
 
   if (size > 0 && memory == nullptr) {
     return Handle<JSArrayBuffer>::null();
diff --git a/src/wasm/wasm-objects.cc b/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();
-  const uint32_t size_increase = pages * WasmModule::kPageSize;
-  const uint32_t new_size = old_size + size_increase;
+  size_t new_size =
+      static_cast<size_t>(old_pages + pages) * WasmModule::kPageSize;
   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,10 +346,6 @@ 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;
diff --git a/test/mjsunit/regress/wasm/regression-769637.js b/test/mjsunit/regress/wasm/regression-769637.js
