napi: keep threadsafe functions alive after their env is torn down - #34067
Conversation
A ThreadSafeFunction stores a raw pointer to the event loop of the VM that created it. Nothing neutralized that pointer when the VM went away, so an addon thread that called or released a threadsafe function created in a worker, after that worker exited, walked a freed EventLoop. next.js hits this: next-swc is a dlopen'd addon whose native threads are process-global and outlive the worker VMs, and next build segfaults at VirtualMachine.event_loop_handle with a null base pointer. Register every threadsafe function with its NapiEnv. Env cleanup now aborts them on the JS thread while JSC is still alive: it marks them closing, drains the queue back to the addon, runs the finalizer, then drops the JS callback, the event-loop keepalive and the env reference and clears the event-loop pointer. Release and call take the threadsafe function's own lock, the same one teardown takes, so once the env is dead they can never schedule onto the loop; whichever thread drops the last thread_count reference frees the object itself. Mirrors Node's ThreadSafeFunction::Cleanup -> Finalize -> MaybeDelete.
|
Updated 5:05 PM PT - Jul 13th, 2026
@Jarred-Sumner, your commit 69f5304 is building: |
|
Found 7 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
WalkthroughChangesThread-safe function teardown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi.test.ts`:
- Around line 445-456: Update the subprocess assertions in the affected test to
inspect the captured stderr before asserting exitCode. Immediately before
expect(exitCode).toBe(0), add the house-style conditional that expects stderr to
be empty only when exitCode is nonzero, preserving the existing stdout and
exit-code checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3d13c17-696f-4b13-82c2-d5df0486b7f3
📒 Files selected for processing (7)
src/jsc/bindings/napi.cppsrc/jsc/bindings/napi.hsrc/runtime/napi/napi_body.rstest/napi/napi-app/async_tests.cpptest/napi/napi-app/tsfn-orphan-worker.jstest/napi/napi-app/tsfn-orphan.jstest/napi/napi.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Moving to draft — a max-effort review found 5 confirmed correctness bugs in this change, including one that would introduce a new use-after-free:
The test in this PR does not catch it because it calls one handle and releases a different one. Also confirmed: Fixing. |
Follow-ups to the env-teardown change: - napi_call_threadsafe_function no longer frees the threadsafe function when a call on an orphaned one consumes the last thread reference. Addons (including node-addon-api's ThreadSafeFunction wrapper) release the same handle right after a call reports napi_closing, and freeing inside the call turned that into a use-after-free. Only napi_release_threadsafe_function frees now; the release reports napi_invalid_arg instead. - schedule_dispatch reaches the event loop through a shared reference. It runs on addon threads, where manufacturing &mut EventLoop aliases the JS thread's own borrow inside tick(). - The release entry point dispatches off *mut Self: it can free the object, and deallocating through a pointer derived from a live &mut self is UB. - Threadsafe-function callbacks drain microtasks between callbacks of one tick again, instead of once before the first (the flag was inverted). - Creating a threadsafe function after its env tore its threadsafe functions down now fails instead of returning a handle whose finalizer already ran; the old check tested a thread count that had just been initialized. - Env cleanup drains the cleanup-hook queue again after aborting threadsafe functions, so a hook registered by a teardown finalizer still runs. Tests: a call followed by a release of the same orphaned handle (ASAN heap-use-after-free before this change), callback-vs-microtask ordering across a multi-item tick, and creating a threadsafe function after teardown. The orphan test now compares against node instead of hardcoding its output.
|
All 5 review findings fixed in
Plus: cleanup re-drains the hook queue after Correction to my earlier claim: Node does not survive call-then-release on the same handle — v26.3 aborts in Verification: the new same-handle test is ASAN heap-use-after-free on the unfixed build and 5/5 clean on the fixed one. 12/12 threadsafe tests. Not fixed here, deliberately: |
There was a problem hiding this comment.
The registration-fails path I flagged earlier is fixed in 9eea9a605a — it now unconditionally frees and returns napi_generic_failure without publishing the handle, so the fallthrough to *result = function; env.ok() is gone. Re-review of the fix commit found no new issues, but this is intricate cross-thread lifetime management (lock-guarded teardown handoff, foreign-thread frees, *mut Self dispatch to avoid freeing through a live borrow), so it warrants a human pass.
Checked on this revision: release drops the lock guard before free_orphaned; enqueue no longer frees (only consumes the ref via release_locked); schedule_dispatch takes &EventLoop via Deref on addon threads while loop_mut is JS-thread-only; env_teardown publishes env_dead and env_teardown_done under the same lock the foreign-thread readers hold; the is_first inversion in dispatch_one is corrected and covered by the new microtask-order test.
Extended reasoning...
Overview
This PR fixes a use-after-free where a napi_threadsafe_function created in a worker holds a raw pointer to that worker's event loop, which is freed at worker shutdown while process-global addon threads (e.g. next-swc's tokio pool) still hold references. The fix adds a per-NapiEnv TSFN registry, a three-phase env_teardown() that neutralizes each TSFN before its VM dies, and an ownership-handoff protocol (env_dead / env_teardown_done) so exactly one party frees the allocation. ~250 lines of Rust in ThreadSafeFunction, ~60 lines of C++ registry plumbing in NapiEnv, and ~350 lines of new native test fixtures.
Prior review and what changed
I left one inline comment on the previous revision about the "registration fails after teardown" path returning napi_ok with a handle whose finalizer had already run. The author's fix commit 9eea9a605a addresses it (unconditional free_orphaned + generic_failure, handle never written to *result) along with four other correctness bugs found in a separate max-effort review — including a foreign-thread free inside enqueue() that would have introduced a new UAF, &mut EventLoop aliasing UB from addon threads, freeing through a live &mut self, and an inverted is_first argument. All five are fixed in the current diff and the bug hunter found nothing on re-review.
Security risks
None in the injection/auth sense. The risk surface is memory safety: cross-thread raw-pointer lifetime, lock ordering between the per-TSFN Mutex and the env's m_threadSafeFunctionsLock, and ensuring every JS-heap-touching field (StrongOptional callback, poll_ref, NapiEnvRef) is released on the JS thread before a foreign thread can free the allocation. Getting any interleaving wrong is a UAF or double-free reachable from untrusted native addons.
Level of scrutiny
High. This is exactly the class of change CLAUDE.md flags as most-blocked: cross-thread ownership with a hand-rolled refcount, atomics under a lock, and unsafe raw-pointer deallocation. The author's own max-effort pass found five real bugs in the first revision — evidence that the invariants here are subtle. The fix commit is well-reasoned (the release(this: *mut Self) refactor and the env_teardown_done handoff token are the right shapes), and test coverage is thorough (ASAN-verified UAF repro, Node parity via checkSameOutput, the call-then-release-same-handle case, late creation during teardown, and the microtask-ordering regression). But a human maintainer should confirm the full ownership matrix in the PR description holds — particularly that Debugger::AsyncTaskTracker and any other un-cleared fields are safe to drop from a foreign thread in free_orphaned, and that the deliberate leak in the enqueue-consumes-last-ref-after-teardown case is the right tradeoff.
Other factors
The PR is out of draft after the fix commit, CI build #72457 is running, and there's a noted overlap with #33968 (a less complete fix for the same bug). The napi_async_work sibling with the same shape is explicitly deferred to a follow-up with a stated reason.
…se does The last round got the ownership model backwards. A threadsafe function is owned by the JS thread while its env lives (it frees it in destroy, always with thread_count == 0), and from env_teardown_done on it is owned by the remaining thread_count references, whichever thread drops the last one freeing it. A call that reports napi_closing consumes the caller's thread reference -- node's ThreadSafeFunction::Push does the same, and a thread that stops calling after napi_closing would otherwise pin the loop forever -- so a call is a reference-drop point exactly like a release, and can be what frees. - napi_call_threadsafe_function honors that. release_locked already reported "you must free"; enqueue dropped the flag on the floor, so a call that dropped the last reference of a torn-down threadsafe function freed nothing: one ThreadSafeFunction plus its queue leaked per handle, for every worker that left one behind. It frees through the raw pointer once the lock is dropped, the way the release entry point does. This means an addon that uses a handle after a call reported napi_closing -- releasing it, say -- touches freed memory. It does in node too (Push deletes and the release aborts), which is why the docs say to make no further use of the function after napi_closing. The previous round leaked the allocation to tolerate that, and its "a call never frees" comments and test went with it. - A creation that fails no longer runs the addon's finalizer. The registration-failure path ran the teardown the threadsafe function had missed, which handed thread_finalize_data back to the addon while the addon's own error handling still owned it: a double free under any wrapper that frees on a failed create. Node's Init failure path just deletes the ThreadSafeFunction, whose destructor releases only its own resources; ours now frees only what it allocated (the Strong callback, the queue, the box). - A call on an aborted threadsafe function whose bounded queue is full reports napi_closing, not napi_queue_full. Node's Push only checks the queue when the function is open. Reporting queue_full left the caller's reference in place, and with nothing left to consume it the finalizer never ran and the event-loop keepalive pinned the process. Tests: the leak is bounded by a new bun:internal-for-testing live count -- five worker-orphaned threadsafe functions per iteration, called and never released, must all be gone before the next iteration (it reports orphaned=5 closing=5 leaked=0 five times; without the fix the second iteration already reports orphaned=10). The failed late creation now passes a finalizer that must not run. The aborted-full-queue call is compared against node.
|
Round 3 ( The fact both earlier rounds had backwards: a thread reference is dropped by
Fixed:
Removed: the Verification — every new test has a negative control that fails when the fix is reverted:
|
The two new threadsafe-function tests compared stdout against a \n-joined string, so they failed on Windows, where the child writes \r\n. The diff renders identical because \r is invisible.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/napi/napi_body.rs (1)
2547-2559: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid reborrowing the loop mutably from TSFN dispatch
src/runtime/dispatch.rs:336-337callsThreadSafeFunction::on_dispatch()fromrun_task(task, el: &mut EventLoop, ...), socall()/maybe_queue_finalizer()can overlap with the dispatcher’s live&mut EventLoop. Ifself.event_looppoints at that same loop,unsafe { back_ref.get_mut() }creates aliased mutable refs. Thread the live loop through this path or otherwise avoidget_mut()here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/napi/napi_body.rs` around lines 2547 - 2559, Avoid using ThreadSafeFunction::loop_mut to call BackRef::get_mut during TSFN dispatch, since run_task already holds a live &mut EventLoop. Thread that existing loop reference through on_dispatch/call/maybe_queue_finalizer or otherwise reuse a shared reference, ensuring no aliased mutable EventLoop is created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/napi/napi_body.rs`:
- Around line 2547-2559: Avoid using ThreadSafeFunction::loop_mut to call
BackRef::get_mut during TSFN dispatch, since run_task already holds a live &mut
EventLoop. Thread that existing loop reference through
on_dispatch/call/maybe_queue_finalizer or otherwise reuse a shared reference,
ensuring no aliased mutable EventLoop is created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdafa0e8-aae3-4c78-83ad-33fd53035acf
📒 Files selected for processing (9)
src/codegen/generate-js2native.tssrc/js/internal-for-testing.tssrc/runtime/napi/mod.rssrc/runtime/napi/napi_body.rstest/napi/napi-app/async_tests.cpptest/napi/napi-app/module.jstest/napi/napi-app/standalone_tests.cpptest/napi/napi-app/tsfn-orphan-worker.jstest/napi/napi.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi.test.ts`:
- Around line 483-486: Update both stdout normalization blocks in the NAPI tests
to have the debug-line regex consume the trailing newline when removing lines,
preventing blank lines between expected records. Preserve the existing CRLF
normalization and trimming behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06d8fc0f-a578-444e-a2f1-58fce145e157
📒 Files selected for processing (1)
test/napi/napi.test.ts
| stdout: stdout | ||
| .replaceAll("\r\n", "\n") | ||
| .replaceAll(/^\[\w+\].+$/gm, "") | ||
| .trim(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
bun -e 'const s = "orphaned=5\n[debug] native\nclosing=5\n"; console.log(JSON.stringify(s.replaceAll(/^\[\w+\].+$/gm, "").trim()))'Repository: oven-sh/bun
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant test file sections.
file="test/napi/napi.test.ts"
wc -l "$file"
sed -n '460,540p' "$file"
# Reproduce the regex behavior with Node (bun is unavailable in this sandbox).
node - <<'JS'
const samples = [
"orphaned=5\n[debug] native\nclosing=5\n",
"orphaned=5\r\n[debug] native\r\nclosing=5\r\n",
"orphaned=5\n[debug] native\n[info] extra\nclosing=5\n",
];
for (const s of samples) {
const out = s.replaceAll(/^\[\w+\].+$/gm, "").trim();
console.log(JSON.stringify({ input: s, output: out, lines: out.split("\n") }));
}
JSRepository: oven-sh/bun
Length of output: 4211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact normalization blocks in the file.
file="test/napi/napi.test.ts"
grep -n 'replaceAll(/^\[\\w\+\].\+\$/gm, "")\|replaceAll("\\r\\n", "\\n")' "$file" || true
# Show nearby context for both occurrences.
sed -n '475,492p' "$file"
sed -n '511,528p' "$file"
# Check regex behavior on newline-terminated and mid-stream log lines.
node - <<'JS'
const cases = [
["single log line between records", "orphaned=5\n[debug] native\nclosing=5\n"],
["multiple log lines between records", "orphaned=5\n[debug] native\n[info] extra\nclosing=5\n"],
["windows newlines", "orphaned=5\r\n[debug] native\r\nclosing=5\r\n"],
];
for (const [name, s] of cases) {
const out = s.replaceAll(/^\[\w+\].+$/gm, "").trim();
console.log(name);
console.log(JSON.stringify(out));
console.log(out.split("\n"));
}
JSRepository: oven-sh/bun
Length of output: 1862
Consume the newline when stripping debug lines. replaceAll(/^\[\w+\].+$/gm, "") leaves line endings behind, so any debug line between expected records turns into a blank line and breaks the exact stdout match. Apply the same fix in both normalization blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/napi/napi.test.ts` around lines 483 - 486, Update both stdout
normalization blocks in the NAPI tests to have the debug-line regex consume the
trailing newline when removing lines, preventing blank lines between expected
records. Preserve the existing CRLF normalization and trimming behavior.
There was a problem hiding this comment.
No new findings after the round-3 fixes — both earlier inline comments (registration-failure finalizer ordering, Windows CRLF in the teardown test) are addressed in the current diff. Deferring to human review given the scope: cross-thread ownership handoff in native N-API code is exactly the kind of change a maintainer should sign off on.
Checked: the env_dead/env_teardown_done handoff — every foreign-thread path that reads them holds lock, and the free only happens after the guard drops.
Checked: push/release now dispatch off *mut Self and end the borrow before free_orphaned.
Checked: abortThreadSafeFunctions() iterates a moved-out snapshot with the lock dropped, so a finalizer that unregisters/creates can't corrupt the set.
Ruled out: missing per-iteration exception isolation between TSFN finalizers — clearExceptionsBetweenFinalizers() brackets the whole abortThreadSafeFunctions() phase, and Finalizer::run doesn't propagate into the next iteration.
Extended reasoning...
Overview
This PR fixes a use-after-free in napi_threadsafe_function when the creating worker's VM is torn down while addon-owned threads still hold references. It touches src/runtime/napi/napi_body.rs (the TSFN state machine: enqueue/push, release/release_locked, env_teardown, schedule_dispatch, destroy), src/jsc/bindings/napi.{h,cpp} (a per-env TSFN registry and abortThreadSafeFunctions() wired into NapiEnv::cleanup()), plus codegen/internal-for-testing plumbing for a live-count probe and ~300 lines of new native test fixtures across async_tests.cpp, standalone_tests.cpp, module.js, a worker fixture, and napi.test.ts.
Security risks
None in the classic sense (no auth/injection surface). The risk profile here is memory safety: cross-thread ownership handoff, freeing through raw pointers, and lock-guarded state publication. A mistake manifests as UAF, double-free, leak, or hang — the PR history already surfaced and fixed five such bugs across three rounds.
Level of scrutiny
High. This is production-critical native code on the N-API hot path, with an explicit ownership model spanning the JS thread, addon threads, and env teardown. The PR itself went through three self-review rounds where the ownership model was revised (round 2's "only release frees" was overturned in round 3), and two of my own prior inline findings were fixed. That churn is a strong signal that a maintainer should read the final env_teardown / release_locked / push interaction end-to-end rather than rely on automated review alone.
Other factors
- Both prior
claude[bot]inline findings are resolved in the current diff (free_orphanedon registration failure without running the addon finalizer;.replaceAll("\\r\\n", "\\n")on the printf-based test). - The bug-hunting pass this run found nothing; the one candidate raised (per-iteration exception isolation in
abortThreadSafeFunctions()) was verified not to be an issue — the phase is bracketed byclearExceptionsBetweenFinalizers()and finalizers run throughFinalizer::run, which does not leak one finalizer's throw into the next iteration's native call. - Test coverage is unusually thorough (Node-parity via
checkSameOutput, a live-count leak test, an ASAN-targeted orphan test withMIMALLOC_PURGE_DELAY=0, and negative controls documented in the PR body), but the correctness argument still rests on a hand-stated ownership table that deserves human eyes. - The PR explicitly scopes out
napi_async_work's analogous back-pointer, which is a design call worth a maintainer nod.
Picks up the safety comment on `ThreadSafeFunction::free_orphaned`, which is what `cargo clippy` was failing on for this PR: `undocumented_unsafe_blocks` is denied workspace-wide, #34067 introduced the block without a comment, and the Clippy workflow has no `push:` trigger so main never caught it.
2 participants
What does this PR do?
Fixes a use-after-free: a
napi_threadsafe_functioncreated in a worker keeps a raw pointer to that worker's event loop, and nothing neutralizes it when the worker's VM is destroyed. A native addon thread that outlives the worker then calls or releases the TSFN and walks freed memory.This is why
bun --bun next buildsegfaults: next.js runs dozens of workers, andnext-swcis adlopen'd addon whose tokio threads are process-global.0x7EB0is the offset ofVirtualMachine.event_loop_handle. The chain:napi_release_threadsafe_function→ThreadSafeFunction::release()→schedule_dispatch()→EventLoop::enqueue_task_concurrent()→EventLoop::wakeup()→ deref of a freedVirtualMachine.The stale SAFETY claim it violates:
True for the main VM. False for workers.
Node had the same bug and fixed it upstream in v24.14 / v25.4 (
EmptyQueueAndMaybeDelete); Node ≤ v25.3 aborts on this scenario. This mirrors that fix.The fix
NapiEnvNapiEnv::cleanup()ThreadSafeFunction.event_loop/.envOption— type-enforced: no path can reach a dead looprelease()/enqueue()release_lockednapi_invalid_argatthread_count <= 0(was< 0), matching Node — a negative count could permanently defeatdispatch_one's== 0checkTeardown is three phases, mirroring Node's
Cleanup → Finalize → ReleaseResources/MaybeDelete. Every foreign-thread path that could reach the loop takes the same lock the teardown holds while publishingenv_dead, so the check-then-enqueue window is closed by the lock, not by a racy flag.Who frees, in every interleaving (
env_teardown_doneis the handoff token):thread_countref droppedthread_count == 0)The token store and the
thread_countread are in one critical section, so exactly one of the two frees.How did you verify your code works?
test/napi/napi.test.tsorphan-TSFN testnext-build.test.ts, Linux x64, releaseSIGSEGV @ 0x7EB0bun bd test test/napi/The ASAN report names it exactly: freed by
WebWorker::shutdown, used byenqueue_task_concurrent.The regression test has an addon own two unref'd TSFNs created in a worker, and makes the last call and last release from a process-global addon thread after the worker has exited — mirroring what
next-swcactually does.