Repository: denoland/deno
Pull request: fix(ext/napi): pass valid env to tsfn call_js_cb after close race

## Summary

- Fix SIGSEGV crash in native addons (e.g. node-pty) caused by a race condition in NAPI threadsafe function teardown
- When a tsfn is released while calls are still pending, `TsFn::drop` can run before a queued call is processed. Previously, the `call_js_cb` was invoked with `env=NULL`, crashing addons that dereference env without a null check (SIGSEGV at address 0x90)
- Fix captures the env pointer in the call closure so it's available after the TsFn is freed. The env is always valid (leaked via `Box::into_raw`, never freed) and V8 is alive (callback runs on V8 thread)
- Add regression test `test_tsfn_close_race` that spawns concurrent calling/releasing threads to exercise the race

Towards https://github.com/denoland/deno/issues/32846

Changed files:
filename: ext/napi/node_api.rs
patch: @@ -824,15 +824,24 @@ impl TsFn {
     let data = SendPtr(data);
     let context = SendPtr(self.context);
     let call_js_cb = self.call_js_cb;
+    // Capture env so we can pass it even after the tsfn is freed. The env
+    // pointer is always valid here because it was created via Box::into_raw
+    // in op_napi_open and is intentionally leaked (never freed).
+    let env = SendPtr(self.env);
 
     self.sender.spawn(move |scope: &mut v8::PinScope<'_, '_>| {
       let data = data.take();
 
-      // if is_closed then tsfn is freed, don't read from it.
+      // If is_closed then the TsFn struct has been freed. Don't read from
+      // the tsfn pointer. We still pass the real env (not null) because:
+      // 1. The env is valid (leaked via Box::into_raw, never freed)
+      // 2. V8 is alive (we're running on the V8 thread with a scope)
+      // 3. Many native addons (e.g. node-pty) dereference env without a
+      //    null check, causing SIGSEGV if we pass null
       if is_closed.load(Ordering::Relaxed) {
         unsafe {
           call_js_cb(
-            std::ptr::null_mut(),
+            env.take() as _,
             None::<v8::Local<v8::Value>>.into(),
             context.take() as _,
             data as _,

filename: tests/napi/async_test.js
patch: @@ -28,3 +28,26 @@ Deno.test("napi async work with threadsafe function from execute", async () => {
   });
   assertEquals(called, true);
 });
+
+Deno.test("napi tsfn call_js_cb receives valid env after close race", async () => {
+  // Reproduces a crash seen with node-pty: when a tsfn is released while
+  // calls are still pending, the call_js_cb must receive a valid env (not
+  // null). The native test spawns two threads (one calling the tsfn and
+  // one releasing it) to trigger the race. The call_js_cb asserts env is
+  // not null and uses it, which would SIGSEGV before the fix.
+  await new Promise((resolve) => {
+    asyncTask.test_tsfn_close_race(() => {
+      resolve();
+    });
+  });
+});
+
+Deno.test("napi tsfn call_js_cb receives valid env after abort race", async () => {
+  // Same race as above, but uses napi_tsfn_abort mode to close the tsfn.
+  // Abort forces immediate close regardless of thread_count.
+  await new Promise((resolve) => {
+    asyncTask.test_tsfn_abort_race(() => {
+      resolve();
+    });
+  });
+});

filename: tests/napi/src/async.rs
patch: @@ -141,7 +141,7 @@ unsafe extern "C" fn tsfn_call_js(
 ) {
   // Release the tsfn from the JS thread after being called.
   // We get the tsfn from the callback info that was stashed earlier.
-  // For simplicity, just do nothing here — the complete callback handles cleanup.
+  // For simplicity, just do nothing here. The complete callback handles cleanup.
   let _ = env;
 }
 
@@ -240,6 +240,200 @@ extern "C" fn test_async_work_with_tsfn(
   ptr::null_mut()
 }
 
+// Test that call_js_cb receives a valid (non-null) env even when the tsfn
+// is closed before all queued calls are processed. This reproduces a crash
+// seen with node-pty: a race between calling and releasing threads causes
+// TsFn::drop to run before a pending call, and the call_js_cb was invoked
+// with env=NULL, crashing addons that dereference env without a null check.
+
+unsafe extern "C" fn tsfn_race_call_js(
+  env: napi_env,
+  _js_callback: napi_value,
+  _context: *mut c_void,
+  _data: *mut c_void,
+) {
+  // Simulate what node-pty does: use the env parameter without null check.
+  // Before the fix, env could be null here when the tsfn was already closed.
+  assert!(
+    !env.is_null(),
+    "call_js_cb received null env, this is the bug that crashes node-pty"
+  );
+  unsafe {
+    let mut global: napi_value = ptr::null_mut();
+    assert_napi_ok!(napi_get_global(env, &mut global));
+  }
+}
+
+unsafe extern "C" fn tsfn_race_finalize(
+  env: napi_env,
+  finalize_data: *mut c_void,
+  _hint: *mut c_void,
+) {
+  // Called when the tsfn is fully closed. Signal completion to JS.
+  unsafe {
+    let func = finalize_data as napi_ref;
+    let mut global: napi_value = ptr::null_mut();
+    assert_napi_ok!(napi_get_global(env, &mut global));
+    let mut callback: napi_value = ptr::null_mut();
+    assert_napi_ok!(napi_get_reference_value(env, func, &mut callback));
+    let mut _result: napi_value = ptr::null_mut();
+    assert_napi_ok!(napi_call_function(
+      env,
+      global,
+      callback,
+      0,
+      ptr::null(),
+      &mut _result
+    ));
+    assert_napi_ok!(napi_delete_reference(env, func));
+  }
+}
+
+extern "C" fn test_tsfn_close_race(
+  env: napi_env,
+  info: napi_callback_info,
+) -> napi_value {
+  let (args, argc, _) = napi_get_callback_info!(env, info, 1);
+  assert_eq!(argc, 1);
+
+  let mut resource_name: napi_value = ptr::null_mut();
+  assert_napi_ok!(napi_create_string_utf8(
+    env,
+    c"tsfn_race".as_ptr(),
+    usize::MAX,
+    &mut resource_name,
+  ));
+
+  let mut func: napi_ref = ptr::null_mut();
+  assert_napi_ok!(napi_create_reference(env, args[0], 1, &mut func));
+
+  let mut tsfn: napi_threadsafe_function = ptr::null_mut();
+  assert_napi_ok!(napi_create_threadsafe_function(
+    env,
+    ptr::null_mut(), // no JS func
+    ptr::null_mut(), // no async resource
+    resource_name,
+    0,                        // unlimited queue
+    1,                        // initial_thread_count
+    func as *mut c_void,      // thread_finalize_data
+    Some(tsfn_race_finalize), // thread_finalize_cb
+    ptr::null_mut(),          // context
+    Some(tsfn_race_call_js),  // call_js_cb
+    &mut tsfn,
+  ));
+
+  // napi_threadsafe_function is a raw pointer which is !Send.
+  // Cast to usize so we can move it into std::thread::spawn.
+  let tsfn_addr = tsfn as usize;
+
+  // Thread A: calls the tsfn many times. The calls are queued to the main
+  // thread via sender.spawn. Some of these calls may be processed after the
+  // tsfn is dropped (if Thread B's release wins the spawn race).
+  let tsfn_a = tsfn_addr;
+  std::thread::spawn(move || {
+    let tsfn = tsfn_a as napi_threadsafe_function;
+    for _ in 0..100 {
+      let status = unsafe {
+        napi_call_threadsafe_function(
+          tsfn,
+          ptr::null_mut(),
+          ThreadsafeFunctionCallMode::nonblocking,
+        )
+      };
+      if status != napi_ok {
+        break; // tsfn is closing
+      }
+    }
+  });
+
+  // Thread B: releases the tsfn, which triggers the close (thread_count
+  // drops from 1 to 0). The drop is queued via sender.spawn. If Thread A's
+  // sender.spawn calls are interleaved with this, some calls will land in
+  // the queue after the drop. Those calls see is_closed=true.
+  let tsfn_b = tsfn_addr;
+  std::thread::spawn(move || {
+    let tsfn = tsfn_b as napi_threadsafe_function;
+    unsafe {
+      napi_release_threadsafe_function(
+        tsfn,
+        ThreadsafeFunctionReleaseMode::release,
+      );
+    }
+  });
+
+  ptr::null_mut()
+}
+
+// Same as test_tsfn_close_race but uses napi_tsfn_abort instead of
+// napi_tsfn_release to close the tsfn. Abort mode immediately marks
+// the tsfn as closing regardless of thread_count.
+extern "C" fn test_tsfn_abort_race(
+  env: napi_env,
+  info: napi_callback_info,
+) -> napi_value {
+  let (args, argc, _) = napi_get_callback_info!(env, info, 1);
+  assert_eq!(argc, 1);
+
+  let mut resource_name: napi_value = ptr::null_mut();
+  assert_napi_ok!(napi_create_string_utf8(
+    env,
+    c"tsfn_abort".as_ptr(),
+    usize::MAX,
+    &mut resource_name,
+  ));
+
+  let mut func: napi_ref = ptr::null_mut();
+  assert_napi_ok!(napi_create_reference(env, args[0], 1, &mut func));
+
+  let mut tsfn: napi_threadsafe_function = ptr::null_mut();
+  assert_napi_ok!(napi_create_threadsafe_function(
+    env,
+    ptr::null_mut(),
+    ptr::null_mut(),
+    resource_name,
+    0,
+    1,
+    func as *mut c_void,
+    Some(tsfn_race_finalize),
+    ptr::null_mut(),
+    Some(tsfn_race_call_js),
+    &mut tsfn,
+  ));
+
+  let tsfn_addr = tsfn as usize;
+
+  let tsfn_a = tsfn_addr;
+  std::thread::spawn(move || {
+    let tsfn = tsfn_a as napi_threadsafe_function;
+    for _ in 0..100 {
+      let status = unsafe {
+        napi_call_threadsafe_function(
+          tsfn,
+          ptr::null_mut(),
+          ThreadsafeFunctionCallMode::nonblocking,
+        )
+      };
+      if status != napi_ok {
+        break;
+      }
+    }
+  });
+
+  // Use abort mode to close. This forces close regardless of thread_count.
+  let tsfn_b = tsfn_addr;
+  std::thread::spawn(move || {
+    let tsfn = tsfn_b as napi_threadsafe_function;
+    unsafe {
+      napi_release_threadsafe_function(
+        tsfn,
+        ThreadsafeFunctionReleaseMode::abort,
+      );
+    }
+  });
+
+  ptr::null_mut()
+}
+
 pub fn init(env: napi_env, exports: napi_value) {
   let properties = &[
     napi_new_property!(env, "test_async_work", test_async_work),
@@ -248,6 +442,8 @@ pub fn init(env: napi_env, exports: napi_value) {
       "test_async_work_with_tsfn",
       test_async_work_with_tsfn
     ),
+    napi_new_property!(env, "test_tsfn_close_race", test_tsfn_close_race),
+    napi_new_property!(env, "test_tsfn_abort_race", test_tsfn_abort_race),
   ];
 
   assert_napi_ok!(napi_define_properties(