[wasm] Detach memory buffer only when GrowMemory is called from the JS API

src/wasm/wasm-module.cc

Index: src/wasm/wasm-module.cc
diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc
index fbe9a91c1a130ead3b125dd1b5f1e5b21fe31510..35e4a2e7b615b0fc0e2d9dc77e4a495250f42622 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -2349,7 +2349,8 @@ Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
   Handle<JSArrayBuffer> old_buffer;
   Address old_mem_start = nullptr;
   uint32_t old_size = 0;
-  if (buffer.ToHandle(&old_buffer) && old_buffer->backing_store() != nullptr) {
+  if (buffer.ToHandle(&old_buffer) && old_buffer->backing_store() != nullptr &&
+      old_buffer->byte_length()->IsNumber()) {

[bradnelson, 2017/03/24 03:49:46]

Why is it ok to keep going below if length has been messed with?

[ahaas, 2017/03/24 08:26:00]

Could you add a comment why this IsNumber check is necessary? 

[gdeepti, 2017/03/24 16:13:19]

GrowWebAssemblyMemory can be called through JS, i.e. with an ArrayBuffer with
size 0/NaN. In the case of byte_length = NaN, on some platforms the IsNumber()
check which is also a part of the Number() function fails, and causes a crash,
on other platforms it returns 0. If the IsNumber check fails, the byte_length is
forced to 0 (above), and Memory is Grown as if from no memory. This is mostly to
iron out platform specific behavior in terms of NaNs/Buffers with byte_length =
0, added comment in code as well. Same answer for question below. 

     old_mem_start = static_cast<Address>(old_buffer->backing_store());
     DCHECK_NOT_NULL(old_mem_start);
     old_size = old_buffer->byte_length()->Number();
@@ -2392,28 +2393,30 @@ void UncheckedUpdateInstanceMemory(Isolate* isolate,
   code_specialization.ApplyToWholeInstance(*instance);
 }
 
-void DetachArrayBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer) {
-  const bool has_guard_regions =
-      (!buffer.is_null() && buffer->has_guard_region());
+void wasm::DetachWebAssemblyMemoryBuffer(Isolate* isolate,
+                                         Handle<JSArrayBuffer> buffer) {
+  int64_t byte_length =
+      buffer->byte_length()->IsNumber()
+          ? 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();
-  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 && !is_external) {
-      buffer->set_is_external(true);
-      isolate->heap()->UnregisterArrayBuffer(*buffer);
-    }
-    buffer->Neuter();
-    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);
-    }
+  DCHECK(!buffer->is_neuterable());
+  if (!has_guard_regions && !is_external) {
+    buffer->set_is_external(true);
+    isolate->heap()->UnregisterArrayBuffer(*buffer);
+  }
+  buffer->set_is_neuterable(true);
+  buffer->Neuter();
+  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);
   }
 }
 
@@ -2428,7 +2431,8 @@ int32_t wasm::GrowWebAssemblyMemory(Isolate* isolate,
   uint32_t old_size = 0;
   Address old_mem_start = nullptr;
   if (memory_buffer.ToHandle(&old_buffer) &&
-      old_buffer->backing_store() != nullptr) {
+      old_buffer->backing_store() != nullptr &&
+      old_buffer->byte_length()->IsNumber()) {

[bradnelson, 2017/03/24 03:49:46]

Same?

     old_size = old_buffer->byte_length()->Number();
     old_mem_start = static_cast<Address>(old_buffer->backing_store());
   }
@@ -2488,7 +2492,6 @@ int32_t wasm::GrowWebAssemblyMemory(Isolate* isolate,
     }
   }
   memory_object->set_buffer(*new_buffer);
-  DetachArrayBuffer(isolate, old_buffer);
   DCHECK(old_size % WasmModule::kPageSize == 0);
   return (old_size / WasmModule::kPageSize);
 }