Skip to content

Commit

Permalink
n-api: handle reference delete before finalize
Browse files Browse the repository at this point in the history
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

Backport-PR-URL: #25574
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>
  • Loading branch information
mhdawson authored and BethGriggs committed Feb 5, 2019
1 parent 0e63d0a commit fe0e119
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 5 deletions.
34 changes: 29 additions & 5 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ class Finalizer {
napi_finalize _finalize_callback;
void* _finalize_data;
void* _finalize_hint;
bool _finalize_ran = false;
};

// Wrapper around node::Persistent that implements reference counting.
Expand Down Expand Up @@ -412,8 +413,29 @@ class Reference : private Finalizer {
finalize_hint);
}

// Delete is called in 2 ways. Either from the finalizer or
// from one of Unwrap or napi_delete_reference.
//
// When it is called from Unwrap or napi_delete_reference we only
// want to do the delete if the finalizer has already run,
// otherwise we may crash when the finalizer does run.
// If the finalizer has not already run delay the delete until
// the finalizer runs by not doing the delete
// and setting _delete_self to true so that the finalizer will
// delete it when it runs.
//
// The second way this is called is from
// the finalizer and _delete_self is set. In this case we
// know we need to do the deletion so just do it.
static void Delete(Reference* reference) {
delete reference;
if ((reference->_delete_self) || (reference->_finalize_ran)) {
delete reference;
} else {
// reduce the reference count to 0 and defer until
// finalizer runs
reference->_delete_self = true;
while (reference->Unref() != 0) {}
}
}

uint32_t Ref() {
Expand Down Expand Up @@ -453,9 +475,6 @@ class Reference : private Finalizer {
Reference* reference = data.GetParameter();
reference->_persistent.Reset();

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

if (reference->_finalize_callback != nullptr) {
Expand All @@ -466,8 +485,13 @@ class Reference : private Finalizer {
reference->_finalize_hint));
}

if (delete_self) {
// this is safe because if a request to delete the reference
// is made in the finalize_callback it will defer deletion
// to this block and set _delete_self to true
if (reference->_delete_self) {
Delete(reference);
} else {
reference->_finalize_ran = true;
}
}

Expand Down
26 changes: 26 additions & 0 deletions test/addons-napi/test_reference/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,29 @@ runTests(0, undefined, [
assert.strictEqual(test_reference.finalizeCount, 1);
},
]);

// This test creates a napi_ref on an object that has
// been wrapped by napi_wrap and for which the finalizer
// for the wrap calls napi_delete_ref on that napi_ref.
//
// Since both the wrap and the reference use the same
// object the finalizer for the wrap and reference
// may run in the same gc and in any order.
//
// It does that to validate that napi_delete_ref can be
// called before the finalizer has been run for the
// reference (there is a finalizer behind the scenes even
// though it cannot be passed to napi_create_reference).
//
// Since the order is not guarranteed, run the
// test a number of times maximize the chance that we
// get a run with the desired order for the test.
//
// 1000 reliably recreated the problem without the fix
// required to ensure delete could be called before
// the finalizer in manual testing.
for (let i = 0; i < 1000; i++) {
const wrapObject = new Object();
test_reference.validateDeleteBeforeFinalize(wrapObject);
global.gc();
}
36 changes: 36 additions & 0 deletions test/addons-napi/test_reference/test_reference.c
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <stdlib.h>
#include <node_api.h>
#include "../common.h"

Expand Down Expand Up @@ -131,6 +132,39 @@ static napi_value GetReferenceValue(napi_env env, napi_callback_info info) {
return result;
}

static void DeleteBeforeFinalizeFinalizer(
napi_env env, void* finalize_data, void* finalize_hint) {
napi_ref* ref = (napi_ref*)finalize_data;
napi_delete_reference(env, *ref);
free(ref);
}

static napi_value ValidateDeleteBeforeFinalize(napi_env env, napi_callback_info info) {
napi_value wrapObject;
size_t argc = 1;
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &wrapObject, NULL, NULL));

napi_ref* ref_t = malloc(sizeof(napi_ref));
NAPI_CALL(env, napi_wrap(env,
wrapObject,
ref_t,
DeleteBeforeFinalizeFinalizer,
NULL,
NULL));

// Create a reference that will be eligible for collection at the same
// time as the wrapped object by passing in the same wrapObject.
// This means that the FinalizeOrderValidation callback may be run
// before the finalizer for the newly created reference (there is a finalizer
// behind the scenes even though it cannot be passed to napi_create_reference)
// The Finalizer for the wrap (which is different than the finalizer
// for the reference) calls napi_delete_reference validating that
// napi_delete_reference can be called before the finalizer for the
// reference runs.
NAPI_CALL(env, napi_create_reference(env, wrapObject, 0, ref_t));
return wrapObject;
}

static napi_value Init(napi_env env, napi_value exports) {
napi_property_descriptor descriptors[] = {
DECLARE_NAPI_GETTER("finalizeCount", GetFinalizeCount),
Expand All @@ -143,6 +177,8 @@ static napi_value Init(napi_env env, napi_value exports) {
DECLARE_NAPI_PROPERTY("incrementRefcount", IncrementRefcount),
DECLARE_NAPI_PROPERTY("decrementRefcount", DecrementRefcount),
DECLARE_NAPI_GETTER("referenceValue", GetReferenceValue),
DECLARE_NAPI_PROPERTY("validateDeleteBeforeFinalize",
ValidateDeleteBeforeFinalize),
};

NAPI_CALL(env, napi_define_properties(
Expand Down

0 comments on commit fe0e119

Please sign in to comment.