From 4cf6a26e5ecb851080d2321c6647b6c3562c9461 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 9 Feb 2021 22:52:54 -0800 Subject: [PATCH 1/5] node-api: force env shutdown deferring behavior The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: https://github.com/nodejs/node/issues/37236 Co-authored-by: Chengzhong Wu --- src/js_native_api_v8.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index e037c4297de0c5..387278e4f634f5 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -270,6 +270,10 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { + // Force deferring behavior if the finalizer happens to delete this + // reference. + if (is_env_teardown && RefCount() > 0) _refcount = 0; + if (_finalize_callback != nullptr) { _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); } From 167bb7f95aa5c1cfe6bfff14c82ea99f5947ce1d Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 9 Feb 2021 23:26:42 -0800 Subject: [PATCH 2/5] add test --- .../test_reference_double_free/binding.gyp | 11 +++++++ .../test_reference_double_free/test.js | 10 ++++++ .../test_reference_double_free.c | 33 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 test/js-native-api/test_reference_double_free/binding.gyp create mode 100644 test/js-native-api/test_reference_double_free/test.js create mode 100644 test/js-native-api/test_reference_double_free/test_reference_double_free.c diff --git a/test/js-native-api/test_reference_double_free/binding.gyp b/test/js-native-api/test_reference_double_free/binding.gyp new file mode 100644 index 00000000000000..864846765d0132 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/binding.gyp @@ -0,0 +1,11 @@ +{ + "targets": [ + { + "target_name": "test_reference_double_free", + "sources": [ + "../entry_point.c", + "test_reference_double_free.c" + ] + } + ] +} diff --git a/test/js-native-api/test_reference_double_free/test.js b/test/js-native-api/test_reference_double_free/test.js new file mode 100644 index 00000000000000..db9f1884a0f1c2 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test.js @@ -0,0 +1,10 @@ +'use strict'; + +// This test makes no assertions. It tests a fix without which it will crash +// with a double free. + +const { buildType } = require('../../common'); + +const addon = require(`./build/${buildType}/test_reference_double_free`); + +{ const obj = new addon.MyObject(); } diff --git a/test/js-native-api/test_reference_double_free/test_reference_double_free.c b/test/js-native-api/test_reference_double_free/test_reference_double_free.c new file mode 100644 index 00000000000000..66f50424df9e7c --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test_reference_double_free.c @@ -0,0 +1,33 @@ +#include +#include +#include "../common.h" + +static void Destructor(napi_env env, void* data, void* nothing) { + napi_ref* ref = data; + NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + free(ref); +} + +static napi_value New(napi_env env, napi_callback_info info) { + size_t argc = 0; + napi_value js_this; + napi_ref* ref = malloc(sizeof(*ref)); + + NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, NULL, &js_this, NULL)); + NODE_API_CALL(env, napi_wrap(env, js_this, ref, Destructor, NULL, ref)); + NODE_API_CALL(env, napi_reference_ref(env, *ref, NULL)); + + return js_this; +} + +EXTERN_C_START +napi_value Init(napi_env env, napi_value exports) { + napi_value myobj_ctor; + NODE_API_CALL(env, + napi_define_class( + env, "MyObject", NAPI_AUTO_LENGTH, New, NULL, 0, NULL, &myobj_ctor)); + NODE_API_CALL(env, + napi_set_named_property(env, exports, "MyObject", myobj_ctor)); + return exports; +} +EXTERN_C_END From 9f723be39abe57ef9d6e6e1e0a78252d7132049e Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 10 Feb 2021 07:57:12 -0800 Subject: [PATCH 3/5] heed the linter --- test/js-native-api/test_reference_double_free/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js-native-api/test_reference_double_free/test.js b/test/js-native-api/test_reference_double_free/test.js index db9f1884a0f1c2..32a4b8f88a881d 100644 --- a/test/js-native-api/test_reference_double_free/test.js +++ b/test/js-native-api/test_reference_double_free/test.js @@ -7,4 +7,4 @@ const { buildType } = require('../../common'); const addon = require(`./build/${buildType}/test_reference_double_free`); -{ const obj = new addon.MyObject(); } +{ new addon.MyObject(); } From df5463bbb0264ed1bee6fbdab778ec889101afd7 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 11 Feb 2021 02:08:55 +0000 Subject: [PATCH 4/5] handle the case where the reference is not deleted in its destructor --- src/js_native_api_v8.cc | 4 +-- .../test_reference_double_free/test.js | 3 +- .../test_reference_double_free.c | 30 ++++++++++++++++--- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 387278e4f634f5..ccd4f4d9f3e674 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -281,10 +281,10 @@ class RefBase : protected Finalizer, RefTracker { // 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 + _finalize_ran = true; + if (_delete_self || is_env_teardown) { Delete(this); - } else { - _finalize_ran = true; } } diff --git a/test/js-native-api/test_reference_double_free/test.js b/test/js-native-api/test_reference_double_free/test.js index 32a4b8f88a881d..242d850ba9f677 100644 --- a/test/js-native-api/test_reference_double_free/test.js +++ b/test/js-native-api/test_reference_double_free/test.js @@ -7,4 +7,5 @@ const { buildType } = require('../../common'); const addon = require(`./build/${buildType}/test_reference_double_free`); -{ new addon.MyObject(); } +{ new addon.MyObject(true); } +{ new addon.MyObject(false); } diff --git a/test/js-native-api/test_reference_double_free/test_reference_double_free.c b/test/js-native-api/test_reference_double_free/test_reference_double_free.c index 66f50424df9e7c..48491ea2aaee63 100644 --- a/test/js-native-api/test_reference_double_free/test_reference_double_free.c +++ b/test/js-native-api/test_reference_double_free/test_reference_double_free.c @@ -2,19 +2,41 @@ #include #include "../common.h" +static size_t g_call_count = 0; + static void Destructor(napi_env env, void* data, void* nothing) { napi_ref* ref = data; NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); free(ref); } +static void NoDeleteDestructor(napi_env env, void* data, void* hint) { + napi_ref* ref = data; + size_t* call_count = hint; + + // This destructor must be called exactly once. + if ((*call_count) > 0) abort(); + *call_count = ((*call_count) + 1); + free(ref); +} + static napi_value New(napi_env env, napi_callback_info info) { - size_t argc = 0; - napi_value js_this; + size_t argc = 1; + napi_value js_this, js_delete; + bool delete; napi_ref* ref = malloc(sizeof(*ref)); - NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, NULL, &js_this, NULL)); - NODE_API_CALL(env, napi_wrap(env, js_this, ref, Destructor, NULL, ref)); + NODE_API_CALL(env, + napi_get_cb_info(env, info, &argc, &js_delete, &js_this, NULL)); + NODE_API_CALL(env, napi_get_value_bool(env, js_delete, &delete)); + + if (delete) { + NODE_API_CALL(env, + napi_wrap(env, js_this, ref, Destructor, NULL, ref)); + } else { + NODE_API_CALL(env, + napi_wrap(env, js_this, ref, NoDeleteDestructor, &g_call_count, ref)); + } NODE_API_CALL(env, napi_reference_ref(env, *ref, NULL)); return js_this; From 21e5b1dad3bc8d045a908d137f45e6da3515be18 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Fri, 12 Feb 2021 10:35:23 -0800 Subject: [PATCH 5/5] put the _finalize_ran = true back in the else body --- src/js_native_api_v8.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index ccd4f4d9f3e674..8275f37677d1a9 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -270,21 +270,26 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { - // Force deferring behavior if the finalizer happens to delete this - // reference. + // During environment teardown we have to convert a strong reference to + // a weak reference to force the deferring behavior if the user's finalizer + // happens to delete this reference so that the code in this function that + // follows the call to the user's finalizer may safely access variables from + // this instance. if (is_env_teardown && RefCount() > 0) _refcount = 0; if (_finalize_callback != nullptr) { _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); + // This ensures that we never call the finalizer twice. + _finalize_callback = nullptr; } // 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 - _finalize_ran = true; - if (_delete_self || is_env_teardown) { Delete(this); + } else { + _finalize_ran = true; } }