buffer,n-api: release external buffers from BackingStore callback

Release `Buffer` and `ArrayBuffer` instances that were created through
our addon APIs and have finalizers attached to them only after V8 has
called the deleter callback passed to the `BackingStore`, instead of
relying on our own GC callback(s).

This fixes the following race condition:

1. Addon code allocates pointer P via `malloc`.
2. P is passed into `napi_create_external_buffer` with a finalization
   callback which calls `free(P)`. P is inserted into V8’s global array
   buffer table for tracking.
3. The finalization callback is executed on GC. P is freed and returned
   to the allocator. P is not yet removed from V8’s global array
   buffer table. (!)
4. Addon code attempts to allocate memory once again. The allocator
   returns P, as it is now available.
5. P is passed into `napi_create_external_buffer`. P still has not been
   removed from the v8 global array buffer table.
6. The world ends with `Check failed: result.second`.

Since our API contract is to call the finalizer on the JS thread on
which the `ArrayBuffer` was created, but V8 may call the `BackingStore`
deleter callback on another thread, fixing this requires posting
a task back to the JS thread.

Refs: https://github.com/nodejs/node/issues/32463#issuecomment-625877175
Fixes: https://github.com/nodejs/node/issues/32463

PR-URL: https://github.com/nodejs/node/pull/33321
Reviewed-By: James M Snell <jasnell@gmail.com>

1 files changed, 75 insertions, 65 deletions

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 1ff60ad721..fd20415936 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -69,109 +69,130 @@ using v8::Uint32;
 using v8::Uint32Array;
 using v8::Uint8Array;
 using v8::Value;
-using v8::WeakCallbackInfo;
 
 namespace {
 
 class CallbackInfo {
  public:
-  ~CallbackInfo();
-
-  static inline void Free(char* data, void* hint);
-  static inline CallbackInfo* New(Environment* env,
-                                  Local<ArrayBuffer> object,
-                                  FreeCallback callback,
-                                  char* data,
-                                  void* hint = nullptr);
+  static inline Local<ArrayBuffer> CreateTrackedArrayBuffer(
+      Environment* env,
+      char* data,
+      size_t length,
+      FreeCallback callback,
+      void* hint);
 
   CallbackInfo(const CallbackInfo&) = delete;
   CallbackInfo& operator=(const CallbackInfo&) = delete;
 
  private:
   static void CleanupHook(void* data);
-  static void WeakCallback(const WeakCallbackInfo<CallbackInfo>&);
-  inline void WeakCallback(Isolate* isolate);
+  inline void OnBackingStoreFree();
+  inline void CallAndResetCallback();
   inline CallbackInfo(Environment* env,
-                      Local<ArrayBuffer> object,
                       FreeCallback callback,
                       char* data,
                       void* hint);
   Global<ArrayBuffer> persistent_;
-  FreeCallback const callback_;
+  Mutex mutex_;  // Protects callback_.
+  FreeCallback callback_;
   char* const data_;
   void* const hint_;
   Environment* const env_;
 };
 
 
-void CallbackInfo::Free(char* data, void*) {
-  ::free(data);
-}
-
+Local<ArrayBuffer> CallbackInfo::CreateTrackedArrayBuffer(
+    Environment* env,
+    char* data,
+    size_t length,
+    FreeCallback callback,
+    void* hint) {
+  CHECK_NOT_NULL(callback);
+  CHECK_IMPLIES(data == nullptr, length == 0);
+
+  CallbackInfo* self = new CallbackInfo(env, callback, data, hint);
+  std::unique_ptr<BackingStore> bs =
+      ArrayBuffer::NewBackingStore(data, length, [](void*, size_t, void* arg) {
+        static_cast<CallbackInfo*>(arg)->OnBackingStoreFree();
+      }, self);
+  Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), std::move(bs));
+
+  // V8 simply ignores the BackingStore deleter callback if data == nullptr,
+  // but our API contract requires it being called.
+  if (data == nullptr) {
+    ab->Detach();
+    self->OnBackingStoreFree();  // This calls `callback` asynchronously.
+  } else {
+    // Store the ArrayBuffer so that we can detach it later.
+    self->persistent_.Reset(env->isolate(), ab);
+    self->persistent_.SetWeak();
+  }
 
