From 24faa37a09e5f77fe5b57d417c56d0f1a91ab215 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 May 2020 06:41:58 +0200 Subject: [PATCH] buffer,n-api: release external buffers from BackingStore callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/js_native_api_v8.cc | 85 ++++--------- src/node_buffer.cc | 140 ++++++++++++---------- test/addons/null-buffer-neuter/binding.cc | 10 +- test/addons/null-buffer-neuter/test.js | 6 +- test/node-api/test_buffer/test.js | 8 +- 5 files changed, 110 insertions(+), 139 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 4255c3e7ec3fff..a09969b9adf28f 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -371,39 +371,6 @@ class Reference : public RefBase { v8impl::Persistent _persistent; }; -class ArrayBufferReference final : public Reference { - public: - // Same signatures for ctor and New() as Reference, except this only works - // with ArrayBuffers: - template - explicit ArrayBufferReference(napi_env env, - v8::Local value, - Args&&... args) - : Reference(env, value, std::forward(args)...) {} - - template - static ArrayBufferReference* New(napi_env env, - v8::Local value, - Args&&... args) { - return new ArrayBufferReference(env, value, std::forward(args)...); - } - - private: - inline void Finalize(bool is_env_teardown) override { - if (is_env_teardown) { - v8::HandleScope handle_scope(_env->isolate); - v8::Local obj = Get(); - CHECK(!obj.IsEmpty()); - CHECK(obj->IsArrayBuffer()); - v8::Local ab = obj.As(); - 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 backing = - v8::ArrayBuffer::NewBackingStore(external_data, - byte_length, - [](void*, size_t, void*){}, - nullptr); - v8::Local buffer = - v8::ArrayBuffer::New(isolate, std::move(backing)); - v8::Maybe 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 index 1ff60ad721753e..fd20415936e169 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 object, - FreeCallback callback, - char* data, - void* hint = nullptr); + static inline Local 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&); - inline void WeakCallback(Isolate* isolate); + inline void OnBackingStoreFree(); + inline void CallAndResetCallback(); inline CallbackInfo(Environment* env, - Local object, FreeCallback callback, char* data, void* hint); Global 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 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 bs = + ArrayBuffer::NewBackingStore(data, length, [](void*, size_t, void* arg) { + static_cast(arg)->OnBackingStoreFree(); + }, self); + Local 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 object, - FreeCallback callback, - char* data, - void* hint) { - return new CallbackInfo(env, object, callback, data, hint); + return ab; } CallbackInfo::CallbackInfo(Environment* env, - Local object, FreeCallback callback, char* data, void* hint) - : persistent_(env->isolate(), object), - callback_(callback), + : callback_(callback), data_(data), hint_(hint), env_(env) { - std::shared_ptr obj_backing = object->GetBackingStore(); - CHECK_EQ(data_, static_cast(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(data); { HandleScope handle_scope(self->env_->isolate()); Local 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(sizeof(*this)); + env_->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes); -void CallbackInfo::WeakCallback( - const WeakCallbackInfo& 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(sizeof(*this)); - isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes); - delete this; +void CallbackInfo::OnBackingStoreFree() { + // This method should always release the memory for `this`. + std::unique_ptr 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 New(Environment* env, return Local(); } - - // The buffer will be released by a CallbackInfo::New() below, - // hence this BackingStore callback is empty. - std::unique_ptr backing = - ArrayBuffer::NewBackingStore(data, - length, - [](void*, size_t, void*){}, - nullptr); - Local ab = ArrayBuffer::New(env->isolate(), - std::move(backing)); + Local 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(); } MaybeLocal ui = Buffer::New(env, ab, 0, length); - CallbackInfo::New(env, ab, callback, data, hint); - if (ui.IsEmpty()) return MaybeLocal(); diff --git a/test/addons/null-buffer-neuter/binding.cc b/test/addons/null-buffer-neuter/binding.cc index 4c9dbf902589d9..7575e29fa85084 100644 --- 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& args) { + args.GetReturnValue().Set(alive); +} + void Run(const v8::FunctionCallbackInfo& args) { v8::Isolate* isolate = args.GetIsolate(); alive++; @@ -27,15 +31,11 @@ void Run(const v8::FunctionCallbackInfo& args) { char* data = node::Buffer::Data(buf); assert(data == nullptr); } - - isolate->RequestGarbageCollectionForTesting( - v8::Isolate::kFullGarbageCollection); - - assert(alive == 0); } void init(v8::Local 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 index 3fc335914c9a44..d99690542a4d84 100644 --- 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 index 6b6c2089afa76e..fe668a3ef9a879 100644 --- a/test/node-api/test_buffer/test.js +++ b/test/node-api/test_buffer/test.js @@ -1,10 +1,10 @@ '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`); const assert = require('assert'); -const setImmediatePromise = require('util').promisify(setImmediate); +const tick = require('util').promisify(require('../../common/tick')); (async function() { assert.strictEqual(binding.newBuffer().toString(), binding.theText); @@ -12,7 +12,7 @@ const setImmediatePromise = require('util').promisify(setImmediate); console.log('gc1'); global.gc(); assert.strictEqual(binding.getDeleterCallCount(), 0); - await setImmediatePromise(); + await tick(10); assert.strictEqual(binding.getDeleterCallCount(), 1); assert.strictEqual(binding.copyBuffer().toString(), binding.theText); @@ -22,7 +22,7 @@ const setImmediatePromise = require('util').promisify(setImmediate); buffer = null; global.gc(); assert.strictEqual(binding.getDeleterCallCount(), 1); - await setImmediatePromise(); + await tick(10); console.log('gc2'); assert.strictEqual(binding.getDeleterCallCount(), 2); })().then(common.mustCall());