Use-after-free in `CleanupHookThunkRun` for every `node::ObjectWrap` alive at teardown

[nsavoire]

Version

v26.7.0

Platform

Linux 6.8.0-100-generic aarch64

Subsystem

No response

What steps will reproduce the bug?

Bug can be reproduced with test/addons/worker-addon-exit from node source (it creates one instance that is still live when the worker
Environment tears down):

$ cd test/addons/worker-addon-exit && npx node-gyp configure build
$ valgrind --undef-value-errors=no node test.js
...
==359== Thread 8 WorkerThread:
==359== Invalid read of size 8
==359==    at 0x89C990: node::CleanupHookThunkRun(void*) (in /usr/local/bin/node)
==359==    by 0x8C6DF7: node::CleanupQueue::Drain() (in /usr/local/bin/node)
==359==    by 0x91F31B: node::Environment::RunCleanup() (in /usr/local/bin/node)
==359==    by 0x8986B3: node::FreeEnvironment(node::Environment*) (in /usr/local/bin/node)
==359==    by 0xAF4AE7: node::worker::Worker::Run() (in /usr/local/bin/node)
==359==    by 0xAF99CF: node::worker::Worker::StartThread(v8::FunctionCallbackInfo<v8::Value> const&)::$_0::__invoke(void*) (in /usr/local/bin/node)
==359==    by 0x7CB202F: start_thread (pthread_create.c:442)
==359==    by 0x7D1BF5B: thread_start (clone.S:79)
==359==  Address 0x338aba88 is 8 bytes inside a block of size 48 free'd
==359==    at 0x7858360: operator delete(void*, unsigned long) (vg_replace_malloc.c:935)
==359==    by 0x89CB47: node::RemoveEnvironmentCleanupHook(v8::Isolate*, void (*)(void*), void*) (in /usr/local/bin/node)
==359==    by 0x54C91FB3: MyObject::~MyObject() (in /app/test/addons/worker-addon-exit/build/Release/binding.node)
==359==    by 0x54C91FF3: MyObject::~MyObject() (in /app/test/addons/worker-addon-exit/build/Release/binding.node)
==359==    by 0x89C98F: node::CleanupHookThunkRun(void*) (in /usr/local/bin/node)
==359==    by 0x8C6DF7: node::CleanupQueue::Drain() (in /usr/local/bin/node)
==359==    by 0x91F31B: node::Environment::RunCleanup() (in /usr/local/bin/node)
==359==    by 0x8986B3: node::FreeEnvironment(node::Environment*) (in /usr/local/bin/node)
==359==    by 0xAF4AE7: node::worker::Worker::Run() (in /usr/local/bin/node)
==359==    by 0xAF99CF: node::worker::Worker::StartThread(v8::FunctionCallbackInfo<v8::Value> const&)::$_0::__invoke(void*) (in /usr/local/bin/node)
==359==    by 0x7CB202F: start_thread (pthread_create.c:442)
==359==    by 0x7D1BF5B: thread_start (clone.S:79)
==359==  Block was alloc'd at
==359==    at 0x7855828: operator new(unsigned long) (vg_replace_malloc.c:422)
==359==    by 0x89DB73: std::pair<std::__detail::_Node_iterator<node::CleanupHookThunk, true, true>, bool> std::_Hashtable<node::CleanupHookThunk, node::CleanupHookThunk, std::allocator<node::CleanupHookThunk>, std::__detail::_Identity, std::equal_to<node::CleanupHookThunk>, node::CleanupHookThunkHash, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, true, true> >::_M_insert_unique<node::CleanupHookThunk, node::CleanupHookThunk, std::__detail::_AllocNode<std::allocator<std::__detail::_Hash_node<node::CleanupHookThunk, true> > > >(node::CleanupHookThunk&&, node::CleanupHookThunk&&, std::__detail::_AllocNode<std::allocator<std::__detail::_Hash_node<node::CleanupHookThunk, true> > > const&) (in /usr/local/bin/node)
==359==    by 0x89C8DB: node::AddEnvironmentCleanupHook(v8::Isolate*, void (*)(void*), void*) (in /usr/local/bin/node)
==359==    by 0x54C92053: MyObject::MyObject(double) (in /app/test/addons/worker-addon-exit/build/Release/binding.node)
==359==    by 0x54C921BF: MyObject::New(v8::FunctionCallbackInfo<v8::Value> const&) (in /app/test/addons/worker-addon-exit/build/Release/binding.node)
==359==    by 0xCAAE2F: v8::internal::FunctionCallbackArguments::CallOrConstruct(v8::internal::Isolate*, v8::internal::Tagged<v8::internal::FunctionTemplateInfo>, bool) (in /usr/local/bin/node)
==359==    by 0xCAA4E3: v8::internal::Builtin_HandleApiConstruct(int, unsigned long*, v8::internal::Isolate*) (in /usr/local/bin/node)
==359==    by 0x1A32BCB: Builtins_CEntry_Return1_ArgvOnStack_BuiltinExit (in /usr/local/bin/node)

How often does it reproduce? Is there a required condition?

100%, deterministic

What is the expected behavior? Why is that the expected behavior?

Deleting an ObjectWrap from the environment cleanup drain should not read freed memory. Since #63642, ~ObjectWrap calls RemoveEnvironmentCleanupHook, so this is now the ordinary teardown path for every ObjectWrap-based addon, not an edge case.

What do you see instead?

==359== Thread 8 WorkerThread:
==359== Invalid read of size 8
==359==    at 0x89C990: node::CleanupHookThunkRun(void*) (in /usr/local/bin/node)
==359==    by 0x8C6DF7: node::CleanupQueue::Drain() (in /usr/local/bin/node)

The process still exits 0, so this is silent unless you run under valgrind.

Additional information

Cause

src/api/hooks.cc:

static void CleanupHookThunkRun(void* arg) {
  const CleanupHookThunk* thunk = static_cast<CleanupHookThunk*>(arg);
  thunk->fun(thunk->arg);                                               // (1)
  RemoveEnvironmentCleanupHook(thunk->isolate, thunk->fun, thunk->arg); // (2)
}

(1) is ObjectWrap::CleanupHook, i.e. delete wrap. ~ObjectWrap calls RemoveEnvironmentCleanupHook itself, which does registry->erase(result) and destroys the CleanupHookThunk node that thunk points into. (2) then reads thunk->isolate / thunk->fun / thunk->arg out of that freed node. The second removal is also redundant.

Introduced by #63985, which added the registry, in combination with #63642, which made ~ObjectWrap remove its own hook.

Suggested fix

Cache the fields before the callback can invalidate the node:

static void CleanupHookThunkRun(void* arg) {
  const CleanupHookThunk* thunk = static_cast<CleanupHookThunk*>(arg);
  Isolate* isolate = thunk->isolate;
  CleanupHook fun = thunk->fun;
  void* fun_arg = thunk->arg;
  fun(fun_arg);
  RemoveEnvironmentCleanupHook(isolate, fun, fun_arg);
}