From 0ceae18f6314c9327993d5dfb429b412a8b21340 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 18 Jun 2026 17:38:03 +0200 Subject: [PATCH 1/2] src: keep global list of addon-provided cleanup hooks A recent change, 215027c8eded2e, introduced flakiness into our test suite that exposed an issue with the cleanup hook API design. Specifically, the signatures of `AddEnvironmentCleanupHook()` and `RemoveEnvironmentCleanupHook()` are problematic. Both functions take `Isolate*` arguments, as addons are not generally expected to have to care about the Node.js `Environment` as a first-class scope provider. However, this model made the incorrect assumption that in the situations in which `RemoveEnvironmentCleanupHook()` would be invoked an `Environment` would always be associated with the current `Isolate` (via the current V8 `Context`, if there is one). This occasionally breaks down when `RemoveEnvironmentCleanupHook()` is called during garbage collection -- which would be an expected use case of the functionality, but one that has not been covered through our tests before 215027c8eded2e. Since Node.js guarantees API and ABI stability within a major version, and this is a bug that is independent from the aforementioned change, this commit resolves it by adding global mutable state to keep track off cleanup hooks registered through the Node.js public API. Obviously, this solution does not represent a desirable long-term state, and a semver-minor follow up should add an API that does not require modifications to these data structures, likely based on the async cleanup hook API which already solves this issue properly. Refs: https://github.com/nodejs/node/pull/63642 Fixes: https://github.com/nodejs/node/issues/63923 Signed-off-by: Anna Henningsen PR-URL: https://github.com/nodejs/node/pull/63985 Reviewed-By: James M Snell Reviewed-By: Santiago Gimeno Reviewed-By: Matteo Collina --- src/api/hooks.cc | 59 +++++++++++++++++++++++++++-- test/addons/worker-addon/binding.cc | 19 +++++++++- 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 86132d5eec29..07efb145a29e 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -127,20 +127,71 @@ struct ACHHandle final { // this. void DeleteACHHandle::operator ()(ACHHandle* handle) const { delete handle; } +// TODO(addaleax): Having this extra set of data structures is far from +// ideal, but unfortunately the public synchronous cleanup hook API was +// slightly mis-designed; in particular, RemoveEnvironmentCleanupHook() needs +// to keep working when the Isolate either has no active context (such as +// during GC) or that context is associated with another Node.js Environment. +// We should align this with the asynchronous API, which handles this properly +// through an explicit reference to the cleanup hook instead of requiring +// lookups in internal maps. +struct CleanupHookThunk final { + Isolate* isolate; + Environment* env; + CleanupHook fun; + void* arg; + + bool operator==(const CleanupHookThunk& other) const { + // `env` is intentionally not part of this comparison + return isolate == other.isolate && fun == other.fun && arg == other.arg; + } +}; +struct CleanupHookThunkHash { + size_t operator()(const CleanupHookThunk& thunk) const { + return std::hash()(thunk.arg); + } +}; +using CleanupHookRegistry = + std::unordered_set; +static ExclusiveAccess cleanup_hook_registry; + +static void CleanupHookThunkRun(void* arg) { + const CleanupHookThunk* thunk = static_cast(arg); + thunk->fun(thunk->arg); + RemoveEnvironmentCleanupHook(thunk->isolate, thunk->fun, thunk->arg); +} + void AddEnvironmentCleanupHook(Isolate* isolate, CleanupHook fun, void* arg) { Environment* env = Environment::GetCurrent(isolate); CHECK_NOT_NULL(env); - env->AddCleanupHook(fun, arg); + void* wrapped_arg; + { + ExclusiveAccess::Scoped registry( + &cleanup_hook_registry); + auto result = registry->insert({isolate, env, fun, arg}); + CHECK(result.second); + wrapped_arg = const_cast(&*result.first); + } + env->AddCleanupHook(CleanupHookThunkRun, wrapped_arg); } void RemoveEnvironmentCleanupHook(Isolate* isolate, CleanupHook fun, void* arg) { - Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); - env->RemoveCleanupHook(fun, arg); + CleanupHookThunk thunk; + void* wrapped_arg; + { + ExclusiveAccess::Scoped registry( + &cleanup_hook_registry); + auto result = registry->find({isolate, nullptr, fun, arg}); + if (result == registry->end()) return; + wrapped_arg = const_cast(&*result); + thunk = *result; + registry->erase(result); + } + thunk.env->RemoveCleanupHook(CleanupHookThunkRun, wrapped_arg); } static void FinishAsyncCleanupHook(void* arg) { diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc index a5f9d8b3f835..1fdfcc3a13e2 100644 --- a/test/addons/worker-addon/binding.cc +++ b/test/addons/worker-addon/binding.cc @@ -55,8 +55,23 @@ void Initialize(Local exports, context->GetIsolate(), Cleanup, const_cast(static_cast("cleanup"))); - node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); - node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); + + // Test that adding and removing a cleanup hook works as expected + { + node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); + node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); + } + + // Test that adding and removing a cleanup hook also works if there + // is no active context during removal + { + node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); + { + context->Exit(); + node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); + context->Enter(); + } + } if (getenv("addExtraItemToEventLoop") != nullptr) { // Add an item to the event loop that we do not clean up in order to make From fa73926c5f6185eeb1293226f4597f49ee15b42c Mon Sep 17 00:00:00 2001 From: Caleb Everett Date: Fri, 4 Sep 2026 09:56:42 -0700 Subject: [PATCH 2/2] src: fix use-after-free in CleanupHookThunkRun CleanupHookThunkRun() read thunk->isolate/fun/arg from the CleanupHookThunk after invoking thunk->fun(). For every node::ObjectWrap alive at teardown, thunk->fun is ObjectWrap::CleanupHook, which deletes the wrap; ~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, erasing the CleanupHookThunk from the registry and freeing the node it lives in. The subsequent read of thunk->isolate/fun/arg to make the (now redundant) second RemoveEnvironmentCleanupHook() call was therefore a use-after-free. Cache the fields before running the hook so nothing is read from `thunk` once it may have been freed. Taken over from #65196, which has been inactive; the original change is unmodified apart from the added comment. This also unblocks #65042, the backport of the cleanup hook registry to v24.x. Without that registry ~ObjectWrap() asserts during garbage collection, so every 24.x runtime aborts for ObjectWrap addons (#65446), as do 26.x runtimes before 26.4.0 when used with newer headers (#65262). Fixes: https://github.com/nodejs/node/issues/65195 Refs: https://github.com/nodejs/node/pull/65196 Refs: https://github.com/nodejs/node/pull/65042 Refs: https://github.com/nodejs/node/issues/65446 Refs: https://github.com/nodejs/node/issues/65262 Assisted-by: a closed-source coding agent Co-authored-by: Sreehari Annam Signed-off-by: Caleb Everett PR-URL: https://github.com/nodejs/node/pull/65630 Reviewed-By: Trivikram Kamat Reviewed-By: Shelley Vohr --- src/api/hooks.cc | 11 +++++++-- test/cctest/test_environment.cc | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 07efb145a29e..3213a30a7bc7 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -157,8 +157,15 @@ static ExclusiveAccess cleanup_hook_registry; static void CleanupHookThunkRun(void* arg) { const CleanupHookThunk* thunk = static_cast(arg); - thunk->fun(thunk->arg); - RemoveEnvironmentCleanupHook(thunk->isolate, thunk->fun, thunk->arg); + // `thunk->fun` may itself remove and free this CleanupHookThunk (e.g. via + // ~ObjectWrap(), which calls RemoveEnvironmentCleanupHook()), so cache the + // fields we still need before invoking it rather than reading them from + // `thunk` afterwards. + Isolate* isolate = thunk->isolate; + CleanupHook fun = thunk->fun; + void* fun_arg = thunk->arg; + fun(fun_arg); + RemoveEnvironmentCleanupHook(isolate, fun, fun_arg); } void AddEnvironmentCleanupHook(Isolate* isolate, diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index a219d5125701..3ccd7845849b 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -27,6 +27,12 @@ static void at_exit_callback_ordered2(void* arg); static void at_exit_js(void* arg); static std::string cb_1_arg; // NOLINT(runtime/string) +struct SelfRemovingCleanupHookState { + v8::Isolate* isolate; + bool ran = false; +}; +static void self_removing_cleanup_hook(void* arg); + class EnvironmentTest : public EnvironmentTestFixture { private: void TearDown() override { @@ -309,6 +315,27 @@ TEST_F(EnvironmentTest, AtExitRunsJS) { EXPECT_TRUE(called_at_exit_js); } +// A cleanup hook that removes itself while the environment cleanup queue is +// being drained must not cause a use-after-free. This registers such a hook +// directly rather than through node::ObjectWrap, whose destructor removes +// its own hook and is what makes this reachable for addons since #63642. +// The use-after-free is silent in ordinary builds; it is caught by the +// ASan/Valgrind CI, which is also how the original assertion (#63923) +// surfaced. Regression test for https://github.com/nodejs/node/issues/65195. +TEST_F(EnvironmentTest, RemoveEnvironmentCleanupHookDuringCleanup) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + SelfRemovingCleanupHookState state{isolate_}; + { + Env env{handle_scope, argv}; + node::AddEnvironmentCleanupHook( + isolate_, self_removing_cleanup_hook, &state); + // Destroying `env` runs FreeEnvironment() -> RunCleanup(), which drains + // the cleanup queue and invokes CleanupHookThunkRun() for the hook above. + } + EXPECT_TRUE(state.ran); +} + TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { const v8::HandleScope handle_scope(isolate_); const Argv argv; @@ -392,6 +419,19 @@ static void at_exit_js(void* arg) { called_at_exit_js = true; } +// Reproduces the sequence node::ObjectWrap performs since +// https://github.com/nodejs/node/pull/63642, without using ObjectWrap +// itself: the hook removes its own environment cleanup hook. When that runs +// while the cleanup queue is being drained, CleanupHookThunkRun() must not +// read the CleanupHookThunk after invoking the hook -- the hook has already +// erased and freed it. See https://github.com/nodejs/node/issues/65195. +static void self_removing_cleanup_hook(void* arg) { + auto* state = static_cast(arg); + state->ran = true; + node::RemoveEnvironmentCleanupHook( + state->isolate, self_removing_cleanup_hook, state); +} + TEST_F(EnvironmentTest, SetImmediateCleanup) { int called = 0; int called_unref = 0;