[wasm] Add guard regions to end of WebAssembly.Memory buffers

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 4aa5445b49bc537b92e1f0bf7cb574e258e013ad..f481e353a07a9239e36d9cd7300816530e4b083e 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -86,28 +86,59 @@ void ReplaceReferenceInCode(Handle<Code> code, Handle<Object> old_ref,
   }
 }
 
-Handle<JSArrayBuffer> NewArrayBuffer(Isolate* isolate, size_t size) {
-  if (size > (WasmModule::kMaxMemPages * WasmModule::kPageSize)) {
-    // TODO(titzer): lift restriction on maximum memory allocated here.
-    return Handle<JSArrayBuffer>::null();
-  }
-  void* memory = isolate->array_buffer_allocator()->Allocate(size);
-  if (memory == nullptr) {
-    return Handle<JSArrayBuffer>::null();
-  }
+#if V8_HOST_ARCH_64_BIT
+static void MemoryFinalizer(const v8::WeakCallbackInfo<void>& data) {
+  JSArrayBuffer** p = reinterpret_cast<JSArrayBuffer**>(data.GetParameter());
+  JSArrayBuffer* buffer = *p;
 
-#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]);
-  }
+  void* memory = buffer->backing_store();
+  base::OS::Free(memory, kWasmMaxHeapOffset);
+
+  data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(
+      -buffer->byte_length()->Number());
+
+  GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
+}
 #endif
 
