mirror of
https://github.com/nodejs/node.git
synced 2025-08-15 13:48:44 +02:00
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>
This commit is contained in:
parent
e3462614db
commit
c1ee70ec16
5 changed files with 108 additions and 137 deletions
|
@ -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,
|
||||
|
|
|
@ -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));
|
||||
|
||||
CallbackInfo* CallbackInfo::New(Environment* env,
|
||||
Local<ArrayBuffer> object,
|
||||
FreeCallback callback,
|
||||
char* data,
|
||||
void* hint) {
|
||||
return new CallbackInfo(env, object, callback, data, hint);
|
||||
// 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();
|
||||
}
|
||||
|
||||
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::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;
|
||||
|
||||
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;
|
||||
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>();
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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);
|
||||
});
|
||||
|
|
|
@ -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());
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue