[wasm] Memory buffer should be detached after Memory.Grow

src/wasm/wasm-module.cc

 // Copyright 2015 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.
 
 #include <memory>
 
 #include "src/assembler-inl.h"
 #include "src/base/adapters.h"
 #include "src/base/atomic-utils.h"
 #include "src/code-stubs.h"
@@ skipping 43 matching lines @@
       it.rinfo()->set_target_object(*new_ref);
     }
   }
 }
 
 static void MemoryFinalizer(const v8::WeakCallbackInfo<void>& data) {
   DisallowHeapAllocation no_gc;
   JSArrayBuffer** p = reinterpret_cast<JSArrayBuffer**>(data.GetParameter());
   JSArrayBuffer* buffer = *p;
 
-  void* memory = buffer->backing_store();
-  base::OS::Free(memory,
-                 RoundUp(kWasmMaxHeapOffset, base::OS::CommitPageSize()));
+  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());
+    data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(
+        -buffer->byte_length()->Number());
+  }
 
   GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
 }
 
 #if V8_TARGET_ARCH_64_BIT
 const bool kGuardRegionsSupported = true;
 #else
 const bool kGuardRegionsSupported = false;
 #endif
 
@@ skipping 669 matching lines @@
   DCHECK(name_chars >= 0 && name_chars < kBufferSize);
   MaybeHandle<String> name_str = isolate->factory()->NewStringFromOneByte(
       Vector<const uint8_t>(reinterpret_cast<uint8_t*>(buffer), name_chars),
       TENURED);
   script->set_name(*name_str.ToHandleChecked());
 
   return script;
 }
 }  // namespace
 
