From 82b6116e0bedb06444c0cef22fc89a315ab88a49 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 9 Feb 2021 22:52:54 -0800 Subject: [PATCH 1/3] 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 PR-URL: https://github.com/nodejs/node/pull/37303 Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 9 +++ .../test_reference_double_free/binding.gyp | 11 ++++ .../test_reference_double_free/test.js | 11 ++++ .../test_reference_double_free.c | 55 +++++++++++++++++++ 4 files changed, 86 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/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index e037c4297de0c5..8275f37677d1a9 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -270,8 +270,17 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { + // 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 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..242d850ba9f677 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test.js @@ -0,0 +1,11 @@ +'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`); + +{ 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 new file mode 100644 index 00000000000000..f38867d220ae68 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test_reference_double_free.c @@ -0,0 +1,55 @@ +#include +#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; + NAPI_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 = 1; + napi_value js_this, js_delete; + bool delete; + napi_ref* ref = malloc(sizeof(*ref)); + + NAPI_CALL(env, + napi_get_cb_info(env, info, &argc, &js_delete, &js_this, NULL)); + NAPI_CALL(env, napi_get_value_bool(env, js_delete, &delete)); + + if (delete) { + NAPI_CALL(env, + napi_wrap(env, js_this, ref, Destructor, NULL, ref)); + } else { + NAPI_CALL(env, + napi_wrap(env, js_this, ref, NoDeleteDestructor, &g_call_count, ref)); + } + NAPI_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; + NAPI_CALL(env, + napi_define_class( + env, "MyObject", NAPI_AUTO_LENGTH, New, NULL, 0, NULL, &myobj_ctor)); + NAPI_CALL(env, + napi_set_named_property(env, exports, "MyObject", myobj_ctor)); + return exports; +} +EXTERN_C_END From 466d76826dc83874e24901733aa7b8ad842fcca2 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 3 Mar 2021 20:37:27 -0800 Subject: [PATCH 2/3] node-api: stop ref gc during environment teardown A gc may happen during environment teardown. Thus, during finalization initiated by environment teardown we must remove the V8 finalizer before calling the Node-API finalizer. Fixes: https://github.com/nodejs/node/issues/37236 PR-URL: https://github.com/nodejs/node/pull/37616 Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 28 +++++++++++++- test/node-api/test_env_teardown_gc/binding.c | 37 +++++++++++++++++++ .../node-api/test_env_teardown_gc/binding.gyp | 8 ++++ test/node-api/test_env_teardown_gc/test.js | 14 +++++++ 4 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 test/node-api/test_env_teardown_gc/binding.c create mode 100644 test/node-api/test_env_teardown_gc/binding.gyp create mode 100644 test/node-api/test_env_teardown_gc/test.js diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 8275f37677d1a9..602ded21569cd7 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -270,6 +270,20 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { + // In addition to being called during environment teardown, this method is + // also the entry point for the garbage collector. During environment + // teardown we have to remove the garbage collector's reference to this + // method so that, if, as part of the user's callback, JS gets executed, + // resulting in a garbage collection pass, this method is not re-entered as + // part of that pass, because that'll cause a double free (as seen in + // https://github.com/nodejs/node/issues/37236). + // + // Since this class does not have access to the V8 persistent reference, + // this method is overridden in the `Reference` class below. Therein the + // weak callback is removed, ensuring that the garbage collector does not + // re-enter this method, and the method chains up to continue the process of + // environment-teardown-induced finalization. + // 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 @@ -278,9 +292,10 @@ class RefBase : protected Finalizer, RefTracker { 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. + napi_finalize fini = _finalize_callback; _finalize_callback = nullptr; + _env->CallFinalizer(fini, _finalize_data, _finalize_hint); } // this is safe because if a request to delete the reference @@ -355,6 +370,17 @@ class Reference : public RefBase { } } + protected: + inline void Finalize(bool is_env_teardown = false) override { + // During env teardown, `~napi_env()` alone is responsible for finalizing. + // Thus, we don't want any stray gc passes to trigger a second call to + // `Finalize()`, so let's reset the persistent here. + if (is_env_teardown) _persistent.ClearWeak(); + + // Chain up to perform the rest of the finalization. + RefBase::Finalize(is_env_teardown); + } + private: // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does diff --git a/test/node-api/test_env_teardown_gc/binding.c b/test/node-api/test_env_teardown_gc/binding.c new file mode 100644 index 00000000000000..f894690b2470b2 --- /dev/null +++ b/test/node-api/test_env_teardown_gc/binding.c @@ -0,0 +1,37 @@ +#include +#include +#include "../../js-native-api/common.h" + +static void MyObject_fini(napi_env env, void* data, void* hint) { + napi_ref* ref = data; + napi_value global; + napi_value cleanup; + NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &global)); + NAPI_CALL_RETURN_VOID( + env, napi_get_named_property(env, global, "cleanup", &cleanup)); + napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL); + // We may not be allowed to call into JS, in which case a pending exception + // will be returned. + NAPI_ASSERT_RETURN_VOID(env, + status == napi_ok || status == napi_pending_exception, + "Unexpected status for napi_call_function"); + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + free(ref); +} + +static napi_value MyObject(napi_env env, napi_callback_info info) { + napi_value js_this; + napi_ref* ref = malloc(sizeof(*ref)); + NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL)); + NAPI_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref)); + return NULL; +} + +NAPI_MODULE_INIT() { + napi_value ctor; + NAPI_CALL( + env, napi_define_class( + env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor)); + NAPI_CALL(env, napi_set_named_property(env, exports, "MyObject", ctor)); + return exports; +} diff --git a/test/node-api/test_env_teardown_gc/binding.gyp b/test/node-api/test_env_teardown_gc/binding.gyp new file mode 100644 index 00000000000000..413621ade335a1 --- /dev/null +++ b/test/node-api/test_env_teardown_gc/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.c' ] + } + ] +} diff --git a/test/node-api/test_env_teardown_gc/test.js b/test/node-api/test_env_teardown_gc/test.js new file mode 100644 index 00000000000000..8f1c4f4038fbab --- /dev/null +++ b/test/node-api/test_env_teardown_gc/test.js @@ -0,0 +1,14 @@ +'use strict'; +// Flags: --expose-gc + +process.env.NODE_TEST_KNOWN_GLOBALS = 0; + +const common = require('../../common'); +const binding = require(`./build/${common.buildType}/binding`); + +global.it = new binding.MyObject(); + +global.cleanup = () => { + delete global.it; + global.gc(); +}; From 180c33dbd446dfe251ea475f72b927a0f6802641 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 23 Mar 2021 11:26:18 -0400 Subject: [PATCH 3/3] node-api: fix crash in finalization Refs: https://github.com/nodejs/node-addon-api/issues/906 Refs: https://github.com/nodejs/node/pull/37616 Fix crash introduced by https://github.com/nodejs/node/pull/37616 Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/37876 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott Reviewed-By: James M Snell --- src/js_native_api_v8.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 602ded21569cd7..664aa2d41bbc98 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -374,8 +374,11 @@ class Reference : public RefBase { inline void Finalize(bool is_env_teardown = false) override { // During env teardown, `~napi_env()` alone is responsible for finalizing. // Thus, we don't want any stray gc passes to trigger a second call to - // `Finalize()`, so let's reset the persistent here. - if (is_env_teardown) _persistent.ClearWeak(); + // `Finalize()`, so let's reset the persistent here if nothing is + // keeping it alive. + if (is_env_teardown && _persistent.IsWeak()) { + _persistent.ClearWeak(); + } // Chain up to perform the rest of the finalization. RefBase::Finalize(is_env_teardown);