-  Handle<JSArrayBuffer> buffer = isolate->factory()->NewJSArrayBuffer();
-  JSArrayBuffer::Setup(buffer, isolate, false, memory, static_cast<int>(size));
-  buffer->set_is_neuterable(false);
-  return buffer;
+void* AllocateBackingStore(Isolate* isolate, size_t size, bool guard) {
+  if (guard) {
+#if V8_HOST_ARCH_64_BIT
+    void* memory;
+    // TODO(eholk): On Windows we want to make sure we don't commit the guard
+    // pages yet.
+
+    // We always allocate the largest possible offset into the heap, so the
+    // addressable memory after the guard page can be make inaccessible.

[Mircea Trofin, 2016/10/28 22:16:29]

can be made

[Eric Holk, 2016/10/29 00:04:30]

Done.

+    const size_t alloc_size = kWasmMaxHeapOffset;
+    DCHECK(size % base::OS::CommitPageSize() == 0);

[Mircea Trofin, 2016/10/28 22:16:29]

DCHECK_EQ(0, size % etc)

[Eric Holk, 2016/10/29 00:04:30]

Done.

+
+    size_t allocated_size = 0;
+    const bool is_executable = false;
+    memory = base::OS::Allocate(alloc_size, &allocated_size, is_executable);
+    if (allocated_size < alloc_size) {
+      base::OS::Free(memory, allocated_size);
+      return nullptr;

[Mircea Trofin, 2016/10/28 22:16:29]

because it may fail, perhaps name the API TryAllocateBackingStore

[Eric Holk, 2016/10/29 00:04:30]

Done.

+    }
+
+    if (memory == nullptr) {
+      return nullptr;
+    }
+
+    byte* bytes = reinterpret_cast<byte*>(memory);
+    base::OS::Guard(bytes + size, alloc_size - size);
+
+    reinterpret_cast<v8::Isolate*>(isolate)
+        ->AdjustAmountOfExternalAllocatedMemory(size);
+
+    return memory;
+#else
+    DCHECK(false && "Guard pages are not supported on 32-bit systems");
+#endif
+  } else {
+    void* memory = isolate->array_buffer_allocator()->Allocate(size);
+    return memory;
+  }
 }
 
 void RelocateMemoryReferencesInCode(Handle<FixedArray> code_table,
@@ -603,6 +634,50 @@ static void InstanceFinalizer(const v8::WeakCallbackInfo<void>& data) {
 
 }  // namespace
 
+Handle<JSArrayBuffer> wasm::NewArrayBuffer(Isolate* isolate, size_t size,
+                                           bool guard) {

[Mircea Trofin, 2016/10/28 22:16:29]

guard->enable_guard_regions

also, if this flag is set, is it guaranteed you get guard regions?

[Eric Holk, 2016/10/29 00:04:30]

Done.

Yes (at least on 64-bit platforms)

+  if (size > (WasmModule::kMaxMemPages * WasmModule::kPageSize)) {
+    // TODO(titzer): lift restriction on maximum memory allocated here.
+    return Handle<JSArrayBuffer>::null();
+  }
+
+  void* memory = AllocateBackingStore(isolate, size, guard);
+  bool is_external = guard;
+
+  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(guard);
+
+#if V8_HOST_ARCH_64_BIT
+  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);
+  }
+#endif
+
+  return buffer;
+}
+
 const char* wasm::SectionName(WasmSectionCode code) {
   switch (code) {
     case kUnknownSectionCode:
@@ -954,8 +1029,9 @@ class WasmInstanceBuilder {
     MaybeHandle<JSArrayBuffer> globals;
     uint32_t globals_size = module_->globals_size;
     if (globals_size > 0) {
+      const bool guard = false;
       Handle<JSArrayBuffer> global_buffer =
-          NewArrayBuffer(isolate_, globals_size);
+          NewArrayBuffer(isolate_, globals_size, guard);
       globals = global_buffer;
       if (globals.is_null()) {
         thrower_->RangeError("Out of memory: wasm globals");
@@ -994,6 +1070,9 @@ class WasmInstanceBuilder {
     if (!memory_.is_null()) {
       // Set externally passed ArrayBuffer non neuterable.
       memory_->set_is_neuterable(false);
+
+      DCHECK_IMPLIES(FLAG_wasm_guard_pages, module_->origin == kAsmJsOrigin ||

[Mircea Trofin, 2016/10/28 22:16:29]

asmjs, not wasm?

[Eric Holk, 2016/10/29 00:04:30]

Yes. For AsmJs origin we still do the bounds checking and therefore don't need
guard pages. Some of our tests have buffers that aren't page size multiples, so
we can't add a guard region to those.

This check is making sure that if we accept an externally-allocated memory then
it was allocated with guard pages. The way this happens now is that the
allocator for WebAssembly.Memory sets up the guard pages, and using some other
object would be a TypeError.

+                                                memory_->has_guard_region());
     } else if (min_mem_pages > 0) {
       memory_ = AllocateMemory(min_mem_pages);
       if (memory_.is_null()) return nothing;  // failed to allocate memory
@@ -1508,8 +1587,9 @@ class WasmInstanceBuilder {
       thrower_->RangeError("Out of memory: wasm memory too large");
       return Handle<JSArrayBuffer>::null();
     }
+    const bool guard = FLAG_wasm_guard_pages;
     Handle<JSArrayBuffer> mem_buffer =
-        NewArrayBuffer(isolate_, min_mem_pages * WasmModule::kPageSize);
+        NewArrayBuffer(isolate_, min_mem_pages * WasmModule::kPageSize, guard);
 
     if (mem_buffer.is_null()) {
       thrower_->RangeError("Out of memory: wasm memory");
@@ -2000,16 +2080,36 @@ int32_t wasm::GrowInstanceMemory(Isolate* isolate, Handle<JSObject> instance,
   if (new_size <= old_size || max_pages * WasmModule::kPageSize < new_size) {
     return -1;
   }
-  Handle<JSArrayBuffer> buffer = NewArrayBuffer(isolate, new_size);
-  if (buffer.is_null()) return -1;
-  Address new_mem_start = static_cast<Address>(buffer->backing_store());
-  if (old_size != 0) {
-    memcpy(new_mem_start, old_mem_start, old_size);
+
+  Handle<JSArrayBuffer> buffer;
+
+  if (FLAG_wasm_guard_pages && !old_buffer.is_null()) {
+    base::OS::Unprotect(old_buffer->backing_store(), new_size);
+    reinterpret_cast<v8::Isolate*>(isolate)
+        ->AdjustAmountOfExternalAllocatedMemory(pages * WasmModule::kPageSize);

[Mircea Trofin, 2016/10/28 22:16:29]

would it make sense to add this API to the internal isolate?

[Eric Holk, 2016/10/29 00:04:30]

Perhaps, but I'd rather leave it how it is.
AdjustAmountOfExternalAllocatedMemory accesses fields in the internal
embedder_data_ field, and there doesn't seem to be a lot of precedent for
accessing this through the internal API.

[Michael Lippautz, 2016/10/29 07:39:19]

+1 for leaving now. This API is used by several consumers (internal and
external) and we have soft and hard limits in place that tell us when to do
which GC. So, unless you want to hunt down memory issues later on, just leave it
as is.

+    Handle<Object> new_size_object =
+        isolate->factory()->NewNumberFromSize(new_size);
+    old_buffer->set_byte_length(*new_size_object);
+
+    SetInstanceMemory(instance, *old_buffer);
+    Handle<FixedArray> code_table = GetCompiledModule(*instance)->code_table();
+    RelocateMemoryReferencesInCode(code_table, old_mem_start, old_mem_start,
+                                   old_size, new_size);
+    buffer = old_buffer;
+  } else {
+    const bool guard = FLAG_wasm_guard_pages;
+    buffer = NewArrayBuffer(isolate, new_size, guard);
+    if (buffer.is_null()) return -1;
+    Address new_mem_start = static_cast<Address>(buffer->backing_store());
+    if (old_size != 0) {
+      memcpy(new_mem_start, old_mem_start, old_size);
+    }
+    SetInstanceMemory(instance, *buffer);
+    Handle<FixedArray> code_table = GetCompiledModule(*instance)->code_table();
+    RelocateMemoryReferencesInCode(code_table, old_mem_start, new_mem_start,
+                                   old_size, new_size);
   }
-  SetInstanceMemory(instance, *buffer);
-  Handle<FixedArray> code_table = GetCompiledModule(*instance)->code_table();
-  RelocateMemoryReferencesInCode(code_table, old_mem_start, new_mem_start,
-                                 old_size, new_size);
+
   Handle<Object> memory_object(instance->GetInternalField(kWasmMemObject),
                                isolate);
   if (!memory_object->IsUndefined(isolate)) {