+Handle<JSArrayBuffer> SetupArrayBuffer(Isolate* isolate, 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,
+                       static_cast<int>(size));
+  buffer->set_is_neuterable(false);
+  buffer->set_has_guard_region(enable_guard_regions);
+
+  if (is_external) {
+    // 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;
+}
+
 Handle<JSArrayBuffer> wasm::NewArrayBuffer(Isolate* isolate, size_t size,
                                            bool enable_guard_regions) {
   if (size > (FLAG_wasm_max_mem_pages * WasmModule::kPageSize)) {
     // TODO(titzer): lift restriction on maximum memory allocated here.
     return Handle<JSArrayBuffer>::null();
   }
 
   enable_guard_regions = enable_guard_regions && kGuardRegionsSupported;
 
   bool is_external;  // Set by TryAllocateBackingStore
   void* memory =
       TryAllocateBackingStore(isolate, size, enable_guard_regions, is_external);
 
   if (memory == nullptr) {
     return Handle<JSArrayBuffer>::null();
   }
 
 #if DEBUG
   // Double check the API allocator actually zero-initialized the memory.
   const byte* bytes = reinterpret_cast<const byte*>(memory);
   for (size_t i = 0; i < size; ++i) {
     DCHECK_EQ(0, bytes[i]);
   }
 #endif
 
-  Handle<JSArrayBuffer> buffer = isolate->factory()->NewJSArrayBuffer();
-  JSArrayBuffer::Setup(buffer, isolate, is_external, memory,
-                       static_cast<int>(size));
-  buffer->set_is_neuterable(false);
-  buffer->set_has_guard_region(enable_guard_regions);
-
-  if (is_external) {
-    // 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;
+  return SetupArrayBuffer(isolate, memory, size, is_external,
+                          enable_guard_regions);
 }
 
 const char* wasm::SectionName(WasmSectionCode code) {
   switch (code) {
     case kUnknownSectionCode:
       return "Unknown";
     case kTypeSectionCode:
       return "Type";
     case kImportSectionCode:
       return "Import";
@@ skipping 1530 matching lines @@
     old_size = old_buffer->byte_length()->Number();
   }
   DCHECK(old_size + pages * WasmModule::kPageSize <=
          std::numeric_limits<uint32_t>::max());
   uint32_t new_size = old_size + pages * WasmModule::kPageSize;
   if (new_size <= old_size || max_pages * WasmModule::kPageSize < new_size ||
       FLAG_wasm_max_mem_pages * WasmModule::kPageSize < new_size) {
     return Handle<JSArrayBuffer>::null();
   }
 
-  Handle<JSArrayBuffer> new_buffer;
-  if (!old_buffer.is_null() && old_buffer->has_guard_region()) {
-    // We don't move the backing store, we simply change the protection to make
-    // more of it accessible.
-    base::OS::Unprotect(old_buffer->backing_store(), new_size);
-    reinterpret_cast<v8::Isolate*>(isolate)
-        ->AdjustAmountOfExternalAllocatedMemory(pages * WasmModule::kPageSize);
-    Handle<Object> new_size_object =
-        isolate->factory()->NewNumberFromSize(new_size);
-    old_buffer->set_byte_length(*new_size_object);
-    new_buffer = old_buffer;
-  } else {
-    const bool enable_guard_regions = false;
-    new_buffer = NewArrayBuffer(isolate, new_size, enable_guard_regions);
-    if (new_buffer.is_null()) return new_buffer;
-    Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
-    if (old_size != 0) {
-      memcpy(new_mem_start, old_mem_start, old_size);
-    }
+  const bool enable_guard_regions =

[titzer, 2017/01/26 21:35:27]

Can you please add a TODO here to reimplement the old functionality. It's only
necessary to do that before guard regions get turned on, though.

[gdeepti, 2017/01/26 21:41:30]

Done.

+      !old_buffer.is_null() && old_buffer->has_guard_region();
+  Handle<JSArrayBuffer> new_buffer =
+      NewArrayBuffer(isolate, new_size, enable_guard_regions);
+  if (new_buffer.is_null()) return new_buffer;
+  Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
+  if (old_size != 0) {
+    memcpy(new_mem_start, old_mem_start, old_size);
   }
   return new_buffer;
 }
 
 void UncheckedUpdateInstanceMemory(Isolate* isolate,
                                    Handle<WasmInstanceObject> instance,
                                    Address old_mem_start, uint32_t old_size) {
   DCHECK(instance->has_memory_buffer());
   Handle<JSArrayBuffer> new_buffer(instance->memory_buffer());
   uint32_t new_size = new_buffer->byte_length()->Number();
   DCHECK(new_size <= std::numeric_limits<uint32_t>::max());
   Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
   DCHECK_NOT_NULL(new_mem_start);
   Handle<FixedArray> code_table = instance->compiled_module()->code_table();
   RelocateMemoryReferencesInCode(
       code_table, instance->compiled_module()->module()->num_imported_functions,
       old_mem_start, new_mem_start, old_size, new_size);
 }
 
+void DetachArrayBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer) {
+  const bool has_guard_regions =
+      (!buffer.is_null() && buffer->has_guard_region());
+  void* backing_store = buffer->backing_store();
+  if (backing_store != nullptr) {
+    DCHECK(!buffer->is_neuterable());
+    int64_t byte_length = NumberToSize(buffer->byte_length());
+    buffer->set_is_neuterable(true);
+    if (!has_guard_regions) {
+      buffer->set_is_external(true);
+      isolate->heap()->UnregisterArrayBuffer(*buffer);
+    }
+    buffer->Neuter();
+    if (!has_guard_regions) {
+      isolate->array_buffer_allocator()->Free(backing_store, byte_length);
+    } else {
+      base::OS::Free(backing_store, RoundUp(i::wasm::kWasmMaxHeapOffset,
+                                            base::OS::CommitPageSize()));
+      reinterpret_cast<v8::Isolate*>(isolate)
+          ->AdjustAmountOfExternalAllocatedMemory(-byte_length);
+    }
+  }
+}
+
 int32_t wasm::GrowWebAssemblyMemory(Isolate* isolate,
                                     Handle<WasmMemoryObject> receiver,
                                     uint32_t pages) {
   DCHECK(WasmJs::IsWasmMemoryObject(isolate, receiver));
   Handle<WasmMemoryObject> memory_object =
       handle(WasmMemoryObject::cast(*receiver));
   MaybeHandle<JSArrayBuffer> memory_buffer = handle(memory_object->buffer());
   Handle<JSArrayBuffer> old_buffer;
   uint32_t old_size = 0;
   Address old_mem_start = nullptr;
   if (memory_buffer.ToHandle(&old_buffer) &&
       old_buffer->backing_store() != nullptr) {
     old_size = old_buffer->byte_length()->Number();
     old_mem_start = static_cast<Address>(old_buffer->backing_store());
   }
+  Handle<JSArrayBuffer> new_buffer;
   // Return current size if grow by 0
   if (pages == 0) {
+    if (!old_buffer.is_null() && old_buffer->backing_store() != nullptr) {
+      new_buffer = SetupArrayBuffer(isolate, old_buffer->backing_store(),
+                                    old_size, old_buffer->is_external(),
+                                    old_buffer->has_guard_region());
+      memory_object->set_buffer(*new_buffer);
+      old_buffer->set_is_neuterable(true);
+      if (!old_buffer->has_guard_region()) {
+        old_buffer->set_is_external(true);
+        isolate->heap()->UnregisterArrayBuffer(*old_buffer);
+      }
+      // Neuter but don't free the memory because it is now being used by
+      // new_buffer.
+      old_buffer->Neuter();
+    }
     DCHECK(old_size % WasmModule::kPageSize == 0);
     return (old_size / WasmModule::kPageSize);
   }
-  Handle<JSArrayBuffer> new_buffer;
   if (!memory_object->has_instances_link()) {
     // Memory object does not have an instance associated with it, just grow
     uint32_t max_pages;
     if (memory_object->has_maximum_pages()) {
       max_pages = static_cast<uint32_t>(memory_object->maximum_pages());
       if (FLAG_wasm_max_mem_pages < max_pages) return -1;
     } else {
       max_pages = FLAG_wasm_max_mem_pages;
     }
     new_buffer = GrowMemoryBuffer(isolate, memory_buffer, pages, max_pages);
@@ skipping 16 matching lines @@
     while (instance_wrapper->has_next()) {
       instance_wrapper = instance_wrapper->next_wrapper();
       DCHECK(WasmInstanceWrapper::IsWasmInstanceWrapper(*instance_wrapper));
       Handle<WasmInstanceObject> instance = instance_wrapper->instance_object();
       DCHECK(IsWasmInstance(*instance));
       SetInstanceMemory(instance, *new_buffer);
       UncheckedUpdateInstanceMemory(isolate, instance, old_mem_start, old_size);
     }
   }
   memory_object->set_buffer(*new_buffer);
+  DetachArrayBuffer(isolate, old_buffer);
   DCHECK(old_size % WasmModule::kPageSize == 0);
   return (old_size / WasmModule::kPageSize);
 }
 
 int32_t wasm::GrowMemory(Isolate* isolate, Handle<WasmInstanceObject> instance,
                          uint32_t pages) {
   if (!IsWasmInstance(*instance)) return -1;
   if (pages == 0) return GetInstanceMemorySize(isolate, instance);
   Handle<WasmInstanceObject> instance_obj(WasmInstanceObject::cast(*instance));
   if (!instance_obj->has_memory_object()) {
@@ skipping 297 matching lines @@
   Handle<FixedArray> storage = factory->NewFixedArray(num_custom_sections);
   JSArray::SetContent(array_object, storage);
   array_object->set_length(Smi::FromInt(num_custom_sections));
 
   for (int i = 0; i < num_custom_sections; i++) {
     storage->set(i, *matching_sections[i]);
   }
 
   return array_object;
 }