-CallbackInfo* CallbackInfo::New(Environment* env,
-                                Local<ArrayBuffer> object,
-                                FreeCallback callback,
-                                char* data,
-                                void* hint) {
-  return new CallbackInfo(env, object, callback, data, hint);
+  return ab;
 }
 
 
 CallbackInfo::CallbackInfo(Environment* env,
-                           Local<ArrayBuffer> object,
                            FreeCallback callback,
                            char* data,
                            void* hint)
-    : persistent_(env->isolate(), object),
-      callback_(callback),
+    : callback_(callback),
       data_(data),
       hint_(hint),
       env_(env) {
-  std::shared_ptr<BackingStore> obj_backing = object->GetBackingStore();
-  CHECK_EQ(data_, static_cast<char*>(obj_backing->Data()));
-  if (object->ByteLength() != 0)
-    CHECK_NOT_NULL(data_);
-
-  persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
   env->AddCleanupHook(CleanupHook, this);
   env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
 }
 
-
-CallbackInfo::~CallbackInfo() {
-  persistent_.Reset();
-  env_->RemoveCleanupHook(CleanupHook, this);
-}
-
-
 void CallbackInfo::CleanupHook(void* data) {
   CallbackInfo* self = static_cast<CallbackInfo*>(data);
 
   {
     HandleScope handle_scope(self->env_->isolate());
     Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());
-    CHECK(!ab.IsEmpty());
-    if (ab->IsDetachable())
+    if (!ab.IsEmpty() && ab->IsDetachable()) {
       ab->Detach();
+      self->persistent_.Reset();
+    }
   }
 
-  self->WeakCallback(self->env_->isolate());
+  // Call the callback in this case, but don't delete `this` yet because the
+  // BackingStore deleter callback will do so later.
+  self->CallAndResetCallback();
 }
 
+void CallbackInfo::CallAndResetCallback() {
+  FreeCallback callback;
+  {
+    Mutex::ScopedLock lock(mutex_);
+    callback = callback_;
+    callback_ = nullptr;
+  }
+  if (callback != nullptr) {
+    // Clean up all Environment-related state and run the callback.
+    env_->RemoveCleanupHook(CleanupHook, this);
+    int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));
+    env_->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
 
-void CallbackInfo::WeakCallback(
-    const WeakCallbackInfo<CallbackInfo>& data) {
-  CallbackInfo* self = data.GetParameter();
-  self->WeakCallback(data.GetIsolate());
+    callback(data_, hint_);
+  }
 }
 
-
-void CallbackInfo::WeakCallback(Isolate* isolate) {
-  callback_(data_, hint_);
-  int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));
-  isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
-  delete this;
+void CallbackInfo::OnBackingStoreFree() {
+  // This method should always release the memory for `this`.
+  std::unique_ptr<CallbackInfo> self { this };
+  Mutex::ScopedLock lock(mutex_);
+  // If callback_ == nullptr, that means that the callback has already run from
+  // the cleanup hook, and there is nothing left to do here besides to clean
+  // up the memory involved. In particular, the underlying `Environment` may
+  // be gone at this point, so don’t attempt to call SetImmediateThreadsafe().
+  if (callback_ == nullptr) return;
+
+  env_->SetImmediateThreadsafe([self = std::move(self)](Environment* env) {
+    CHECK_EQ(self->env_, env);  // Consistency check.
+
+    self->CallAndResetCallback();
+  });
 }
 
 
@@ -408,26 +429,15 @@ MaybeLocal<Object> New(Environment* env,
     return Local<Object>();
   }
 
-
-  // The buffer will be released by a CallbackInfo::New() below,
-  // hence this BackingStore callback is empty.
-  std::unique_ptr<BackingStore> backing =
-      ArrayBuffer::NewBackingStore(data,
-                                   length,
-                                   [](void*, size_t, void*){},
-                                   nullptr);
-  Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(),
-                                           std::move(backing));
+  Local<ArrayBuffer> ab =
+      CallbackInfo::CreateTrackedArrayBuffer(env, data, length, callback, hint);
   if (ab->SetPrivate(env->context(),
                      env->arraybuffer_untransferable_private_symbol(),
                      True(env->isolate())).IsNothing()) {
-    callback(data, hint);
     return Local<Object>();
   }
   MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
 
-  CallbackInfo::New(env, ab, callback, data, hint);
-
   if (ui.IsEmpty())
     return MaybeLocal<Object>();