From 1fce7d604a4919f84b8d21ef11b6206650542b7b Mon Sep 17 00:00:00 2001
From: gdeepti <gdeepti@chromium.org>
Date: Thu, 16 Mar 2017 18:12:07 -0700
Subject: [PATCH] [wasm] Fix DetachArrayBuffer for WebAssembly.Memory on grow

DetachArrayBuffer makes incorrect assumptions about the state of the ArrayBuffer. It assumes that that the ArrayBuffer is internal to wasm unless guard pages are enabled, this is not the case as the ArrayBuffer can be externalized outside of wasm, in this case through gin.

BUG=chromium:700384

Review-Url: https://codereview.chromium.org/2754153002
Cr-Commit-Position: refs/heads/master@{#43880}
---
 src/wasm/wasm-module.cc                  |  9 ++--
 test/cctest/wasm/test-run-wasm-module.cc | 65 ++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc
index dbacfb85dc..8a3dd5d1d3 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -2211,23 +2211,24 @@ void UncheckedUpdateInstanceMemory(Isolate* isolate,
 void DetachArrayBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer) {
   const bool has_guard_regions =
       (!buffer.is_null() && buffer->has_guard_region());
+  const bool is_external = buffer->is_external();
   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) {
+    if (!has_guard_regions && !is_external) {
       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 {
+    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);
     }
   }
 }
diff --git a/test/cctest/wasm/test-run-wasm-module.cc b/test/cctest/wasm/test-run-wasm-module.cc
index a3dd35713d..a6c4d177b7 100644
--- a/test/cctest/wasm/test-run-wasm-module.cc
+++ b/test/cctest/wasm/test-run-wasm-module.cc
@@ -974,3 +974,68 @@ TEST(MemoryWithOOBEmptyDataSegment) {
   }
   Cleanup();
 }
+
+TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
+  {
+    Isolate* isolate = CcTest::InitIsolateOnce();
+    HandleScope scope(isolate);
+    // Initial memory size = 16 + GrowWebAssemblyMemory(4) + GrowMemory(6)
+    static const int kExpectedValue = 26;
+    TestSignatures sigs;
+    v8::internal::AccountingAllocator allocator;
+    Zone zone(&allocator, ZONE_NAME);
+
+    WasmModuleBuilder* builder = new (&zone) WasmModuleBuilder(&zone);
+    WasmFunctionBuilder* f = builder->AddFunction(sigs.i_v());
+    ExportAsMain(f);
+    byte code[] = {WASM_GROW_MEMORY(WASM_I32V_1(6)), WASM_DROP,
+                   WASM_MEMORY_SIZE};
+    EMIT_CODE_WITH_END(f, code);
+
+    ZoneBuffer buffer(&zone);
+    builder->WriteTo(buffer);
+    testing::SetupIsolateForWasmModule(isolate);
+    ErrorThrower thrower(isolate, "Test");
+    const Handle<WasmInstanceObject> instance =
+        testing::CompileInstantiateWasmModuleForTesting(
+            isolate, &thrower, buffer.begin(), buffer.end(),
+            ModuleOrigin::kWasmOrigin);
+    CHECK(!instance.is_null());
+    MaybeHandle<JSArrayBuffer> maybe_memory =
+        GetInstanceMemory(isolate, instance);
+    Handle<JSArrayBuffer> memory = maybe_memory.ToHandleChecked();
+
+    // Fake the Embedder flow by creating a memory object, externalize and grow.
+    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());
+    uint32_t result = GrowWebAssemblyMemory(isolate, mem_obj, 4);
+    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();
+    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()));
+    }
+  }
+  Cleanup();
+}