[api] Add possibility for BackingStore to keep Allocator alive

Add an `array_buffer_allocator_shared` field to the
`Isolate::CreateParams` struct that allows embedders to share
ownership of the ArrayBuffer::Allocator with V8, and which in
particular means that when this method is used that the
BackingStore deleter will not perform an use-after-free access to the
Allocator under certain circumstances.

For Background:

tl;dr: This is necessary for Node.js to perform the transition to
V8 7.9, because of the way that ArrayBuffer::Allocators and their
lifetimes currently work there.

In Node.js, each Worker thread has its own ArrayBuffer::Allocator.
Changing that would currently be impractical, as each allocator
depends on per-Isolate state. However, now that backing stores
are managed globally and keep a pointer to the original
ArrayBuffer::Allocator, this means that when transferring an
ArrayBuffer (e.g. from one Worker to another through postMessage()),
the original Allocator has to be kept alive until the ArrayBuffer
no longer exists in the receiving Isolate (or until that Isolate
is disposed). See [1] for an example Node.js test that fails with
V8 7.9.

This problem also existed for SharedArrayBuffers, where Node.js
was broken by V8 earlier for the same reasons (see [2] for the bug
report on that and [3] for the resolution in Node.js).
For SharedArrayBuffers, we already had extensive tracking logic,
so adding a shared_ptr to keep alive the ArrayBuffer::Allocator
was not a significant amount of work. However, the mechanism for
transferring non-shared ArrayBuffers is quite different, and
it seems both easier for us and better for V8 from an API standpoint
to keep the Allocator alive from where it is being referenced.

By sharing memory with the custom deleter function/data pair,
this comes at no memory overhead.

[1]: https://github.com/nodejs/node/pull/30044
[2]: https://github.com/nodejs/node-v8/issues/115
[3]: https://github.com/nodejs/node/pull/29637

Bug: v8:9380
Change-Id: Ibc2c4fb6341b53653cbd637bd8cb3d4ac43809c7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1874347
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64542}

include/v8.h

@@ -4851,6 +4851,10 @@ enum class ArrayBufferCreationMode { kInternalized, kExternalized };
  * V8. Clients should always use standard C++ memory ownership types (i.e.
  * std::unique_ptr and std::shared_ptr) to manage lifetimes of backing stores
  * properly, since V8 internal objects may alias backing stores.
