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: #32463 (comment) Fixes: #32463
nodejs · addaleax · May 9, 2020 · May 9, 2020 · May 10, 2020 · May 9, 2020
commit 655585c7dbd9735f254319611f62332165baf5ae
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
@@ -371,39 +371,6 @@ class Reference : public RefBase {
   v8impl::Persistent<v8::Value> _persistent;
 };
 
-class ArrayBufferReference final : public Reference {
- public:
-  // Same signatures for ctor and New() as Reference, except this only works
-  // with ArrayBuffers:
-  template <typename... Args>
-  explicit ArrayBufferReference(napi_env env,
-                                v8::Local<v8::ArrayBuffer> value,
-                                Args&&... args)
-    : Reference(env, value, std::forward<Args>(args)...) {}
-
-  template <typename... Args>
-  static ArrayBufferReference* New(napi_env env,
-                                   v8::Local<v8::ArrayBuffer> value,
-                                   Args&&... args) {
-    return new ArrayBufferReference(env, value, std::forward<Args>(args)...);
-  }
-
- private:
-  inline void Finalize(bool is_env_teardown) override {
-    if (is_env_teardown) {
-      v8::HandleScope handle_scope(_env->isolate);
-      v8::Local<v8::Value> obj = Get();
-      CHECK(!obj.IsEmpty());
-      CHECK(obj->IsArrayBuffer());
-      v8::Local<v8::ArrayBuffer> ab = obj.As<v8::ArrayBuffer>();
-      if (ab->IsDetachable())
-        ab->Detach();
-    }
-
-    Reference::Finalize(is_env_teardown);
-  }
-};
-
 enum UnwrapAction {
   KeepWrap,
   RemoveWrap
@@ -2710,37 +2677,27 @@ napi_status napi_create_external_arraybuffer(napi_env env,
                                              napi_finalize finalize_cb,
                                              void* finalize_hint,
                                              napi_value* result) {
-  NAPI_PREAMBLE(env);
-  CHECK_ARG(env, result);
-
-  v8::Isolate* isolate = env->isolate;
-  // The buffer will be freed with v8impl::ArrayBufferReference::New()
-  // below, hence this BackingStore does not need to free the buffer.
-  std::unique_ptr<v8::BackingStore> backing =
-      v8::ArrayBuffer::NewBackingStore(external_data,
-                                       byte_length,
-                                       [](void*, size_t, void*){},
-                                       nullptr);
-  v8::Local<v8::ArrayBuffer> buffer =
-      v8::ArrayBuffer::New(isolate, std::move(backing));
-  v8::Maybe<bool> marked = env->mark_arraybuffer_as_untransferable(buffer);
-  CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure);
-
-  if (finalize_cb != nullptr) {
-    // Create a self-deleting weak reference that invokes the finalizer
-    // callback and detaches the ArrayBuffer if it still exists on Environment
-    // teardown.
-    v8impl::ArrayBufferReference::New(env,
-        buffer,
-        0,
-        true,
-        finalize_cb,
-        external_data,
-        finalize_hint);
-  }
-
-  *result = v8impl::JsValueFromV8LocalValue(buffer);
-  return GET_RETURN_STATUS(env);
+  // The API contract here is that the cleanup function runs on the JS thread,
+  // and is able to use napi_env. Implementing that properly is hard, so use the
+  // `Buffer` variant for easier implementation.
+  napi_value buffer;
+  napi_status status;
+  status = napi_create_external_buffer(
+      env,
+      byte_length,
+      external_data,
+      finalize_cb,
+      finalize_hint,
+      &buffer);
+  if (status != napi_ok) return status;
+  return napi_get_typedarray_info(
+      env,
+      buffer,
+      nullptr,
+      nullptr,
+      nullptr,
+      result,
+      nullptr);
 }
 
 napi_status napi_get_arraybuffer_info(napi_env env,

diff --git 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>();
 

diff --git a/test/addons/null-buffer-neuter/binding.cc b/test/addons/null-buffer-neuter/binding.cc
@@ -11,6 +11,10 @@ static void FreeCallback(char* data, void* hint) {
   alive--;
 }
 
+void IsAlive(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  args.GetReturnValue().Set(alive);
+}
+
 void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   alive++;
@@ -27,15 +31,11 @@ void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {
     char* data = node::Buffer::Data(buf);
     assert(data == nullptr);
   }
-
-  isolate->RequestGarbageCollectionForTesting(
-      v8::Isolate::kFullGarbageCollection);
-
-  assert(alive == 0);
 }
 
 void init(v8::Local<v8::Object> exports) {
   NODE_SET_METHOD(exports, "run", Run);
+  NODE_SET_METHOD(exports, "isAlive", IsAlive);
 }
 
 NODE_MODULE(NODE_GYP_MODULE_NAME, init)
diff --git a/test/addons/null-buffer-neuter/test.js b/test/addons/null-buffer-neuter/test.js
@@ -1,7 +1,11 @@
 'use strict';
 // Flags: --expose-gc
-
 const common = require('../../common');
+const assert = require('assert');
 const binding = require(`./build/${common.buildType}/binding`);
 
 binding.run();
+global.gc();
+setImmediate(() => {
+  assert.strictEqual(binding.isAlive(), 0);
+});
diff --git a/test/node-api/test_buffer/test.js b/test/node-api/test_buffer/test.js
@@ -1,5 +1,5 @@
 'use strict';
-// Flags: --expose-gc
+// Flags: --expose-gc --no-concurrent-array-buffer-freeing --no-concurrent-array-buffer-sweeping
 
 const common = require('../../common');
 const binding = require(`./build/${common.buildType}/test_buffer`);