FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer,n-api: release external buffers from BackingStore callback · nodejs/node@24faa37 · GitHub

/ node Public

Commit 24faa37

Browse files
addaleax
authored and
codebytere
committed
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 PR-URL: #33321 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 099f18e commit 24faa37

5 files changed

Lines changed: 110 additions & 139 deletions

File tree

‎src/js_native_api_v8.cc‎

Lines changed: 21 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -371,39 +371,6 @@ class Reference : public RefBase {
371371
v8impl::Persistent<v8::Value> _persistent;
372372
};
373373

374-
class ArrayBufferReference final : public Reference {
375-
public:
376-
// Same signatures for ctor and New() as Reference, except this only works
377-
// with ArrayBuffers:
378-
template <typename... Args>
379-
explicit ArrayBufferReference(napi_env env,
380-
v8::Local<v8::ArrayBuffer> value,
381-
Args&&... args)
382-
: Reference(env, value, std::forward<Args>(args)...) {}
383-
384-
template <typename... Args>
385-
static ArrayBufferReference* New(napi_env env,
386-
v8::Local<v8::ArrayBuffer> value,
387-
Args&&... args) {
388-
return new ArrayBufferReference(env, value, std::forward<Args>(args)...);
389-
}
390-
391-
private:
392-
inline void Finalize(bool is_env_teardown) override {
393-
if (is_env_teardown) {
394-
v8::HandleScope handle_scope(_env->isolate);
395-
v8::Local<v8::Value> obj = Get();
396-
CHECK(!obj.IsEmpty());
397-
CHECK(obj->IsArrayBuffer());
398-
v8::Local<v8::ArrayBuffer> ab = obj.As<v8::ArrayBuffer>();
399-
if (ab->IsDetachable())
400-
ab->Detach();
401-
}
402-
403-
Reference::Finalize(is_env_teardown);
404-
}
405-
};
406-
407374
enum UnwrapAction {
408375
KeepWrap,
409376
RemoveWrap
@@ -2710,37 +2677,27 @@ napi_status napi_create_external_arraybuffer(napi_env env,
27102677
napi_finalize finalize_cb,
27112678
void* finalize_hint,
27122679
napi_value* result) {
2713-
NAPI_PREAMBLE(env);
2714-
CHECK_ARG(env, result);
2715-
2716-
v8::Isolate* isolate = env->isolate;
2717-
// The buffer will be freed with v8impl::ArrayBufferReference::New()
2718-
// below, hence this BackingStore does not need to free the buffer.
2719-
std::unique_ptr<v8::BackingStore> backing =
2720-
v8::ArrayBuffer::NewBackingStore(external_data,
2721-
byte_length,
2722-
[](void*, size_t, void*){},
2723-
nullptr);
2724-
v8::Local<v8::ArrayBuffer> buffer =
2725-
v8::ArrayBuffer::New(isolate, std::move(backing));
2726-
v8::Maybe<bool> marked = env->mark_arraybuffer_as_untransferable(buffer);
2727-
CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure);
2728-
2729-
if (finalize_cb != nullptr) {
2730-
// Create a self-deleting weak reference that invokes the finalizer
2731-
// callback and detaches the ArrayBuffer if it still exists on Environment
2732-
// teardown.
2733-
v8impl::ArrayBufferReference::New(env,
2734-
buffer,
2735-
0,
2736-
true,
2737-
finalize_cb,
2738-
external_data,
2739-
finalize_hint);
2740-
}
2741-
2742-
*result = v8impl::JsValueFromV8LocalValue(buffer);
2743-
return GET_RETURN_STATUS(env);
2680+
// The API contract here is that the cleanup function runs on the JS thread,
2681+
// and is able to use napi_env. Implementing that properly is hard, so use the
2682+
// `Buffer` variant for easier implementation.
2683+
napi_value buffer;
2684+
napi_status status;
2685+
status = napi_create_external_buffer(
2686+
env,
2687+
byte_length,
2688+
external_data,
2689+
finalize_cb,
2690+
finalize_hint,
2691+
&buffer);
2692+
if (status != napi_ok) return status;
2693+
return napi_get_typedarray_info(
2694+
env,
2695+
buffer,
2696+
nullptr,
2697+
nullptr,
2698+
nullptr,
2699+
result,
2700+
nullptr);
27442701
}
27452702

