[wasm] Don't mutate ArrayBuffer sizes for wasm memory.

R=eholk@chromium.org,mlippautz@chromium.org
B=https://bugs.chromium.org/p/chromium/issues/detail?id=775047

Change-Id: Ia3b2f51d6cb4dabbf0f1f9ec78ecb8935775f53a
Reviewed-on: https://chromium-review.googlesource.com/809165
Commit-Queue: Brad Nelson <bradnelson@chromium.org>
Commit-Queue: Eric Holk <eholk@chromium.org>
Reviewed-by: Eric Holk <eholk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50254}

src/objects.cc

@@ -18993,7 +18993,7 @@ void JSArrayBuffer::Setup(Handle<JSArrayBuffer> array_buffer, Isolate* isolate,
   // already been promoted.
   array_buffer->set_backing_store(data);
 
-  array_buffer->set_allocation_base(data);
+  array_buffer->set_allocation_base(allocation_base);
   array_buffer->set_allocation_length(allocation_length);
 
   if (data && !is_external) {

src/wasm/wasm-js.cc

@@ -600,8 +600,10 @@ void WebAssemblyMemory(const v8::FunctionCallbackInfo<v8::Value>& args) {
 
   size_t size = static_cast<size_t>(i::wasm::WasmModule::kPageSize) *
                 static_cast<size_t>(initial);
+  const bool enable_guard_regions =
+      internal::trap_handler::IsTrapHandlerEnabled();
   i::Handle<i::JSArrayBuffer> buffer = i::wasm::NewArrayBuffer(
-      i_isolate, size, internal::trap_handler::IsTrapHandlerEnabled(),
+      i_isolate, size, enable_guard_regions,
       is_shared_memory ? i::SharedFlag::kShared : i::SharedFlag::kNotShared);
   if (buffer.is_null()) {
     thrower.RangeError("could not allocate memory");
@@ -808,15 +810,7 @@ void WebAssemblyMemoryGrow(const v8::FunctionCallbackInfo<v8::Value>& args) {
     return;
   }
   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::DetachMemoryBuffer(i_isolate, old_buffer, free_memory);
   }
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();

src/wasm/wasm-memory.cc

@@ -109,8 +109,8 @@ Handle<JSArrayBuffer> NewArrayBuffer(Isolate* isolate, size_t size,
                           size, is_external, enable_guard_regions, shared);
 }
 
-void DetachMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
-                        bool free_memory) {
+void ExternalizeMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
+                             bool free_memory) {
   const bool is_external = buffer->is_external();
   DCHECK(!buffer->is_neuterable());
   if (!is_external) {
@@ -125,6 +125,13 @@ void DetachMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
       buffer->FreeBackingStore();
     }
   }
+}
+
+void DetachMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
+                        bool free_memory) {
+  DCHECK(!buffer->is_neuterable());
+  ExternalizeMemoryBuffer(isolate, buffer, free_memory);
+  DCHECK(buffer->is_external());
   buffer->set_is_neuterable(true);
   buffer->Neuter();
 }

src/wasm/wasm-memory.h

@@ -22,6 +22,8 @@ Handle<JSArrayBuffer> SetupArrayBuffer(
     void* backing_store, size_t size, bool is_external,
     bool enable_guard_regions, SharedFlag shared = SharedFlag::kNotShared);
 
+void ExternalizeMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
+                             bool free_memory);
 void DetachMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
                         bool free_memory);
 

src/wasm/wasm-objects.cc

@@ -373,26 +373,43 @@ Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
       old_buffer.is_null() ? use_trap_handler : old_buffer->has_guard_region();
   size_t new_size =
       static_cast<size_t>(old_pages + pages) * WasmModule::kPageSize;
-  if (enable_guard_regions && old_size != 0) {
+  if (new_size > FLAG_wasm_max_mem_pages * WasmModule::kPageSize ||
+      new_size > kMaxInt) {
+    return Handle<JSArrayBuffer>::null();
+  }
+  if ((enable_guard_regions || old_size == new_size) && old_size != 0) {
     DCHECK_NOT_NULL(old_buffer->backing_store());
-    if (new_size > FLAG_wasm_max_mem_pages * WasmModule::kPageSize ||
-        new_size > kMaxInt) {
-      return Handle<JSArrayBuffer>::null();
+    if (old_size != new_size) {
+      isolate->array_buffer_allocator()->SetProtection(
+          old_mem_start, new_size,
+          v8::ArrayBuffer::Allocator::Protection::kReadWrite);
+      reinterpret_cast<v8::Isolate*>(isolate)
+          ->AdjustAmountOfExternalAllocatedMemory(pages *
+                                                  WasmModule::kPageSize);
     }
-    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;
+    // NOTE: We must allocate a new array buffer here because the spec
+    // assumes that ArrayBuffers do not change size.
+    void* allocation_base = old_buffer->allocation_base();
+    size_t allocation_length = old_buffer->allocation_length();
+    void* backing_store = old_buffer->backing_store();
+    bool has_guard_region = old_buffer->has_guard_region();
+    bool is_external = old_buffer->is_external();
+    // Disconnect buffer early so GC won't free it.
+    i::wasm::ExternalizeMemoryBuffer(isolate, old_buffer, false);
+    Handle<JSArrayBuffer> new_buffer = wasm::SetupArrayBuffer(
+        isolate, allocation_base, allocation_length, backing_store, new_size,
+        is_external, has_guard_region);
+    return new_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);
+    DCHECK(old_buffer.is_null() || !old_buffer->is_shared());
+    DCHECK(old_buffer.is_null() || !old_buffer->has_guard_region());
+    bool free_memory = pages != 0;
+    i::wasm::ExternalizeMemoryBuffer(isolate, old_buffer, free_memory);
     return new_buffer;
   }
 }
@@ -476,32 +493,6 @@ 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,
@@ -512,8 +503,6 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
   CHECK(old_buffer->byte_length()->ToUint32(&old_size));
   DCHECK_EQ(0, old_size % WasmModule::kPageSize);
   Handle<JSArrayBuffer> new_buffer;
-  // Return current size if grow by 0.
-  if (pages == 0) return old_size / WasmModule::kPageSize;
 
   uint32_t maximum_pages = FLAG_wasm_max_mem_pages;
   if (memory_object->has_maximum_pages()) {

src/wasm/wasm-objects.h

@@ -175,8 +175,6 @@ class WasmMemoryObject : public JSObject {
       Isolate* isolate, MaybeHandle<JSArrayBuffer> buffer, 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

@@ -1096,19 +1096,15 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
 
     uint32_t result = WasmMemoryObject::Grow(isolate, mem_obj, 4);
     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::DetachMemoryBuffer(isolate, memory, free_memory);
     CHECK_EQ(16, result);
     memory = handle(mem_obj->array_buffer());
     instance->memory_object()->set_array_buffer(*memory);
     // Externalize should make no difference without the JS API as in this case
     // the buffer is not detached.
-    v8::Utils::ToLocal(memory)->Externalize();
+    if (!memory->has_guard_region()) {
+      v8::Utils::ToLocal(memory)->Externalize();
+    }
     result = testing::RunWasmModuleForTesting(isolate, instance, 0, nullptr);
     CHECK_EQ(kExpectedValue, result);
     // Free the buffer as the tracker does not know about it.