[wasm] Use ArrayBuffer::Allocator API for guard regions

The WebAssembly code now uses these new APIs to allocate memory with guard
regions. Guarded array buffers are no longer always external, which eliminates
a lot of special cases around WebAssembly memory.

Bug: chromium:720302
Change-Id: I355b74ac30a05a18c8b363bd256d57458742849f
Reviewed-on: https://chromium-review.googlesource.com/505715Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Commit-Queue: Eric Holk <eholk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45436}

src/objects-inl.h

@@ -6924,6 +6924,12 @@ void JSArrayBuffer::set_allocation_length(size_t value) {
       value;
 }
 
+ArrayBuffer::Allocator::AllocationMode JSArrayBuffer::allocation_mode() const {
+  using AllocationMode = ArrayBuffer::Allocator::AllocationMode;
+  return has_guard_region() ? AllocationMode::kReservation
+                            : AllocationMode::kNormal;
+}
+
 void JSArrayBuffer::set_bit_field(uint32_t bits) {
   if (kInt32Size != kPointerSize) {
 #if V8_TARGET_LITTLE_ENDIAN
@@ -6945,7 +6951,6 @@ bool JSArrayBuffer::is_external() { return IsExternal::decode(bit_field()); }
 
 
 void JSArrayBuffer::set_is_external(bool value) {
-  DCHECK(!value || !has_guard_region());
   set_bit_field(IsExternal::update(bit_field(), value));
 }
 
@@ -6975,7 +6980,7 @@ void JSArrayBuffer::set_is_shared(bool value) {
   set_bit_field(IsShared::update(bit_field(), value));
 }
 
-bool JSArrayBuffer::has_guard_region() {
+bool JSArrayBuffer::has_guard_region() const {
   return HasGuardRegion::decode(bit_field());
 }
 

src/objects.cc

@@ -19606,11 +19606,22 @@ void JSArrayBuffer::Neuter() {
 }
 
 void JSArrayBuffer::FreeBackingStore() {
+  if (allocation_base() == nullptr) {
+    return;
+  }
   using AllocationMode = ArrayBuffer::Allocator::AllocationMode;
   const size_t length = allocation_length();
-  const AllocationMode mode = has_guard_region() ? AllocationMode::kReservation
-                                                 : AllocationMode::kNormal;
+  const AllocationMode mode = allocation_mode();
   GetIsolate()->array_buffer_allocator()->Free(allocation_base(), length, mode);
+
+  // Zero out the backing store and allocation base to avoid dangling
+  // pointers.
+  set_backing_store(nullptr);
+  // TODO(eholk): set_byte_length(0) once we aren't using Smis for the
+  // byte_length. We can't do it now because the GC needs to call
+  // FreeBackingStore while it is collecting.
+  set_allocation_base(nullptr);
+  set_allocation_length(0);
 }
 
 void JSArrayBuffer::Setup(Handle<JSArrayBuffer> array_buffer, Isolate* isolate,

src/objects.h

@@ -8533,7 +8533,7 @@ class JSArrayBuffer: public JSObject {
   inline bool is_shared();
   inline void set_is_shared(bool value);
 
-  inline bool has_guard_region();
+  inline bool has_guard_region() const;
   inline void set_has_guard_region(bool value);
 
   // TODO(gdeepti): This flag is introduced to disable asm.js optimizations in
@@ -8545,6 +8545,8 @@ class JSArrayBuffer: public JSObject {
 
   void Neuter();
 
+  inline ArrayBuffer::Allocator::AllocationMode allocation_mode() const;
+
   void FreeBackingStore();
 
   V8_EXPORT_PRIVATE static void Setup(

src/wasm/wasm-module.cc

@@ -54,24 +54,6 @@ byte* raw_buffer_ptr(MaybeHandle<JSArrayBuffer> buffer, int offset) {
   return static_cast<byte*>(buffer.ToHandleChecked()->backing_store()) + offset;
 }
 
-static void MemoryFinalizer(const v8::WeakCallbackInfo<void>& data) {
-  DisallowHeapAllocation no_gc;
-  JSArrayBuffer** p = reinterpret_cast<JSArrayBuffer**>(data.GetParameter());
-  JSArrayBuffer* buffer = *p;
-
-  if (!buffer->was_neutered()) {
-    void* memory = buffer->backing_store();
-    DCHECK(memory != nullptr);
-    base::OS::Free(memory,
-                   RoundUp(kWasmMaxHeapOffset, base::OS::CommitPageSize()));
-
-    data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(
-        -buffer->byte_length()->Number());
-  }
-
-  GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
-}
-
 static void RecordStats(Isolate* isolate, Code* code, bool is_sync) {
   if (is_sync) {
     // TODO(karlschimpf): Make this work when asynchronous.
@@ -92,8 +74,8 @@ static void RecordStats(Isolate* isolate, Handle<FixedArray> functions,
 }
 
 void* TryAllocateBackingStore(Isolate* isolate, size_t size,
-                              bool enable_guard_regions, bool& is_external) {
-  is_external = false;
+                              bool enable_guard_regions, void*& allocation_base,
+                              size_t& allocation_length) {
   // TODO(eholk): Right now enable_guard_regions has no effect on 32-bit
   // systems. It may be safer to fail instead, given that other code might do
   // things that would be unsafe if they expected guard pages where there
@@ -104,26 +86,30 @@ void* TryAllocateBackingStore(Isolate* isolate, size_t size,
 
     // We always allocate the largest possible offset into the heap, so the
     // addressable memory after the guard page can be made inaccessible.
-    const size_t alloc_size =
-        RoundUp(kWasmMaxHeapOffset, base::OS::CommitPageSize());
+    allocation_length = RoundUp(kWasmMaxHeapOffset, base::OS::CommitPageSize());
     DCHECK_EQ(0, size % base::OS::CommitPageSize());
 
     // AllocateGuarded makes the whole region inaccessible by default.
-    void* memory = base::OS::AllocateGuarded(alloc_size);
-    if (memory == nullptr) {
+    allocation_base =
+        isolate->array_buffer_allocator()->Reserve(allocation_length);
+    if (allocation_base == nullptr) {
       return nullptr;
     }
 
+    void* memory = allocation_base;
+
     // Make the part we care about accessible.
-    base::OS::Unprotect(memory, size);
+    isolate->array_buffer_allocator()->SetProtection(
+        memory, size, v8::ArrayBuffer::Allocator::Protection::kReadWrite);
 
     reinterpret_cast<v8::Isolate*>(isolate)
         ->AdjustAmountOfExternalAllocatedMemory(size);
 
-    is_external = true;
     return memory;
   } else {
     void* memory = isolate->array_buffer_allocator()->Allocate(size);
+    allocation_base = memory;
+    allocation_length = size;
     return memory;
   }
 }
@@ -845,26 +831,18 @@ void RecordLazyCodeStats(Isolate* isolate, Code* code) {
 }  // namespace
 
 Handle<JSArrayBuffer> wasm::SetupArrayBuffer(Isolate* isolate,
+                                             void* allocation_base,
+                                             size_t allocation_length,
                                              void* backing_store, size_t size,
                                              bool is_external,
                                              bool enable_guard_regions) {
   Handle<JSArrayBuffer> buffer = isolate->factory()->NewJSArrayBuffer();
-  JSArrayBuffer::Setup(buffer, isolate, is_external, backing_store,
+  JSArrayBuffer::Setup(buffer, isolate, is_external, allocation_base,
+                       allocation_length, backing_store,
                        static_cast<int>(size));
   buffer->set_is_neuterable(false);
   buffer->set_is_wasm_buffer(true);
   buffer->set_has_guard_region(enable_guard_regions);
-
-  if (enable_guard_regions) {
-    // We mark the buffer as external if we allocated it here with guard
-    // pages. That means we need to arrange for it to be freed.
-
-    // TODO(eholk): Finalizers may not run when the main thread is shutting
-    // down, which means we may leak memory here.
-    Handle<Object> global_handle = isolate->global_handles()->Create(*buffer);
-    GlobalHandles::MakeWeak(global_handle.location(), global_handle.location(),
-                            &MemoryFinalizer, v8::WeakCallbackType::kFinalizer);
-  }
   return buffer;
 }
 
@@ -877,9 +855,10 @@ Handle<JSArrayBuffer> wasm::NewArrayBuffer(Isolate* isolate, size_t size,
 
   enable_guard_regions = enable_guard_regions && kGuardRegionsSupported;
 
-  bool is_external;  // Set by TryAllocateBackingStore
-  void* memory =
-      TryAllocateBackingStore(isolate, size, enable_guard_regions, is_external);
+  void* allocation_base = nullptr;  // Set by TryAllocateBackingStore
+  size_t allocation_length = 0;     // Set by TryAllocateBackingStore
+  void* memory = TryAllocateBackingStore(isolate, size, enable_guard_regions,
+                                         allocation_base, allocation_length);
 
   if (memory == nullptr) {
     return Handle<JSArrayBuffer>::null();
@@ -893,8 +872,9 @@ Handle<JSArrayBuffer> wasm::NewArrayBuffer(Isolate* isolate, size_t size,
   }
 #endif
 
-  return SetupArrayBuffer(isolate, memory, size, is_external,
-                          enable_guard_regions);
+  const bool is_external = false;
+  return SetupArrayBuffer(isolate, allocation_base, allocation_length, memory,
+                          size, is_external, enable_guard_regions);
 }
 
 void wasm::UnpackAndRegisterProtectedInstructions(
@@ -2238,27 +2218,22 @@ void wasm::DetachWebAssemblyMemoryBuffer(Isolate* isolate,
           ? static_cast<uint32_t>(buffer->byte_length()->Number())
           : 0;
   if (buffer.is_null() || byte_length == 0) return;
-  const bool has_guard_regions = buffer->has_guard_region();
   const bool is_external = buffer->is_external();
-  void* backing_store = buffer->backing_store();
   DCHECK(!buffer->is_neuterable());
-  if (!has_guard_regions && !is_external) {
+  if (!is_external) {
     buffer->set_is_external(true);
     isolate->heap()->UnregisterArrayBuffer(*buffer);
   }
+  if (free_memory) {
+    // We need to free the memory before neutering the buffer because
+    // FreeBackingStore reads buffer->allocation_base(), which is nulled out by
+    // Neuter. This means there is a dangling pointer until we neuter the
+    // buffer. Since there is no way for the user to directly call
+    // FreeBackingStore, we can ensure this is safe.
+    buffer->FreeBackingStore();
+  }
   buffer->set_is_neuterable(true);
   buffer->Neuter();
-  // Neuter but do not free, as when pages == 0, the backing store is being used
-  // by the new buffer.
-  if (!free_memory) return;
-  if (has_guard_regions) {
-    base::OS::Free(backing_store, RoundUp(i::wasm::kWasmMaxHeapOffset,
-                                          base::OS::CommitPageSize()));
-    reinterpret_cast<v8::Isolate*>(isolate)
-        ->AdjustAmountOfExternalAllocatedMemory(-byte_length);
-  } else if (!has_guard_regions && !is_external) {
-    isolate->array_buffer_allocator()->Free(backing_store, byte_length);
-  }
 }
 
 void testing::ValidateInstancesChain(Isolate* isolate,
@@ -2471,14 +2446,19 @@ Handle<JSArray> wasm::GetCustomSections(Isolate* isolate,
     if (!name->Equals(*section_name.ToHandleChecked())) continue;
 
     // Make a copy of the payload data in the section.
-    bool is_external;  // Set by TryAllocateBackingStore
+    void* allocation_base = nullptr;  // Set by TryAllocateBackingStore
+    size_t allocation_length = 0;     // Set by TryAllocateBackingStore
+    const bool enable_guard_regions = false;
     void* memory = TryAllocateBackingStore(isolate, section.payload_length,
-                                           false, is_external);
+                                           enable_guard_regions,
+                                           allocation_base, allocation_length);
 
     Handle<Object> section_data = factory->undefined_value();
     if (memory) {
       Handle<JSArrayBuffer> buffer = isolate->factory()->NewJSArrayBuffer();
-      JSArrayBuffer::Setup(buffer, isolate, is_external, memory,
+      const bool is_external = false;
+      JSArrayBuffer::Setup(buffer, isolate, is_external, allocation_base,
+                           allocation_length, memory,
                            static_cast<int>(section.payload_length));
       DisallowHeapAllocation no_gc;  // for raw access to string bytes.
       Handle<SeqOneByteString> module_bytes(compiled_module->module_bytes(),

src/wasm/wasm-module.h

@@ -430,8 +430,10 @@ WasmInstanceObject* GetOwningWasmInstance(Code* code);
 Handle<JSArrayBuffer> NewArrayBuffer(Isolate*, size_t size,
                                      bool enable_guard_regions);
 
-Handle<JSArrayBuffer> SetupArrayBuffer(Isolate*, void* backing_store,
-                                       size_t size, bool is_external,
+Handle<JSArrayBuffer> SetupArrayBuffer(Isolate*, void* allocation_base,
+                                       size_t allocation_length,
+                                       void* backing_store, size_t size,
+                                       bool is_external,
                                        bool enable_guard_regions);
 
 void DetachWebAssemblyMemoryBuffer(Isolate* isolate,

src/wasm/wasm-objects.cc

@@ -479,9 +479,10 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
     // Even for pages == 0, we need to attach a new JSArrayBuffer with the same
     // backing store and neuter the old one to be spec compliant.
     if (!old_buffer.is_null() && old_size != 0) {
-      new_buffer = SetupArrayBuffer(isolate, old_buffer->backing_store(),
-                                    old_size, old_buffer->is_external(),
-                                    old_buffer->has_guard_region());
+      new_buffer = SetupArrayBuffer(
+          isolate, old_buffer->allocation_base(),
+          old_buffer->allocation_length(), old_buffer->backing_store(),
+          old_size, old_buffer->is_external(), old_buffer->has_guard_region());
       memory_object->set_buffer(*new_buffer);
     }
     DCHECK_EQ(0, old_size % WasmModule::kPageSize);

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

@@ -1105,34 +1105,28 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
     Handle<WasmMemoryObject> mem_obj =
         WasmMemoryObject::New(isolate, memory, 100);
 
-    // TODO(eholk): Skipping calls to externalize when guard pages are enabled
-    // for now. This will have to be dealt with when turning on guard pages as
-    // currently gin assumes that it can take ownership of the ArrayBuffer.
-    // Potential for crashes as this might lead to externalizing an already
-    // externalized buffer.
-    if (!memory->has_guard_region()) v8::Utils::ToLocal(memory)->Externalize();
-    void* backing_store = memory->backing_store();
-    uint64_t byte_length = NumberToSize(memory->byte_length());
+    v8::Utils::ToLocal(memory)->Externalize();
+
     uint32_t result = WasmMemoryObject::Grow(isolate, mem_obj, 4);
-    wasm::DetachWebAssemblyMemoryBuffer(isolate, memory, true);
+    const bool free_memory = true;
+    wasm::DetachWebAssemblyMemoryBuffer(isolate, memory, free_memory);
     CHECK_EQ(16, result);
-    if (!memory->has_guard_region()) {
-      isolate->array_buffer_allocator()->Free(backing_store, byte_length);
-    }
     memory = handle(mem_obj->buffer());
-    byte_length = NumberToSize(memory->byte_length());
     instance->set_memory_buffer(*memory);
     // Externalize should make no difference without the JS API as in this case
     // the buffer is not detached.
-    if (!memory->has_guard_region()) v8::Utils::ToLocal(memory)->Externalize();
+    v8::Utils::ToLocal(memory)->Externalize();
     result = testing::RunWasmModuleForTesting(isolate, instance, 0, nullptr,
                                               ModuleOrigin::kWasmOrigin);
     CHECK_EQ(kExpectedValue, result);
     // Free the buffer as the tracker does not know about it.
-    if (!memory->has_guard_region()) {
-      isolate->array_buffer_allocator()->Free(
-          memory->backing_store(), NumberToSize(memory->byte_length()));
-    }
+    const v8::ArrayBuffer::Allocator::AllocationMode allocation_mode =
+        memory->allocation_mode();
+    isolate->array_buffer_allocator()->Free(memory->allocation_base(),
+                                            memory->allocation_length(),
+                                            allocation_mode);
+    memory->set_allocation_base(nullptr);
+    memory->set_allocation_length(0);
   }
   Cleanup();
 }
@@ -1144,7 +1138,8 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMemMemSize) {
     void* backing_store =
         isolate->array_buffer_allocator()->Allocate(16 * WasmModule::kPageSize);
     Handle<JSArrayBuffer> buffer = wasm::SetupArrayBuffer(
-        isolate, backing_store, 16 * WasmModule::kPageSize, false, false);
+        isolate, backing_store, 16 * WasmModule::kPageSize, backing_store,
+        16 * WasmModule::kPageSize, false, false);
     Handle<WasmMemoryObject> mem_obj =
         WasmMemoryObject::New(isolate, buffer, 100);
     v8::Utils::ToLocal(buffer)->Externalize();