27462703
napi_status napi_get_arraybuffer_info(napi_env env,

‎src/node_buffer.cc‎

Lines changed: 75 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -69,109 +69,130 @@ using v8::Uint32;
6969
using v8::Uint32Array;
7070
using v8::Uint8Array;
7171
using v8::Value;
72-
using v8::WeakCallbackInfo;
7372

7473
namespace {
7574

7675
class CallbackInfo {
7776
public:
78-
~CallbackInfo();
79-
80-
static inline void Free(char* data, void* hint);
81-
static inline CallbackInfo* New(Environment* env,
82-
Local<ArrayBuffer> object,
83-
FreeCallback callback,
84-
char* data,
85-
void* hint = nullptr);
77+
static inline Local<ArrayBuffer> CreateTrackedArrayBuffer(
78+
Environment* env,
79+
char* data,
80+
size_t length,
81+
FreeCallback callback,
82+
void* hint);
8683

8784
CallbackInfo(const CallbackInfo&) = delete;
8885
CallbackInfo& operator=(const CallbackInfo&) = delete;
8986

9087
private:
9188
static void CleanupHook(void* data);
92-
static void WeakCallback(const WeakCallbackInfo<CallbackInfo>&);
93-
inline void WeakCallback(Isolate* isolate);
89+
inline void OnBackingStoreFree();
90+
inline void CallAndResetCallback();
9491
inline CallbackInfo(Environment* env,
95-
Local<ArrayBuffer> object,
9692
FreeCallback callback,
9793
char* data,
9894
void* hint);
9995
Global<ArrayBuffer> persistent_;
100-
FreeCallback const callback_;
96+
Mutex mutex_; // Protects callback_.
97+
FreeCallback callback_;
10198
char* const data_;
10299
void* const hint_;
103100
Environment* const env_;
104101
};
105102

106103

107-
void CallbackInfo::Free(char* data, void*) {
108-
::free(data);
109-
}
110-
104+
Local<ArrayBuffer> CallbackInfo::CreateTrackedArrayBuffer(
105+
Environment* env,
106+
char* data,
107+
size_t length,
108+
FreeCallback callback,
109+
void* hint) {
110+
CHECK_NOT_NULL(callback);
111+
CHECK_IMPLIES(data == nullptr, length == 0);
112+
113+
CallbackInfo* self = new CallbackInfo(env, callback, data, hint);
114+
std::unique_ptr<BackingStore> bs =
115+
ArrayBuffer::NewBackingStore(data, length, [](void*, size_t, void* arg) {
116+
static_cast<CallbackInfo*>(arg)->OnBackingStoreFree();
117+
}, self);
118+
Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), std::move(bs));
119+
120+
// V8 simply ignores the BackingStore deleter callback if data == nullptr,
121+
// but our API contract requires it being called.
122+
if (data == nullptr) {
123+
ab->Detach();
124+
self->OnBackingStoreFree(); // This calls `callback` asynchronously.
125+
} else {
126+
// Store the ArrayBuffer so that we can detach it later.
127+
self->persistent_.Reset(env->isolate(), ab);
128+
self->persistent_.SetWeak();
129+
}
111130

112-
CallbackInfo* CallbackInfo::New(Environment* env,
113-
Local<ArrayBuffer> object,
114-
FreeCallback callback,
115-
char* data,
116-
void* hint) {
117-
return new CallbackInfo(env, object, callback, data, hint);
131+
return ab;
118132
}
119133

120134

121135
CallbackInfo::CallbackInfo(Environment* env,
122-
Local<ArrayBuffer> object,
123136
FreeCallback callback,
124137
char* data,
125138
void* hint)
126-
: persistent_(env->isolate(), object),
127-
callback_(callback),
139+
: callback_(callback),
128140
data_(data),
129141
hint_(hint),
130142
env_(env) {
131-
std::shared_ptr<BackingStore> obj_backing = object->GetBackingStore();
132-
CHECK_EQ(data_, static_cast<char*>(obj_backing->Data()));
133-
if (object->ByteLength() != 0)
134-
CHECK_NOT_NULL(data_);
135-
136-
persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
137143
env->AddCleanupHook(CleanupHook, this);
138144
env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
139145
}
140146

141-
142-
CallbackInfo::~CallbackInfo() {
143-
persistent_.Reset();
144-
env_->RemoveCleanupHook(CleanupHook, this);
145-
}
146-
147-
148147
void CallbackInfo::CleanupHook(void* data) {
149148
CallbackInfo* self = static_cast<CallbackInfo*>(data);
150149

151150
{
152151
HandleScope handle_scope(self->env_->isolate());
153152
Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());
154-
CHECK(!ab.IsEmpty());
155-
if (ab->IsDetachable())
153+
if (!ab.IsEmpty() && ab->IsDetachable()) {
156154
ab->Detach();
155+
self->persistent_.Reset();
156+
}
157157
}
158158

159-
self->WeakCallback(self->env_->isolate());
159+
// Call the callback in this case, but don't delete `this` yet because the
160+
// BackingStore deleter callback will do so later.
161+
self->CallAndResetCallback();
160162
}
161163

164+
void CallbackInfo::CallAndResetCallback() {
165+
FreeCallback callback;
166+
{
167+
Mutex::ScopedLock lock(mutex_);
168+
callback = callback_;
169+
callback_ = nullptr;
170+
}
171+
if (callback != nullptr) {
172+
// Clean up all Environment-related state and run the callback.
173+
env_->RemoveCleanupHook(CleanupHook, this);
174+
int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));
175+
env_->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
162176

163-
void CallbackInfo::WeakCallback(
164-
const WeakCallbackInfo<CallbackInfo>& data) {
165-
CallbackInfo* self = data.GetParameter();
166-
self->WeakCallback(data.GetIsolate());
177+
callback(data_, hint_);
178+
}
167179
}
168180

