From fe0e119f559a7ebc0e208324ec3b972c4473d11b Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 16 Nov 2018 21:42:31 +0000 Subject: [PATCH] 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: https://github.com/nodejs/node-addon-api/issues/393 Backport-PR-URL: https://github.com/nodejs/node/pull/25574 PR-URL: https://github.com/nodejs/node/pull/24494 Reviewed-By: Anna Henningsen Reviewed-By: Franziska Hinkelmann Reviewed-By: Refael Ackermann --- src/node_api.cc | 34 +++++++++++++++--- test/addons-napi/test_reference/test.js | 26 ++++++++++++++ .../test_reference/test_reference.c | 36 +++++++++++++++++++ 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 1f7b1405539e7b..98d13ff6008abf 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -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. @@ -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() { @@ -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) { @@ -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; } } diff --git a/test/addons-napi/test_reference/test.js b/test/addons-napi/test_reference/test.js index 14932a74ca70b0..389ee11d7e5f5b 100644 --- a/test/addons-napi/test_reference/test.js +++ b/test/addons-napi/test_reference/test.js @@ -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(); +} diff --git a/test/addons-napi/test_reference/test_reference.c b/test/addons-napi/test_reference/test_reference.c index 75abc49ad3280e..f3dc3644770ab0 100644 --- a/test/addons-napi/test_reference/test_reference.c +++ b/test/addons-napi/test_reference/test_reference.c @@ -1,3 +1,4 @@ +#include #include #include "../common.h" @@ -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), @@ -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(