[wasm] Memory.Grow with guard pages enabled should adjust memory allocated

- Memory.Grow with guard pages enabled should adjust amount of allocated
   memory, and not allocate a new buffer. This was disabled because previously
   the backing store was freed in the MemoryFinalizer, and we needed to be sure
   that the backing store is not released till the last buffer using it is
   released. This is now safe as we no longer use the MemoryFinalizer
 - SetProtection should use Guard/Unprotect that use mprotect underneath,
   instead of CommitRegion/UncommitRegion that use mmap
 - Move buffer allocation to the end to avoid inconsistent memory due to GC

BUG=v8:5886

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I0d7edb884bd1e3167eb5fbced6953c6401688d40
Reviewed-on: https://chromium-review.googlesource.com/629517
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Reviewed-by: Eric Holk <eholk@chromium.org>
Commit-Queue: Deepti Gandluri <gdeepti@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47960}

src/api.cc

@@ -504,12 +504,11 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
       v8::ArrayBuffer::Allocator::Protection protection) {
     switch (protection) {
       case v8::ArrayBuffer::Allocator::Protection::kNoAccess: {
-        base::VirtualMemory::UncommitRegion(data, length);
+        base::OS::Guard(data, length);
         return;
       }
       case v8::ArrayBuffer::Allocator::Protection::kReadWrite: {
-        const bool is_executable = false;
-        base::VirtualMemory::CommitRegion(data, length, is_executable);
+        base::OS::Unprotect(data, length);
         return;
       }
     }

src/base/platform/platform-win32.cc

@@ -828,7 +828,6 @@ void OS::Guard(void* address, const size_t size) {
 
 void OS::Unprotect(void* address, const size_t size) {
   LPVOID result = VirtualAlloc(address, size, MEM_COMMIT, PAGE_READWRITE);
-  DCHECK_IMPLIES(result != nullptr, GetLastError() == 0);
   USE(result);
 }
 

src/wasm/wasm-js.cc

@@ -764,8 +764,16 @@ void WebAssemblyMemoryGrow(const v8::FunctionCallbackInfo<v8::Value>& args) {
     thrower.RangeError("Unable to grow instance memory.");
     return;
   }
-  bool free_memory = (delta_size != 0);
   if (!old_buffer->is_shared()) {
+    // When delta_size == 0, or guard pages are enabled, the same backing store
+    // is used. To be spec compliant, the buffer associated with the memory
+    // object needs to be detached. Setup a new buffer with the same backing
+    // store, detach the old buffer, and do not free backing store memory.
+    bool free_memory = delta_size != 0 && !old_buffer->has_guard_region();
+    if ((!free_memory && old_size != 0) || new_size64 == 0) {
+      i::WasmMemoryObject::SetupNewBufferWithSameBackingStore(
+          i_isolate, receiver, static_cast<uint32_t>(new_size64));
+    }
     i::wasm::DetachWebAssemblyMemoryBuffer(i_isolate, old_buffer, free_memory);
   }
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();

src/wasm/wasm-module.cc

@@ -251,7 +251,8 @@ Handle<JSArrayBuffer> SetupArrayBuffer(Isolate* isolate, void* allocation_base,
                                        bool is_external,
                                        bool enable_guard_regions,
                                        SharedFlag shared) {
-  Handle<JSArrayBuffer> buffer = isolate->factory()->NewJSArrayBuffer(shared);
+  Handle<JSArrayBuffer> buffer =
+      isolate->factory()->NewJSArrayBuffer(shared, TENURED);
   DCHECK_GE(kMaxInt, size);
   if (shared == SharedFlag::kShared) DCHECK(FLAG_experimental_wasm_threads);
   JSArrayBuffer::Setup(buffer, isolate, is_external, allocation_base,

src/wasm/wasm-objects.cc

@@ -292,20 +292,33 @@ Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
   if (old_pages > maximum_pages || pages > maximum_pages - old_pages) {
     return Handle<JSArrayBuffer>::null();
   }
-
-  // TODO(gdeepti): Change the protection here instead of allocating a new
-  // buffer before guard regions are turned on, see issue #5886.
   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;
-  Handle<JSArrayBuffer> new_buffer =
-      wasm::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());
-  memcpy(new_mem_start, old_mem_start, old_size);
-  return new_buffer;
+  if (enable_guard_regions && old_size != 0) {
+    DCHECK(old_buffer->backing_store() != nullptr);
+    if (new_size > FLAG_wasm_max_mem_pages * WasmModule::kPageSize ||
+        new_size > kMaxInt) {
+      return Handle<JSArrayBuffer>::null();
+    }
+    isolate->array_buffer_allocator()->SetProtection(
+        old_mem_start, new_size,
+        v8::ArrayBuffer::Allocator::Protection::kReadWrite);
+    reinterpret_cast<v8::Isolate*>(isolate)
+        ->AdjustAmountOfExternalAllocatedMemory(pages * WasmModule::kPageSize);
+    Handle<Object> length_obj = isolate->factory()->NewNumberFromSize(new_size);
+    old_buffer->set_byte_length(*length_obj);
+    return old_buffer;
+  } else {
+    Handle<JSArrayBuffer> new_buffer;
+    new_buffer = wasm::NewArrayBuffer(isolate, new_size, enable_guard_regions);
+    if (new_buffer.is_null() || old_size == 0) return new_buffer;
+    Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
+    memcpy(new_mem_start, old_mem_start, old_size);
+    return new_buffer;
+  }
 }
 
 // May GC, because SetSpecializationMemInfoFrom may GC
@@ -379,6 +392,32 @@ void WasmMemoryObject::RemoveInstance(Isolate* isolate,
   }
 }
 
+void WasmMemoryObject::SetupNewBufferWithSameBackingStore(
+    Isolate* isolate, Handle<WasmMemoryObject> memory_object, uint32_t size) {
+  // In case of Memory.Grow(0), or Memory.Grow(delta) with guard pages enabled,
+  // Setup a new buffer, update memory object, and instances associated with the
+  // memory object, as the current buffer will be detached.
+  Handle<JSArrayBuffer> old_buffer(memory_object->array_buffer());
+  Handle<JSArrayBuffer> new_buffer;
+
+  constexpr bool is_external = false;
+  new_buffer = wasm::SetupArrayBuffer(
+      isolate, old_buffer->allocation_base(), old_buffer->allocation_length(),
+      old_buffer->backing_store(), size * WasmModule::kPageSize, is_external,
+      old_buffer->has_guard_region());
+  if (memory_object->has_instances()) {
+    Handle<WeakFixedArray> instances(memory_object->instances(), isolate);
+    for (int i = 0; i < instances->Length(); i++) {
+      Object* elem = instances->Get(i);
+      if (!elem->IsWasmInstanceObject()) continue;
+      Handle<WasmInstanceObject> instance(WasmInstanceObject::cast(elem),
+                                          isolate);
+      SetInstanceMemory(isolate, instance, new_buffer);
+    }
+  }
+  memory_object->set_array_buffer(*new_buffer);
+}
+
 // static
 int32_t WasmMemoryObject::Grow(Isolate* isolate,
                                Handle<WasmMemoryObject> memory_object,
@@ -389,13 +428,6 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
   Handle<JSArrayBuffer> new_buffer;
   // Return current size if grow by 0.
   if (pages == 0) {
-    // 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.
-    new_buffer = wasm::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_array_buffer(*new_buffer);
     DCHECK_EQ(0, old_size % WasmModule::kPageSize);
     return old_size / WasmModule::kPageSize;
   }
@@ -422,7 +454,6 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
       UncheckedUpdateInstanceMemory(isolate, instance, old_mem_start, old_size);
     }
   }
