napi: mark threadsafe functions closing on env teardown - #33968
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 3:02 PM PT - Jul 13th, 2026
❌ @robobun, your commit 93b6b42 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33968That installs a local version of the PR into your bun-33968 --bun |
|
No clearly related issues were found for this PR. The 28 candidate issues were reviewed, but none specifically describe a crash caused by NAPI ThreadSafeFunction use-after-free when a worker_threads Worker exits (the exact bug this PR fixes). The closest candidates were either about different NAPI mechanisms (async_work, callback conversion errors), bun:ffi threadsafe callbacks (not NAPI TSF), general worker lifecycle races without NAPI involvement, or broader architectural tracking issues (#32073). 🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a cross-thread lifetime/UAF fix in the NAPI threadsafe-function teardown path with an intentional leak of the TSF allocation — worth a human look.
Checked: release()/enqueue()/acquire() all early-return on is_closing() before touching event_loop, so the hook's atomic store is sufficient.
Checked: finalizer-before-queue-drain ordering matches Node's EmptyQueueAndDelete; ctx is passed as-is (not freed by Bun) so the drain loop's use of self_.ctx is safe.
Checked: hook is removed in destroy() before the box is freed, so normal-path teardown won't call env_cleanup on freed storage.
Extended reasoning...
Overview
The PR adds an env-cleanup hook per ThreadSafeFunction so that when a worker's VM tears down, outstanding TSFs are marked closing (under the TSF lock), their Strong JS handle is dropped, the poll ref is disabled, the user finalizer is run synchronously, and queued items are drained via call_js_cb(null, null, ctx, item). The TSF heap allocation is then intentionally left alive so a late napi_release_threadsafe_function from a native thread can still take the lock and hit the is_closing() early-return instead of dereferencing the freed event_loop. A regression test spawns a worker_threads Worker that creates an unref'd TSF, waits for the worker to exit, then releases the TSF from the main thread and asserts the finalizer ran once.
Security risks
None in the traditional sense — no user-controlled input parsing, auth, or crypto. The risk profile here is memory safety: the fix trades a heap-use-after-free for an intentional per-TSF leak on env teardown, and reorders finalizer/drain relative to the normal dispatch path.
Level of scrutiny
High. This is exactly the category the repo's review guidance calls out as most-blocked: cross-thread lifetime, refcounted env, Strong JS handles, atomics under a lock, and an intentional allocation leak. The change mirrors Node's ThreadSafeFunction::Cleanup closely and the PR description traces the mechanism precisely, but the interaction between env_cleanup and any concurrently-running dispatch() / in-flight schedule_dispatch() task, the choice to set aborted alongside closing, and the decision to leak rather than refcount all deserve a maintainer's eyes.
Other factors
The bug-hunting pass raised and ruled out the finalizer-before-drain ordering (Node does the same in EmptyQueueAndDelete; ctx ownership stays with the addon so drain-after-finalize is only unsafe if the addon frees ctx in its finalizer, which is the addon's contract to manage). The new test asserts stderr: "", which the repo guidance discourages, but it matches the established pattern in the surrounding tests in this file. CI build #71868 is still running; existing TSF and cleanup-hook test groups reportedly pass locally.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate-PR bot: #32071 covers the same underlying bug class (cross-thread enqueue into a freed worker VM) with a process-global |
|
Adversarial self-review (pushback workflow, 71 subagents across 4 rounds) plus a separate pass against Node's
Invariants confirmed with line cites:
The test now also asserts |
There was a problem hiding this comment.
I didn't find any bugs, but this is a cross-thread memory-safety fix in the NAPI TSF lifecycle (new env-cleanup hook, lock/condvar/atomic interplay, intentionally-leaked TSF box, user-finalizer invocation during teardown) and it overlaps architecturally with #32071 — worth a human look before landing.
What was reviewed:
schedule_dispatchreachability: confirmedenqueue()/release()are the only callers and both gate onis_closing()underself.lock, same lockenv_cleanupholds when setting closing.- Wait-loop predicate change:
&& !is_closing()plusqueue.count = 0before broadcast prevents parked producers re-sleeping. - Drain-then-finalizer order matches Node v24+
Finalize(); hook add/remove uses no-preamble internals so a pending VM exception can't skip them. - Test asserts
napi_closingon post-teardown call andfinalized=1; ruled out the comment-length nits as non-issues.
Extended reasoning...
Overview
This PR fixes a heap-use-after-free where a native thread calls napi_release_threadsafe_function / napi_call_threadsafe_function on a TSF whose owning worker VM has been dealloc'd. The fix registers a per-TSF env-cleanup hook (mirroring Node's ThreadSafeFunction::Cleanup) that runs during vm.on_exit(), sets closing under the TSF lock, broadcasts to wake parked producers, drops the Strong JS callback, drains queued items via call_js_cb(null, null, ctx, item), and runs the user finalizer — all before the VM box is freed. It touches src/jsc/bindings/napi.cpp (two new no-preamble internal shims), src/runtime/napi/napi_body.rs (~100 lines: extern decls, env_cleanup hook, wait-predicate changes, hook registration/removal), and adds a C++/TS test fixture.
Security risks
No direct security surface (no untrusted-input parsing, auth, or crypto). The risk profile is memory safety: the change manipulates cross-thread locks, atomics, and condvars, invokes user-supplied C callbacks (call_js_cb, finalizer) during env teardown, and intentionally leaks the TSF heap allocation so late release() from native threads can still take the lock. A mistake here would be a UAF/deadlock rather than an injection or auth bypass.
Level of scrutiny
High. This is exactly the category CLAUDE.md flags as most-blocked in review: cross-thread lifetime, refcount balance, lock/condvar ordering, calling user callbacks while state is mid-teardown, and an intentional allocation leak. The self-review comment already caught and fixed four non-trivial issues (blocked-producer deadlock, drain/finalizer order, preamble skip, queue_full vs closing), which is evidence the change is subtle enough that independent human eyes are warranted. The PR also intentionally diverges from the normal destroy() free path and leaves the box allocated — a design choice that matches Node's MaybeDelete but should be signed off by a maintainer.
Other factors
- Architectural overlap with open PR #32071 (process-global
live_vm_registry). The author argues they're complementary, but a maintainer should decide sequencing. - The bug-hunting system found no defects; two style nits (comment length, test comment content) were raised and refuted.
- Test coverage is good: a deterministic ASAN repro that asserts
napi_closingon post-teardown call,napi_okon release, and finalizer-ran-once. The 31 existing TSF/cleanup-hook tests still pass per the description. - The test asserts
stderr: ""exactly, which CLAUDE.md warns against for ASAN/debug builds — minor, but a human reviewer may want that relaxed.
Given the complexity and the memory-safety-critical nature, deferring to human review is the right call even with zero findings.
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.
- 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).
…ort 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.
249f215 to
93b6b42
Compare
There was a problem hiding this comment.
I didn't find bugs, but this is a cross-thread memory-safety change in the NAPI TSF lifecycle (locks, atomics, env-teardown ordering, intentionally-leaked allocations) and it overlaps with the open architectural PR #32071 — worth a human look before landing.
What was reviewed:
env_cleanupsetsClosedunder the lock beforerelease()/enqueue()can reachschedule_dispatch, and theClosing-only guard on the last-release-after-abort path prevents re-touching the freed loop.- The internal no-preamble add/remove hook shims — remove-before-free in
destroy()looks correctly ordered so the hook can't fire on freed storage. - The drain-then-finalize order and the
&& !is_closing()wait-predicate change match Node'sPush()/Finalize()contract as described.
Extended reasoning...
Overview
Adds a per-TSF env-cleanup hook so worker/env teardown marks NAPI threadsafe functions closing before the VM's event loop is freed, fixing a heap-use-after-free when a native thread later calls napi_release_threadsafe_function/napi_call_threadsafe_function. Touches src/runtime/napi/napi_body.rs (new env_cleanup hook, enqueue() wait-predicate change, release() Closing-only gate, hook registration in napi_create_threadsafe_function, hook removal in destroy()), src/jsc/bindings/napi.cpp (two no-preamble internal cleanup-hook entry points), plus a C++ test fixture and a worker_threads regression test.
Security risks
None specific to this change — it's a lifecycle/UAF fix in the NAPI runtime, no auth/crypto/permissions surface. The security-adjacent concern is memory safety itself, which is exactly what the PR is changing.
Level of scrutiny
High. This is squarely in the memory-safety / cross-thread-lifetime category CLAUDE.md flags as most-blocked: it introduces a new Closed state distinct from Closing, intentionally leaves the TSF box allocated so late foreign-thread calls can still take the lock, reorders finalizer/drain, and interacts with #34026's last-release-after-abort dispatch path. The reasoning in the PR description and the adversarial self-review comment is thorough and cites Node's node_api.cc line-by-line, but the correctness depends on subtle ordering invariants (no event-loop tick between vm.on_exit() and VM dealloc, schedule_dispatch reachable only under the lock and behind is_closing(), cleanup-hook removal strictly preceding the box free) that a maintainer familiar with the worker shutdown path should confirm.
Other factors
- Open PR #32071 addresses the same underlying bug class with a broader
live_vm_registrydesign and is currently marked conflicting with this one; a human should decide sequencing. - The change deliberately leaks the TSF allocation when native threads still hold references (matching Node's
MaybeDelete), which is a lifetime-policy decision worth explicit sign-off. - The new test asserts
stderr: ""exactly, which the repo guidance cautions against for ASAN/debug noise — several sibling tests in the file do the same, so it follows local convention, but it may be worth a glance if CI is flaky on the ASAN lane. - No prior human review on the PR; the only substantive review is the author's own adversarial pass.
|
CI build 72502 is done. The diff is green:
The only red annotation is Ready for review. |
|
Superseded by #34067 which landed on main with a more complete fix: a lock-guarded per-env TSFN registry, |
1 participant
Fixes
test/integration/next-pages/test/next-build.test.tswhich has been red on the musl lanes since #31216 landed (builds 71706, 71726, 71800).Reproduction
next buildunderbun --bunsegfaults in a tokio worker thread:Minimal repro (added as a test): a
worker_threadsWorker creates an unref'd TSF and exits; the main thread then callsnapi_release_threadsafe_functionon it. Under ASAN this is a deterministic heap-use-after-free atEventLoop::vm_ref, with the freed region being the worker'sVirtualMachinebox (dealloc'd inweb_worker.rs::shutdown).Cause
A
ThreadSafeFunctionholds a rawBackRef<EventLoop>into its owning VM. Bun has no mechanism to abort TSFs when their env tears down (Node registers a per-TSF env-cleanup hook; Bun did not), so after the worker's VM box is dealloc'd anynapi_release_threadsafe_function/napi_call_threadsafe_functionfrom a native thread callsschedule_dispatch()on freed memory.This was latent until #31216: that PR made
MessagePort.unref()release the event-loop ref taken by a'message'listener (correct, matches Node). Before, next's worker-side port with.on('message', fn); .unref()left onerefEventLoop()outstanding and the worker never drained. Now it drains, its VM is freed, and next-swc's tokio threads (which hold unref'd TSFs) hit the UAF when they release.Fix
Register an env-cleanup hook per TSF in
napi_create_threadsafe_functionand remove it inThreadSafeFunction::destroy. The hook runs on the owning JS thread duringvm.on_exit(), i.e. before the VM box is freed, and:closing = Closingand broadcasts to wake blocked producers;call_js_cb(null, null, ctx, item).This mirrors Node's
ThreadSafeFunction::Cleanup/CloseHandlesAndMaybeDelete(true)/EmptyQueueAndDeletein node_api.cc. Onceclosingis set,release(),enqueue()andacquire()take their existingis_closing()early-return and never touchevent_loop. The TSF struct itself is left allocated so a laterelease()from a native thread can still take the lock and read the atomic.Verification
The existing
napi_threadsafe_functionandcleanup hookstest groups (31 tests) still pass, andrust:check-allis green across all 10 targets.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts