Skip to content

Commit 0c51fc5

Browse files
mhdawsonrvagg
authored andcommitted
n-api: handle reference delete before finalize
Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion. This change ensures the deletion of the memory for the reference only occurs after the finalizer has run. Fixes: nodejs/node-addon-api#393 PR-URL: #24494 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
1 parent 2a67a49 commit 0c51fc5

File tree

4 files changed

+91
-5
lines changed

4 files changed

+91
-5
lines changed

src/js_native_api_v8.cc

+28-5
Original file line numberDiff line numberDiff line change
@@ -227,8 +227,29 @@ class Reference : private Finalizer {
227227
finalize_hint);
228228
}
229229

230+
// Delete is called in 2 ways. Either from the finalizer or
231+
// from one of Unwrap or napi_delete_reference.
232+
//
233+
// When it is called from Unwrap or napi_delete_reference we only
234+
// want to do the delete if the finalizer has already run,
235+
// otherwise we may crash when the finalizer does run.
236+
// If the finalizer has not already run delay the delete until
237+
// the finalizer runs by not doing the delete
238+
// and setting _delete_self to true so that the finalizer will
239+
// delete it when it runs.
240+
//
241+
// The second way this is called is from
242+
// the finalizer and _delete_self is set. In this case we
243+
// know we need to do the deletion so just do it.
230244
static void Delete(Reference* reference) {
231-
delete reference;
245+
if ((reference->_delete_self) || (reference->_finalize_ran)) {
246+
delete reference;
247+
} else {
248+
// reduce the reference count to 0 and defer until
249+
// finalizer runs
250+
reference->_delete_self = true;
251+
while (reference->Unref() != 0) {}
252+
}
232253
}
233254

234255
uint32_t Ref() {
@@ -268,9 +289,6 @@ class Reference : private Finalizer {
268289
Reference* reference = data.GetParameter();
269290
reference->_persistent.Reset();
270291

271-
// Check before calling the finalize callback, because the callback might
272-
// delete it.
273-
bool delete_self = reference->_delete_self;
274292
napi_env env = reference->_env;
275293

276294
if (reference->_finalize_callback != nullptr) {
@@ -281,8 +299,13 @@ class Reference : private Finalizer {
281299
reference->_finalize_hint));
282300
}
283301

284-
if (delete_self) {
302+
// this is safe because if a request to delete the reference
303+
// is made in the finalize_callback it will defer deletion
304+
// to this block and set _delete_self to true
305+
if (reference->_delete_self) {
285306
Delete(reference);
307+
} else {
308+
reference->_finalize_ran = true;
286309
}
287310
}
288311

src/js_native_api_v8.h

+1
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,7 @@ class Finalizer {
180180
napi_finalize _finalize_callback;
181181
void* _finalize_data;
182182
void* _finalize_hint;
183+
bool _finalize_ran = false;
183184
};
184185

185186
class TryCatch : public v8::TryCatch {

test/addons-napi/test_reference/test.js

+26
Original file line numberDiff line numberDiff line change
@@ -118,3 +118,29 @@ runTests(0, undefined, [
118118
assert.strictEqual(test_reference.finalizeCount, 1);
119119
},
120120
]);
121+
122+
// This test creates a napi_ref on an object that has
123+
// been wrapped by napi_wrap and for which the finalizer
124+
// for the wrap calls napi_delete_ref on that napi_ref.
125+
//
126+
// Since both the wrap and the reference use the same
127+
// object the finalizer for the wrap and reference
128+
// may run in the same gc and in any order.
129+
//
130+
// It does that to validate that napi_delete_ref can be
131+
// called before the finalizer has been run for the
132+
// reference (there is a finalizer behind the scenes even
133+
// though it cannot be passed to napi_create_reference).
134+
//
135+
// Since the order is not guarranteed, run the
136+
// test a number of times maximize the chance that we
137+
// get a run with the desired order for the test.
138+
//
139+
// 1000 reliably recreated the problem without the fix
140+
// required to ensure delete could be called before
141+
// the finalizer in manual testing.
142+
for (let i = 0; i < 1000; i++) {
143+
const wrapObject = new Object();
144+
test_reference.validateDeleteBeforeFinalize(wrapObject);
145+
global.gc();
146+
}

test/addons-napi/test_reference/test_reference.c

+36
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#include <stdlib.h>
12
#include <node_api.h>
23
#include "../common.h"
34

@@ -131,6 +132,39 @@ static napi_value GetReferenceValue(napi_env env, napi_callback_info info) {
131132
return result;
132133
}
133134

135+
static void DeleteBeforeFinalizeFinalizer(
136+
napi_env env, void* finalize_data, void* finalize_hint) {
137+
napi_ref* ref = (napi_ref*)finalize_data;
138+
napi_delete_reference(env, *ref);
139+
free(ref);
140+
}
141+
142+
static napi_value ValidateDeleteBeforeFinalize(napi_env env, napi_callback_info info) {
143+
napi_value wrapObject;
144+
size_t argc = 1;
145+
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &wrapObject, NULL, NULL));
146+
147+
napi_ref* ref_t = malloc(sizeof(napi_ref));
148+
NAPI_CALL(env, napi_wrap(env,
149+
wrapObject,
150+
ref_t,
151+
DeleteBeforeFinalizeFinalizer,
152+
NULL,
153+
NULL));
154+
155+
// Create a reference that will be eligible for collection at the same
156+
// time as the wrapped object by passing in the same wrapObject.
157+
// This means that the FinalizeOrderValidation callback may be run
158+
// before the finalizer for the newly created reference (there is a finalizer
159+
// behind the scenes even though it cannot be passed to napi_create_reference)
160+
// The Finalizer for the wrap (which is different than the finalizer
161+
// for the reference) calls napi_delete_reference validating that
162+
// napi_delete_reference can be called before the finalizer for the
163+
// reference runs.
164+
NAPI_CALL(env, napi_create_reference(env, wrapObject, 0, ref_t));
165+
return wrapObject;
166+
}
167+
134168
static napi_value Init(napi_env env, napi_value exports) {
135169
napi_property_descriptor descriptors[] = {
136170
DECLARE_NAPI_GETTER("finalizeCount", GetFinalizeCount),
@@ -143,6 +177,8 @@ static napi_value Init(napi_env env, napi_value exports) {
143177
DECLARE_NAPI_PROPERTY("incrementRefcount", IncrementRefcount),
144178
DECLARE_NAPI_PROPERTY("decrementRefcount", DecrementRefcount),
145179
DECLARE_NAPI_GETTER("referenceValue", GetReferenceValue),
180+
DECLARE_NAPI_PROPERTY("validateDeleteBeforeFinalize",
181+
ValidateDeleteBeforeFinalize),
146182
};
147183

148184
NAPI_CALL(env, napi_define_properties(

0 commit comments

Comments
 (0)