+ *
+ * This object does not keep the underlying |ArrayBuffer::Allocator| alive by
+ * default. Use Isolate::CreateParams::array_buffer_allocator_shared when
+ * creating the Isolate to make it hold a reference to the allocator itself.
  */
 class V8_EXPORT BackingStore : public v8::internal::BackingStoreBase {
  public:
@@ -7902,6 +7906,7 @@ class V8_EXPORT Isolate {
           create_histogram_callback(nullptr),
           add_histogram_sample_callback(nullptr),
           array_buffer_allocator(nullptr),
+          array_buffer_allocator_shared(),
           external_references(nullptr),
           allow_atomics_wait(true),
           only_terminate_in_safe_scope(false) {}
@@ -7941,8 +7946,14 @@ class V8_EXPORT Isolate {
     /**
      * The ArrayBuffer::Allocator to use for allocating and freeing the backing
      * store of ArrayBuffers.
+     *
+     * If the shared_ptr version is used, the Isolate instance and every
+     * |BackingStore| allocated using this allocator hold a std::shared_ptr
+     * to the allocator, in order to facilitate lifetime
+     * management for the allocator instance.
      */
     ArrayBuffer::Allocator* array_buffer_allocator;
+    std::shared_ptr<ArrayBuffer::Allocator> array_buffer_allocator_shared;
 
     /**
      * Specifies an optional nullptr-terminated array of raw addresses in the

src/api/api.cc

@@ -8198,8 +8198,15 @@ Isolate* Isolate::Allocate() {
 void Isolate::Initialize(Isolate* isolate,
                          const v8::Isolate::CreateParams& params) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
-  CHECK_NOT_NULL(params.array_buffer_allocator);
-  i_isolate->set_array_buffer_allocator(params.array_buffer_allocator);
+  if (auto allocator = params.array_buffer_allocator_shared) {
+    CHECK(params.array_buffer_allocator == nullptr ||
+          params.array_buffer_allocator == allocator.get());
+    i_isolate->set_array_buffer_allocator(allocator.get());
+    i_isolate->set_array_buffer_allocator_shared(std::move(allocator));
+  } else {
+    CHECK_NOT_NULL(params.array_buffer_allocator);
+    i_isolate->set_array_buffer_allocator(params.array_buffer_allocator);
+  }
   if (params.snapshot_blob != nullptr) {
     i_isolate->set_snapshot_blob(params.snapshot_blob);
   } else {

src/execution/isolate.h

@@ -1344,6 +1344,15 @@ class Isolate final : private HiddenFactory {
     return array_buffer_allocator_;
   }
 
+  void set_array_buffer_allocator_shared(
+      std::shared_ptr<v8::ArrayBuffer::Allocator> allocator) {
+    array_buffer_allocator_shared_ = std::move(allocator);
+  }
+  std::shared_ptr<v8::ArrayBuffer::Allocator> array_buffer_allocator_shared()
+      const {
+    return array_buffer_allocator_shared_;
+  }
+
   FutexWaitListNode* futex_wait_list_node() { return &futex_wait_list_node_; }
 
   CancelableTaskManager* cancelable_task_manager() {
@@ -1755,6 +1764,7 @@ class Isolate final : private HiddenFactory {
   uint32_t embedded_blob_size_ = 0;
 
   v8::ArrayBuffer::Allocator* array_buffer_allocator_ = nullptr;
+  std::shared_ptr<v8::ArrayBuffer::Allocator> array_buffer_allocator_shared_;
 
   FutexWaitListNode futex_wait_list_node_;
 

src/objects/backing-store.cc

@@ -114,6 +114,11 @@ void BackingStore::Clear() {
   buffer_start_ = nullptr;
   byte_length_ = 0;
   has_guard_regions_ = false;
+  if (holds_shared_ptr_to_allocator_) {
+    type_specific_data_.v8_api_array_buffer_allocator_shared
+        .std::shared_ptr<v8::ArrayBuffer::Allocator>::~shared_ptr();
+    holds_shared_ptr_to_allocator_ = false;
+  }
   type_specific_data_.v8_api_array_buffer_allocator = nullptr;
 }
 
@@ -154,14 +159,14 @@ BackingStore::~BackingStore() {
     DCHECK(free_on_destruct_);
     TRACE_BS("BS:custome deleter bs=%p mem=%p (length=%zu, capacity=%zu)\n",
              this, buffer_start_, byte_length(), byte_capacity_);
-    type_specific_data_.deleter(buffer_start_, byte_length_, deleter_data_);
+    type_specific_data_.deleter.callback(buffer_start_, byte_length_,
+                                         type_specific_data_.deleter.data);
     Clear();
     return;
   }
   if (free_on_destruct_) {
     // JSArrayBuffer backing store. Deallocate through the embedder's allocator.
-    auto allocator = reinterpret_cast<v8::ArrayBuffer::Allocator*>(
-        get_v8_api_array_buffer_allocator());
+    auto allocator = get_v8_api_array_buffer_allocator();
     TRACE_BS("BS:free   bs=%p mem=%p (length=%zu, capacity=%zu)\n", this,
              buffer_start_, byte_length(), byte_capacity_);
     allocator->Free(buffer_start_, byte_length_);
@@ -224,10 +229,22 @@ std::unique_ptr<BackingStore> BackingStore::Allocate(
 
   TRACE_BS("BS:alloc  bs=%p mem=%p (length=%zu)\n", result,
            result->buffer_start(), byte_length);
-  result->type_specific_data_.v8_api_array_buffer_allocator = allocator;
+  result->SetAllocatorFromIsolate(isolate);
   return std::unique_ptr<BackingStore>(result);
 }
 
+void BackingStore::SetAllocatorFromIsolate(Isolate* isolate) {
+  if (auto allocator_shared = isolate->array_buffer_allocator_shared()) {
+    holds_shared_ptr_to_allocator_ = true;
+    new (&type_specific_data_.v8_api_array_buffer_allocator_shared)
+        std::shared_ptr<v8::ArrayBuffer::Allocator>(
+            std::move(allocator_shared));
+  } else {
+    type_specific_data_.v8_api_array_buffer_allocator =
+        isolate->array_buffer_allocator();
+  }
+}
+
 // Allocate a backing store for a Wasm memory. Always use the page allocator
 // and add guard regions.
 std::unique_ptr<BackingStore> BackingStore::TryAllocateWasmMemory(
@@ -470,8 +487,7 @@ std::unique_ptr<BackingStore> BackingStore::WrapAllocation(
                                  free_on_destruct,   // free_on_destruct
                                  false,              // has_guard_regions
                                  false);             // custom_deleter
-  result->type_specific_data_.v8_api_array_buffer_allocator =
-      isolate->array_buffer_allocator();
+  result->SetAllocatorFromIsolate(isolate);
   TRACE_BS("BS:wrap   bs=%p mem=%p (length=%zu)\n", result,
            result->buffer_start(), result->byte_length());
   return std::unique_ptr<BackingStore>(result);
@@ -489,8 +505,7 @@ std::unique_ptr<BackingStore> BackingStore::WrapAllocation(
                                  true,               // free_on_destruct
                                  false,              // has_guard_regions
                                  true);              // custom_deleter
-  result->type_specific_data_.deleter = deleter;
-  result->deleter_data_ = deleter_data;
+  result->type_specific_data_.deleter = {deleter, deleter_data};
   TRACE_BS("BS:wrap   bs=%p mem=%p (length=%zu)\n", result,
            result->buffer_start(), result->byte_length());
   return std::unique_ptr<BackingStore>(result);
@@ -510,10 +525,12 @@ std::unique_ptr<BackingStore> BackingStore::EmptyBackingStore(
   return std::unique_ptr<BackingStore>(result);
 }
 
-void* BackingStore::get_v8_api_array_buffer_allocator() {
+v8::ArrayBuffer::Allocator* BackingStore::get_v8_api_array_buffer_allocator() {
   CHECK(!is_wasm_memory_);
   auto array_buffer_allocator =
-      type_specific_data_.v8_api_array_buffer_allocator;
+      holds_shared_ptr_to_allocator_
+          ? type_specific_data_.v8_api_array_buffer_allocator_shared.get()
+          : type_specific_data_.v8_api_array_buffer_allocator;
   CHECK_NOT_NULL(array_buffer_allocator);
   return array_buffer_allocator;
 }

src/objects/backing-store.h

@@ -128,24 +128,36 @@ class V8_EXPORT_PRIVATE BackingStore : public BackingStoreBase {
         byte_capacity_(byte_capacity),
         is_shared_(shared == SharedFlag::kShared),
         is_wasm_memory_(is_wasm_memory),
+        holds_shared_ptr_to_allocator_(false),
         free_on_destruct_(free_on_destruct),
         has_guard_regions_(has_guard_regions),
         globally_registered_(false),
-        custom_deleter_(custom_deleter) {
-    type_specific_data_.v8_api_array_buffer_allocator = nullptr;
-    deleter_data_ = nullptr;
-  }
+        custom_deleter_(custom_deleter) {}
+  void SetAllocatorFromIsolate(Isolate* isolate);
 
   void* buffer_start_ = nullptr;
   std::atomic<size_t> byte_length_{0};
   size_t byte_capacity_ = 0;
-  union {
+
+  struct DeleterInfo {
+    v8::BackingStoreDeleterCallback callback;
+    void* data;
+  };
+
+  union TypeSpecificData {
+    TypeSpecificData() : v8_api_array_buffer_allocator(nullptr) {}
+    ~TypeSpecificData() {}
+
     // If this backing store was allocated through the ArrayBufferAllocator API,
     // this is a direct pointer to the API object for freeing the backing
     // store.
-    // Note: we use {void*} here because we cannot forward-declare an inner
-    // class from the API.
-    void* v8_api_array_buffer_allocator;
+    v8::ArrayBuffer::Allocator* v8_api_array_buffer_allocator;
+
+    // Holds a shared_ptr to the ArrayBuffer::Allocator instance, if requested
+    // so by the embedder through setting
+    // Isolate::CreateParams::array_buffer_allocator_shared.
+    std::shared_ptr<v8::ArrayBuffer::Allocator>
+        v8_api_array_buffer_allocator_shared;
 
     // For shared Wasm memories, this is a list of all the attached memory
     // objects, which is needed to grow shared backing stores.
@@ -153,20 +165,19 @@ class V8_EXPORT_PRIVATE BackingStore : public BackingStoreBase {
 
     // Custom deleter for the backing stores that wrap memory blocks that are
     // allocated with a custom allocator.
-    v8::BackingStoreDeleterCallback deleter;
+    DeleterInfo deleter;
   } type_specific_data_;
 
-  void* deleter_data_;
-
   bool is_shared_ : 1;
   bool is_wasm_memory_ : 1;
+  bool holds_shared_ptr_to_allocator_ : 1;
   bool free_on_destruct_ : 1;
   bool has_guard_regions_ : 1;
   bool globally_registered_ : 1;
   bool custom_deleter_ : 1;
 
   // Accessors for type-specific data.
-  void* get_v8_api_array_buffer_allocator();
+  v8::ArrayBuffer::Allocator* get_v8_api_array_buffer_allocator();
   SharedWasmMemoryData* get_shared_wasm_memory_data();
 
   void Clear();  // Internally clears fields after deallocation.

test/cctest/test-api-array-buffer.cc

@@ -642,3 +642,95 @@ THREADED_TEST(BackingStore_Shared) {
             reinterpret_cast<void*>(backing_store_custom_deleter_data))
             ->IsShared());
 }
+
+class DummyAllocator final : public v8::ArrayBuffer::Allocator {
+ public:
+  DummyAllocator() : allocator_(NewDefaultAllocator()) {}
+
+  ~DummyAllocator() override { CHECK_EQ(allocation_count(), 0); }
+
+  void* Allocate(size_t length) override {
+    allocation_count_++;
+    return allocator_->Allocate(length);
+  }
+  void* AllocateUninitialized(size_t length) override {
+    allocation_count_++;
+    return allocator_->AllocateUninitialized(length);
+  }
+  void Free(void* data, size_t length) override {
+    allocation_count_--;
+    allocator_->Free(data, length);
+  }
+
+  uint64_t allocation_count() const { return allocation_count_; }
+
+ private:
+  std::unique_ptr<v8::ArrayBuffer::Allocator> allocator_;
+  uint64_t allocation_count_ = 0;
+};
+
+TEST(BackingStore_HoldAllocatorAlive_UntilIsolateShutdown) {
+  std::shared_ptr<DummyAllocator> allocator =
+      std::make_shared<DummyAllocator>();
+  std::weak_ptr<DummyAllocator> allocator_weak(allocator);
+
+  v8::Isolate::CreateParams create_params;
+  create_params.array_buffer_allocator_shared = allocator;
+  v8::Isolate* isolate = v8::Isolate::New(create_params);
+  isolate->Enter();
+
+  allocator.reset();
+  create_params.array_buffer_allocator_shared.reset();
+  CHECK(!allocator_weak.expired());
+  CHECK_EQ(allocator_weak.lock()->allocation_count(), 0);
+
+  {
+    // Create an ArrayBuffer and do not garbage collect it. This should make
+    // the allocator be released automatically once the Isolate is disposed.
+    v8::HandleScope handle_scope(isolate);
+    v8::Context::Scope context_scope(Context::New(isolate));
+    v8::ArrayBuffer::New(isolate, 8);
+
+    // This should be inside the HandleScope, so that we can be sure that
+    // the allocation is not garbage collected yet.
+    CHECK(!allocator_weak.expired());
+    CHECK_EQ(allocator_weak.lock()->allocation_count(), 1);
+  }
+
+  isolate->Exit();
+  isolate->Dispose();
+  CHECK(allocator_weak.expired());
+}
+
+TEST(BackingStore_HoldAllocatorAlive_AfterIsolateShutdown) {
+  std::shared_ptr<DummyAllocator> allocator =
+      std::make_shared<DummyAllocator>();
+  std::weak_ptr<DummyAllocator> allocator_weak(allocator);
+
+  v8::Isolate::CreateParams create_params;
+  create_params.array_buffer_allocator_shared = allocator;
+  v8::Isolate* isolate = v8::Isolate::New(create_params);
+  isolate->Enter();
+
+  allocator.reset();
+  create_params.array_buffer_allocator_shared.reset();
+  CHECK(!allocator_weak.expired());
+  CHECK_EQ(allocator_weak.lock()->allocation_count(), 0);
+
+  std::shared_ptr<v8::BackingStore> backing_store;
+  {
+    // Create an ArrayBuffer and do not garbage collect it. This should make
+    // the allocator be released automatically once the Isolate is disposed.
+    v8::HandleScope handle_scope(isolate);
+    v8::Context::Scope context_scope(Context::New(isolate));
+    v8::Local<v8::ArrayBuffer> ab = v8::ArrayBuffer::New(isolate, 8);
+    backing_store = ab->GetBackingStore();
+  }
+
+  isolate->Exit();
+  isolate->Dispose();
+  CHECK(!allocator_weak.expired());
+  CHECK_EQ(allocator_weak.lock()->allocation_count(), 1);
+  backing_store.reset();
+  CHECK(allocator_weak.expired());
+}