From 893ed58ca0aa086a2cc8f7d93ae51805ab901e5b Mon Sep 17 00:00:00 2001 From: Sreehari Annam Date: Mon, 10 Aug 2026 10:58:05 -0400 Subject: [PATCH 1/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. Fixes: https://github.com/nodejs/node/issues/65195 --- src/api/hooks.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 09a79fb9da81..b46073b6b7cf 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -145,8 +145,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, From 5cf34f77de1cb2e06e792096b6ad1beddb4b097e Mon Sep 17 00:00:00 2001 From: Caleb Everett Date: Wed, 19 Aug 2026 22:39:13 +0000 Subject: [PATCH 2/2] test: add regression test for cleanup hook self-removal UAF Add a cctest that registers an environment cleanup hook which removes itself while the cleanup queue is drained -- the ordinary teardown path for every node::ObjectWrap still alive at exit since #63642. It exercises CleanupHookThunkRun(), which must not read the CleanupHookThunk after invoking the hook, because the hook has already erased and freed it. The use-after-free is silent in ordinary builds and is caught by the ASan/Valgrind CI, which is how the original assertion (#63923) surfaced. Refs: https://github.com/nodejs/node/issues/65195 Co-authored-by: Sreehari Annam Co-authored-by: nsavoire <19255994+nsavoire@users.noreply.github.com> Signed-off-by: Caleb Everett --- test/cctest/test_environment.cc | 39 +++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 59c71835499e..9203c4f8d713 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -28,6 +28,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 { @@ -289,6 +295,26 @@ 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. Since #63642 this is the +// ordinary teardown path for every node::ObjectWrap still alive at exit. +// 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; @@ -372,6 +398,19 @@ static void at_exit_js(void* arg) { called_at_exit_js = true; } +// Mirrors what node::ObjectWrap does since +// https://github.com/nodejs/node/pull/63642: the destructor removes the +// object's 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;