From 4d6cdb88cc49d7e5fe35c886bb8d7fee7059ce68 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 11 Jul 2026 05:44:20 +0000 Subject: [PATCH 1/3] napi: mark threadsafe functions closing on env teardown A napi_threadsafe_function created in a worker_threads Worker holds a raw BackRef to the worker's EventLoop. When the worker exits the VM box is dealloc'd, and a later napi_release_threadsafe_function / napi_call_threadsafe_function from a native thread (next-swc's tokio runtime) would call schedule_dispatch() on the freed event loop, segfaulting in us_wakeup_loop. This became visible after #31216 made MessagePort.unref() release the listener loop-ref, so next build's workers now drain and exit instead of being accidentally pinned; next-build.test.ts has been red on musl lanes since. Register a per-TSF env-cleanup hook (mirroring Node's ThreadSafeFunction::Cleanup in node_api.cc) that, on the owning JS thread and before the VM is freed, marks the TSF closing, wakes blocked producers, drops the JS Strong handle, disables the poll ref, runs the user finalizer, and drains queued items with a null env. Once closing is set, release()/enqueue()/acquire() take their is_closing() early-return and never touch the event loop. The hook is removed in destroy() so the normal release path stays unchanged. --- src/runtime/napi/napi_body.rs | 80 ++++++++++++++++++++++++++++++ test/napi/napi-app/async_tests.cpp | 50 +++++++++++++++++++ test/napi/napi.test.ts | 33 ++++++++++++ 3 files changed, 163 insertions(+) diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index 3110313dddd7..b9ed61d65bb7 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -2668,6 +2668,16 @@ impl ThreadSafeFunction { pub unsafe fn destroy(this: *mut ThreadSafeFunction) { // SAFETY: caller contract — `this` is a live heap allocation; we consume it here. let self_ = unsafe { &mut *this }; + // Drop the env-cleanup hook registered at creation so that a later env + // teardown does not call `env_cleanup` on freed storage. + // SAFETY: env is refcounted (alive while `self_.env` holds it). + unsafe { + napi_remove_env_cleanup_hook( + self_.env.as_ptr(), + Some(ThreadSafeFunction::env_cleanup), + this.cast::(), + ) + }; self_.unref(); if let Some(fun) = self_.finalizer_fun { @@ -2710,6 +2720,63 @@ impl ThreadSafeFunction { NapiStatus::ok as napi_status } + /// Env-cleanup hook (registered in `napi_create_threadsafe_function`, removed + /// in `destroy`). Runs on the owning JS thread during `NapiEnv::cleanup()` / + /// `vm.on_exit()`, i.e. before the worker's `VirtualMachine` box is dealloc'd. + /// Marks the TSF closing so a later `napi_release_threadsafe_function` / + /// `napi_call_threadsafe_function` from a native thread takes the + /// `is_closing()` early-return instead of `schedule_dispatch()` (which would + /// touch `event_loop` after it is freed). Mirrors Node's + /// `ThreadSafeFunction::Cleanup` in node_api.cc. + extern "C" fn env_cleanup(data: *mut c_void) { + let this = data.cast::(); + // SAFETY: `this` is the live heap allocation we registered at creation; the + // hook is removed in `destroy()` before the allocation is freed. + let self_ = unsafe { &mut *this }; + { + let _g = self_.lock.lock_guard(); + self_ + .closing + .store(ClosingState::Closing as u8, Ordering::SeqCst); + self_.aborted.store(true, Ordering::SeqCst); + if self_.queue.max_queue_size > 0 { + self_.blocking_condvar.broadcast(); + } + } + // Drop the Strong JS handle while JSC is still live; keep the C call_js_cb + // pointer so we can drain queued items. Anything left in the struct is plain + // heap state that the (leaked) allocation keeps valid for any late + // `release()`/`acquire()` from native threads. + let call_js_cb = match core::mem::replace( + &mut self_.callback, + TsfnCallback::Js(StrongOptional::empty()), + ) { + TsfnCallback::C { + napi_threadsafe_function_call_js, + .. + } => Some(napi_threadsafe_function_call_js), + TsfnCallback::Js(_) => None, + }; + self_.poll_ref.disable(); + // Call the user finalizer now (the normal finalize path goes through + // `event_loop.enqueue_task`, which we can no longer touch). + if let Some(fun) = self_.finalizer_fun.take() { + let env_ptr = self_.env.as_ptr(); + // SAFETY: env is refcounted and still live during cleanup. + let env_ref = unsafe { &*env_ptr }; + let _hs = NapiHandleScope::open_scoped(env_ref); + fun(env_ptr, self_.finalizer_data, self_.ctx); + } + // Drain queued items so the addon can free per-call data (Node calls + // `call_js_cb(null, null, ctx, data)` for each). No new items can be + // enqueued once `closing` is set under the lock. + if let Some(call_js_cb) = call_js_cb { + while let Some(item) = self_.queue.data.read_item() { + call_js_cb(core::ptr::null_mut(), napi_value(0), self_.ctx, item); + } + } + } + pub fn release( &mut self, mode: napi_threadsafe_function_release_mode, @@ -2821,6 +2888,19 @@ pub(super) extern "C" fn napi_create_threadsafe_function( function_ref.ref_(); function_ref.tracker.did_schedule(vm.global()); + // Node registers an env-cleanup hook per TSF so that teardown (worker + // termination / process exit) marks it closing before the loop is freed; + // without this a native thread's later `napi_release_threadsafe_function` + // would wake a dealloc'd event loop. + // SAFETY: `env_` is the non-null napi_env we validated via get_env! above. + unsafe { + napi_add_env_cleanup_hook( + env_, + Some(ThreadSafeFunction::env_cleanup), + function.cast::(), + ) + }; + *result = function; env.ok() } diff --git a/test/napi/napi-app/async_tests.cpp b/test/napi/napi-app/async_tests.cpp index 8b1434e3f927..f7326038bbd1 100644 --- a/test/napi/napi-app/async_tests.cpp +++ b/test/napi/napi-app/async_tests.cpp @@ -1,6 +1,7 @@ #include "async_tests.h" #include "utils.h" +#include #include #include #include @@ -186,6 +187,53 @@ create_promise_with_threadsafe_function(const Napi::CallbackInfo &info) { return promise; } +// Shared between a worker thread's env and the main thread's env (dlopen loads +// the .node once per process so static storage is process-global). +static std::atomic g_late_release_tsfn{nullptr}; +static std::atomic g_late_release_finalized{0}; + +static void late_release_noop_call_js(napi_env, napi_value, void *, void *) {} + +static void late_release_finalize(napi_env, void *, void *) { + g_late_release_finalized.fetch_add(1); +} + +// Called inside a worker_thread: create a TSF and unref it so the worker's +// event loop is free to drain. The main thread releases it after the worker has +// exited (see release_tsfn_from_other_thread). +napi_value create_tsfn_for_late_release(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + napi_value resource_name = + Napi::String::New(env, "napitests::create_tsfn_for_late_release"); + napi_threadsafe_function tsfn; + NODE_API_CALL(env, napi_create_threadsafe_function( + env, nullptr, nullptr, resource_name, + /* max_queue_size */ 0, /* initial_thread_count */ 1, + /* finalize_data */ nullptr, late_release_finalize, + /* context */ nullptr, late_release_noop_call_js, + &tsfn)); + NODE_API_CALL(env, napi_unref_threadsafe_function(env, tsfn)); + g_late_release_tsfn.store(tsfn); + return info.Env().Undefined(); +} + +// Called on the main thread after the worker's 'exit' event: release the TSF +// created above. Without env-teardown handling this would schedule a dispatch +// on the worker's freed event loop (heap-use-after-free under ASAN); with it +// the TSF is already marked closing during worker env cleanup so release() +// is a no-op and the finalizer has already run. +napi_value release_tsfn_from_other_thread(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + napi_threadsafe_function tsfn = g_late_release_tsfn.exchange(nullptr); + NODE_API_ASSERT(env, tsfn != nullptr); + napi_status status = napi_release_threadsafe_function(tsfn, napi_tsfn_release); + NODE_API_ASSERT(env, status == napi_ok); + napi_value result; + NODE_API_CALL(env, + napi_create_int32(env, g_late_release_finalized.load(), &result)); + return result; +} + napi_value create_async_work_with_null_execute(const Napi::CallbackInfo &info) { napi_env env = info.Env(); @@ -326,6 +374,8 @@ void register_async_tests(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, create_promise); REGISTER_FUNCTION(env, exports, create_promise_with_napi_cpp); REGISTER_FUNCTION(env, exports, create_promise_with_threadsafe_function); + REGISTER_FUNCTION(env, exports, create_tsfn_for_late_release); + REGISTER_FUNCTION(env, exports, release_tsfn_from_other_thread); REGISTER_FUNCTION(env, exports, create_async_work_with_null_execute); REGISTER_FUNCTION(env, exports, create_async_work_with_null_complete); REGISTER_FUNCTION(env, exports, test_cancel_async_work); diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 770dcbe1b9a7..4ee1c84f92c3 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -429,6 +429,39 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { const result = await checkSameOutput("test_threadsafe_function_abort_blocked_producers", []); expect(result).toContain("finalized: true"); }); + + it("is marked closing when its worker_threads owner exits so a later release does not touch the freed loop", async () => { + // Reproduces the next-swc crash from next build: a Worker creates an + // unref'd TSF and exits; a foreign thread then releases it. Before the + // fix release() scheduled a dispatch on the worker's dealloc'd event + // loop (SIGSEGV in us_wakeup_loop on release builds, heap-use-after-free + // under ASAN). After the fix env teardown marks the TSF closing and runs + // its finalizer, so release() takes the is_closing early-return. + const addon = JSON.stringify(join(__dirname, "napi-app/build/Debug/napitests.node")); + const code = ` + const { Worker } = require("node:worker_threads"); + const main = require(${addon}); + const w = new Worker( + "require(" + ${JSON.stringify(addon)} + ").create_tsfn_for_late_release()", + { eval: true }, + ); + w.on("error", e => { console.error(e); process.exit(1); }); + w.on("exit", code => { + if (code !== 0) { console.error("worker exit " + code); process.exit(1); } + const finalized = main.release_tsfn_from_other_thread(); + console.log("finalized=" + finalized); + }); + `; + await using proc = spawn({ + cmd: [bunExe(), "-e", code], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: "finalized=1", stderr: "", exitCode: 0 }); + expect(proc.signalCode).toBeNull(); + }); }); describe("exception handling", () => { From 12b0880f79f5bb9a09f39fc83f322eabf4d4894b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 11 Jul 2026 09:24:43 +0000 Subject: [PATCH 2/3] napi(tsfn): address pushback findings on env_cleanup - enqueue(): fold !is_closing() into the blocking wait predicate and the nonblocking queue_full guard, matching Node's Push() (state == kOpen). Without this a producer parked on a full bounded queue would re-sleep after env_cleanup's broadcast, and a nonblocking caller would see napi_queue_full instead of napi_closing. - env_cleanup(): zero queue.count under the lock before broadcasting so is_blocked() goes false; drain queued items before calling the user finalizer (Node v24+ Finalize(): EmptyQueue then CallFinalizer, nodejs/node#61956) so ctx is still valid for each drained item. - Register/remove the per-TSF cleanup hook via internal no-preamble shims (napi_internal_{add,remove}_env_cleanup_hook). The public napi_{add,remove}_env_cleanup_hook start with NAPI_PREAMBLE which early-returns on a pending VM exception; a skipped add would leave the UAF reachable and a skipped remove would leave a dangling hook. - Test: assert napi_call_threadsafe_function after worker exit returns napi_closing (Node's Push contract). --- src/jsc/bindings/napi.cpp | 13 ++++++++ src/runtime/napi/napi_body.rs | 53 +++++++++++++++++++++--------- test/napi/napi-app/async_tests.cpp | 5 +++ 3 files changed, 56 insertions(+), 15 deletions(-) diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index a7f5b8143bd8..a091760d497c 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -3039,6 +3039,19 @@ extern "C" void napi_internal_cleanup_env_cpp(napi_env env) env->cleanup(); } +// No-preamble entry points for the per-TSF cleanup hook: NAPI_PREAMBLE early-returns +// on a pending VM exception, which would silently skip registration (leaving the +// UAF this hook guards against) or skip removal (leaving a dangling hook). +extern "C" void napi_internal_add_env_cleanup_hook(napi_env env, void (*function)(void*), void* data) +{ + env->addCleanupHook(function, data); +} + +extern "C" void napi_internal_remove_env_cleanup_hook(napi_env env, void (*function)(void*), void* data) +{ + env->removeCleanupHook(function, data); +} + extern "C" void napi_internal_remove_finalizer(napi_env env, napi_finalize callback, void* hint, void* data) { env->removeFinalizer(callback, hint, data); diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index b9ed61d65bb7..a6703985484c 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -2225,6 +2225,16 @@ unsafe extern "C" { ) -> napi_status; fn napi_internal_cleanup_env_cpp(env: napi_env); + fn napi_internal_add_env_cleanup_hook( + env: napi_env, + function: extern "C" fn(*mut c_void), + data: *mut c_void, + ); + fn napi_internal_remove_env_cleanup_hook( + env: napi_env, + function: extern "C" fn(*mut c_void), + data: *mut c_void, + ); fn napi_internal_check_gc(env: napi_env); } @@ -2618,12 +2628,15 @@ impl ThreadSafeFunction { pub fn enqueue(&mut self, ctx: *mut c_void, block: bool) -> napi_status { let _g = self.lock.lock_guard(); + // Node's Push() gates the wait on `state == kOpen`: once env teardown has + // set closing, a full bounded queue must fall through to the napi_closing + // return below instead of reporting queue_full or re-parking forever. if block { while self.queue.is_blocked() && !self.is_closing() { self.blocking_condvar.wait(&self.lock); } } else { - if self.queue.is_blocked() { + if self.queue.is_blocked() && !self.is_closing() { // don't set the error on the env as this is run from another thread return NapiStatus::queue_full as napi_status; } @@ -2669,12 +2682,14 @@ impl ThreadSafeFunction { // SAFETY: caller contract — `this` is a live heap allocation; we consume it here. let self_ = unsafe { &mut *this }; // Drop the env-cleanup hook registered at creation so that a later env - // teardown does not call `env_cleanup` on freed storage. + // teardown does not call `env_cleanup` on freed storage. The internal + // no-preamble entry point is used so a pending VM exception cannot + // silently skip removal and leave a dangling hook. // SAFETY: env is refcounted (alive while `self_.env` holds it). unsafe { - napi_remove_env_cleanup_hook( + napi_internal_remove_env_cleanup_hook( self_.env.as_ptr(), - Some(ThreadSafeFunction::env_cleanup), + ThreadSafeFunction::env_cleanup, this.cast::(), ) }; @@ -2739,6 +2754,11 @@ impl ThreadSafeFunction { .closing .store(ClosingState::Closing as u8, Ordering::SeqCst); self_.aborted.store(true, Ordering::SeqCst); + // Zero the public count so is_blocked() goes false; together with the + // `&& !is_closing()` guard on enqueue()'s wait loop this guarantees a + // producer parked on a full bounded queue wakes into the napi_closing + // return instead of re-sleeping. + self_.queue.count.store(0, Ordering::SeqCst); if self_.queue.max_queue_size > 0 { self_.blocking_condvar.broadcast(); } @@ -2758,6 +2778,16 @@ impl ThreadSafeFunction { TsfnCallback::Js(_) => None, }; self_.poll_ref.disable(); + // Drain queued items so the addon can free per-call data (Node calls + // `call_js_cb(null, null, ctx, data)` for each). No new items can be + // enqueued once `closing` is set under the lock. Runs before the user + // finalizer (Node v24+ `Finalize`: `EmptyQueue` then `CallFinalizer`, + // nodejs/node#61956) so `ctx` is still valid for each drained item. + if let Some(call_js_cb) = call_js_cb { + while let Some(item) = self_.queue.data.read_item() { + call_js_cb(core::ptr::null_mut(), napi_value(0), self_.ctx, item); + } + } // Call the user finalizer now (the normal finalize path goes through // `event_loop.enqueue_task`, which we can no longer touch). if let Some(fun) = self_.finalizer_fun.take() { @@ -2767,14 +2797,6 @@ impl ThreadSafeFunction { let _hs = NapiHandleScope::open_scoped(env_ref); fun(env_ptr, self_.finalizer_data, self_.ctx); } - // Drain queued items so the addon can free per-call data (Node calls - // `call_js_cb(null, null, ctx, data)` for each). No new items can be - // enqueued once `closing` is set under the lock. - if let Some(call_js_cb) = call_js_cb { - while let Some(item) = self_.queue.data.read_item() { - call_js_cb(core::ptr::null_mut(), napi_value(0), self_.ctx, item); - } - } } pub fn release( @@ -2891,12 +2913,13 @@ pub(super) extern "C" fn napi_create_threadsafe_function( // Node registers an env-cleanup hook per TSF so that teardown (worker // termination / process exit) marks it closing before the loop is freed; // without this a native thread's later `napi_release_threadsafe_function` - // would wake a dealloc'd event loop. + // would wake a dealloc'd event loop. The internal no-preamble entry point + // is used so a pending VM exception cannot silently skip registration. // SAFETY: `env_` is the non-null napi_env we validated via get_env! above. unsafe { - napi_add_env_cleanup_hook( + napi_internal_add_env_cleanup_hook( env_, - Some(ThreadSafeFunction::env_cleanup), + ThreadSafeFunction::env_cleanup, function.cast::(), ) }; diff --git a/test/napi/napi-app/async_tests.cpp b/test/napi/napi-app/async_tests.cpp index f7326038bbd1..e418c991acca 100644 --- a/test/napi/napi-app/async_tests.cpp +++ b/test/napi/napi-app/async_tests.cpp @@ -226,6 +226,11 @@ napi_value release_tsfn_from_other_thread(const Napi::CallbackInfo &info) { napi_env env = info.Env(); napi_threadsafe_function tsfn = g_late_release_tsfn.exchange(nullptr); NODE_API_ASSERT(env, tsfn != nullptr); + // A call after env teardown must return napi_closing (Node's Push() contract) + // rather than napi_queue_full or touching the freed event loop. + napi_status call_status = + napi_call_threadsafe_function(tsfn, nullptr, napi_tsfn_nonblocking); + NODE_API_ASSERT(env, call_status == napi_closing); napi_status status = napi_release_threadsafe_function(tsfn, napi_tsfn_release); NODE_API_ASSERT(env, status == napi_ok); napi_value result; From 93b6b4233d58eb120cfb96280fea91fa0ab4caa2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:40:21 +0000 Subject: [PATCH 3/3] napi(tsfn): reconcile env_cleanup with #34026's last-release-after-abort path #34026 added `else if prev_remaining == 1 { schedule_dispatch() }` to release() so the last release after an abort reaches the finalize path. That unguarded schedule_dispatch() would touch the freed event loop when the closing state came from env_cleanup (worker teardown). env_cleanup now advances the state to `Closed` (the finalizer has run synchronously, nothing left to dispatch) and the last-release-after-abort branch targets `Closing` only. #34026's three new tests and the worker-teardown repro both pass. --- src/runtime/napi/napi_body.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index a6703985484c..e97c7afd39f4 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -2750,9 +2750,13 @@ impl ThreadSafeFunction { let self_ = unsafe { &mut *this }; { let _g = self_.lock.lock_guard(); + // `Closed` (not `Closing`): the finalizer runs below, so release()'s + // last-ref-after-abort dispatch (which targets `Closing`) must not + // fire here — `event_loop` is about to be freed. self_ .closing - .store(ClosingState::Closing as u8, Ordering::SeqCst); + .store(ClosingState::Closed as u8, Ordering::SeqCst); + self_.has_queued_finalizer = true; self_.aborted.store(true, Ordering::SeqCst); // Zero the public count so is_blocked() goes false; together with the // `&& !is_closing()` guard on enqueue()'s wait loop this guarantees a @@ -2825,10 +2829,14 @@ impl ThreadSafeFunction { } } self.schedule_dispatch(); - } else if prev_remaining == 1 { + } else if prev_remaining == 1 + && self.closing.load(Ordering::SeqCst) == ClosingState::Closing as u8 + { // Already closing from an earlier abort. The last release must // still reach dispatch_one's thread_count==0 path so the // finalizer runs and the event-loop keepalive is dropped. + // `Closing` only: `Closed` means env_cleanup already ran the + // finalizer and `event_loop` may be freed. self.schedule_dispatch(); } }