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 3110313dddd7..e97c7afd39f4 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; } @@ -2668,6 +2681,18 @@ 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. 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_internal_remove_env_cleanup_hook( + self_.env.as_ptr(), + ThreadSafeFunction::env_cleanup, + this.cast::(), + ) + }; self_.unref(); if let Some(fun) = self_.finalizer_fun { @@ -2710,6 +2735,74 @@ 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(); + // `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::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 + // 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(); + } + } + // 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(); + // 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() { + 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); + } + } + pub fn release( &mut self, mode: napi_threadsafe_function_release_mode, @@ -2736,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(); } } @@ -2821,6 +2918,20 @@ 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. 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_internal_add_env_cleanup_hook( + env_, + 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..e418c991acca 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,58 @@ 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); + // 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; + 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 +379,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", () => {