From 7b6b7d343edb7b4d23a75a2e3992f8284914f1dd Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 3 Mar 2021 20:37:27 -0800 Subject: [PATCH] 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_reference_double_free.c | 2 +- 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 +++++++ 5 files changed, 87 insertions(+), 2 deletions(-) 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 5163503beea7c1..e969adc3918ec4 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -269,6 +269,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 @@ -277,9 +291,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 @@ -354,6 +369,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/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 45329e075954c9..f38867d220ae68 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 @@ -6,7 +6,7 @@ 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)); + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); free(ref); } 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(); +};