-
   memory_object->set_array_buffer(*new_buffer);
   DCHECK_EQ(0, old_size % WasmModule::kPageSize);
   return old_size / WasmModule::kPageSize;

src/wasm/wasm-objects.h

@@ -137,6 +137,8 @@ class WasmMemoryObject : public JSObject {
                                       int32_t maximum);
 
   static int32_t Grow(Isolate*, Handle<WasmMemoryObject>, uint32_t pages);
+  static void SetupNewBufferWithSameBackingStore(
+      Isolate* isolate, Handle<WasmMemoryObject> memory_object, uint32_t size);
 };
 
 // A WebAssembly.Instance JavaScript-level object.

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

@@ -1105,7 +1105,13 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
     v8::Utils::ToLocal(memory)->Externalize();
 
     uint32_t result = WasmMemoryObject::Grow(isolate, mem_obj, 4);
-    const bool free_memory = true;
+    bool free_memory = !memory->has_guard_region();
+    if (!free_memory) {
+      // current_pages = Initial memory size(16) + GrowWebAssemblyMemory(4)
+      const uint32_t current_pages = 20;
+      i::WasmMemoryObject::SetupNewBufferWithSameBackingStore(isolate, mem_obj,
+                                                              current_pages);
+    }
     wasm::DetachWebAssemblyMemoryBuffer(isolate, memory, free_memory);
     CHECK_EQ(16, result);
     memory = handle(mem_obj->array_buffer());
@@ -1122,8 +1128,12 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
     isolate->array_buffer_allocator()->Free(memory->allocation_base(),
                                             memory->allocation_length(),
                                             allocation_mode);
-    isolate->array_buffer_allocator()->Free(
-        old_allocation_base, old_allocation_length, allocation_mode);
+    if (free_memory) {
+      // GrowMemory without guard pages enabled allocates an extra buffer,
+      // that needs to be freed as well
+      isolate->array_buffer_allocator()->Free(
+          old_allocation_base, old_allocation_length, allocation_mode);
+    }
     memory->set_allocation_base(nullptr);
     memory->set_allocation_length(0);
   }