169-
170-
void CallbackInfo::WeakCallback(Isolate* isolate) {
171-
callback_(data_, hint_);
172-
int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));
173-
isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
174-
delete this;
181+
void CallbackInfo::OnBackingStoreFree() {
182+
// This method should always release the memory for `this`.
183+
std::unique_ptr<CallbackInfo> self { this };
184+
Mutex::ScopedLock lock(mutex_);
185+
// If callback_ == nullptr, that means that the callback has already run from
186+
// the cleanup hook, and there is nothing left to do here besides to clean
187+
// up the memory involved. In particular, the underlying `Environment` may
188+
// be gone at this point, so don’t attempt to call SetImmediateThreadsafe().
189+
if (callback_ == nullptr) return;
190+
191+
env_->SetImmediateThreadsafe([self = std::move(self)](Environment* env) {
192+
CHECK_EQ(self->env_, env); // Consistency check.
193+
194+
self->CallAndResetCallback();
195+
});
175196
}
176197

177198

@@ -408,26 +429,15 @@ MaybeLocal<Object> New(Environment* env,
408429
return Local<Object>();
409430
}
410431

411-
412-
// The buffer will be released by a CallbackInfo::New() below,
413-
// hence this BackingStore callback is empty.
414-
std::unique_ptr<BackingStore> backing =
415-
ArrayBuffer::NewBackingStore(data,
416-
length,
417-
[](void*, size_t, void*){},
418-
nullptr);
419-
Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(),
420-
std::move(backing));
432+
Local<ArrayBuffer> ab =
433+
CallbackInfo::CreateTrackedArrayBuffer(env, data, length, callback, hint);
421434
if (ab->SetPrivate(env->context(),
422435
env->arraybuffer_untransferable_private_symbol(),
423436
True(env->isolate())).IsNothing()) {
424-
callback(data, hint);
425437
return Local<Object>();
426438
}
427439
MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
428440

429-
CallbackInfo::New(env, ab, callback, data, hint);
430-
431441
if (ui.IsEmpty())
432442
return MaybeLocal<Object>();
433443

‎test/addons/null-buffer-neuter/binding.cc‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ static void FreeCallback(char* data, void* hint) {
1111
alive--;
1212
}
1313

14+
void IsAlive(const v8::FunctionCallbackInfo<v8::Value>& args) {
15+
args.GetReturnValue().Set(alive);
16+
}
17+
1418
void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {
1519
v8::Isolate* isolate = args.GetIsolate();
1620
alive++;
@@ -27,15 +31,11 @@ void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {
2731
char* data = node::Buffer::Data(buf);
2832
assert(data == nullptr);
2933
}
30-
31-
isolate->RequestGarbageCollectionForTesting(
32-
v8::Isolate::kFullGarbageCollection);
33-
34-
assert(alive == 0);
3534
}
3635

3736
void init(v8::Local<v8::Object> exports) {
3837
NODE_SET_METHOD(exports, "run", Run);
38+
NODE_SET_METHOD(exports, "isAlive", IsAlive);
3939
}
4040

4141
NODE_MODULE(NODE_GYP_MODULE_NAME, init)

‎test/addons/null-buffer-neuter/test.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
'use strict';
22
// Flags: --expose-gc
3-
43
const common = require('../../common');
4+
const assert = require('assert');
55
const binding = require(`./build/${common.buildType}/binding`);
66

77
binding.run();
8+
global.gc();
9+
setImmediate(() => {
10+
assert.strictEqual(binding.isAlive(), 0);
11+
});

‎test/node-api/test_buffer/test.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
'use strict';
2-
// Flags: --expose-gc
2+
// Flags: --expose-gc --no-concurrent-array-buffer-freeing --no-concurrent-array-buffer-sweeping
33

44
const common = require('../../common');
55
const binding = require(`./build/${common.buildType}/test_buffer`);
66
const assert = require('assert');
7-
const setImmediatePromise = require('util').promisify(setImmediate);
7+
const tick = require('util').promisify(require('../../common/tick'));
88

99
(async function() {
1010
assert.strictEqual(binding.newBuffer().toString(), binding.theText);
1111
assert.strictEqual(binding.newExternalBuffer().toString(), binding.theText);
1212
console.log('gc1');
1313
global.gc();
1414
assert.strictEqual(binding.getDeleterCallCount(), 0);
15-
await setImmediatePromise();
15+
await tick(10);
1616
assert.strictEqual(binding.getDeleterCallCount(), 1);
1717
assert.strictEqual(binding.copyBuffer().toString(), binding.theText);
1818

@@ -22,7 +22,7 @@ const setImmediatePromise = require('util').promisify(setImmediate);
2222
buffer = null;
2323
global.gc();
2424
assert.strictEqual(binding.getDeleterCallCount(), 1);
25-
await setImmediatePromise();
25+
await tick(10);
2626
console.log('gc2');
2727
assert.strictEqual(binding.getDeleterCallCount(), 2);
2828
})().then(common.mustCall());

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL