napi: block worker shutdown on in-flight napi_async_work (UAF) - #36855
napi: block worker shutdown on in-flight napi_async_work (UAF)#36855robobun wants to merge 6 commits into
Conversation
worker.terminate() with napi_async_work execute callbacks still running on
the thread pool freed the worker's VirtualMachine (and its JSC heap) out
from under them. The pool-thread completion then posts into a freed
EventLoop via enqueue_task_concurrent, and the addon's execute callback
writes a freed ArrayBuffer backing store.
heap-use-after-free READ 8 thread (Bun Pool)
EventLoop::vm_ref src/jsc/event_loop.rs
EventLoop::enqueue_task_concurrent
napi_async_work::run src/runtime/napi/napi_body.rs
freed by thread (Worker): WebWorker::shutdown
Add a small work_pool_pending shutdown barrier on EventLoop:
napi_async_work::schedule() refs it before WorkPool::schedule, run()
unrefs after enqueue_task_concurrent, and WebWorker::shutdown spins on it
reaching zero before teardownJSCVM / VM dealloc. The shutdown drain then
runs complete() for each joined work so the addon can free its per-work
native state, matching Node.js.
|
Updated 6:05 PM PT - Aug 3rd, 2026
❌ @autofix-ci[bot], your commit 40d869d has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36855That installs a local version of the PR into your bun-36855 --bun |
WalkthroughChangesThe change adds an EventLoop barrier for pending WorkPool callbacks, drains queued N-API async-work completions during WebWorker shutdown, and adds regression coverage for terminating workers while native async work is active. N-API async-work shutdown
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: 3
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)
1830-1837: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPost-publish
selfderef in bothrun()exit paths.enqueue_task_concurrenthands this work item to the JS thread, which can runcompleteand free the allocation throughnapi_delete_async_work→napi_async_work::destroy. Both newwork_pool_task_unref()calls then read theevent_loopfield of that allocation. Copy theevent_loophandle (it isCopy) into a local before the enqueue and unref through the local.
src/runtime/napi/napi_body.rs#L1830-L1837: bindlet event_loop = self.event_loop;before the normal-completion enqueue and callevent_loop.work_pool_task_unref().src/runtime/napi/napi_body.rs#L1813-L1820: bind the same local before the cancelled-path enqueue and callevent_loop.work_pool_task_unref().🤖 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 1830 - 1837, Avoid dereferencing self after enqueueing the work item in both run() exit paths. In src/runtime/napi/napi_body.rs lines 1830-1837 and 1813-1820, copy self.event_loop into a local before each enqueue_task_concurrent call, then invoke work_pool_task_unref() through that local in both paths.
🤖 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 `@src/jsc/event_loop.rs`:
- Around line 1023-1032: Replace the busy-wait in
`wait_for_pending_work_pool_tasks` with an existing blocking synchronization
primitive such as `Futex`, `Condition`, or `WaitGroup`, waking waiters when
`work_pool_pending` reaches zero and preserving correct task ref/unref
synchronization. Update the method’s doc comment to remove the claim that
pending work is bounded.
In `@src/runtime/dispatch.rs`:
- Around line 1334-1346: Update global_exit() to wait for the current VM’s
work_pool_pending barrier before calling release_queued_tasks_for_shutdown().
Ensure this ordering prevents pending work-pool callbacks from retaining
NapiAsyncWork while shutdown drains and reclaims queued tasks.
In `@test/napi/napi.test.ts`:
- Around line 553-561: Update the worker promise in the test loop around the
"message" and "exit" listeners so it resolves only after receiving the expected
message and rejects if the worker exits first. Wire worker "error" events to
rejection as well, ensuring addon-load or queueWork failures cannot resolve the
test early; preserve the existing error propagation and termination flow after
successful message receipt.
---
Outside diff comments:
In `@src/runtime/napi/napi_body.rs`:
- Around line 1830-1837: Avoid dereferencing self after enqueueing the work item
in both run() exit paths. In src/runtime/napi/napi_body.rs lines 1830-1837 and
1813-1820, copy self.event_loop into a local before each enqueue_task_concurrent
call, then invoke work_pool_task_unref() through that local in both paths.
🪄 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: a9b36ecb-d4ce-41d2-b062-5d6b4284e14b
📒 Files selected for processing (7)
src/jsc/event_loop.rssrc/jsc/web_worker.rssrc/runtime/dispatch.rssrc/runtime/napi/napi_body.rstest/napi/napi-app/binding.gyptest/napi/napi-app/test_async_work_worker_terminate.ctest/napi/napi.test.ts
- napi_async_work::run(): copy the event_loop BackRef to a local before enqueue_task_concurrent so the trailing work_pool_task_unref() cannot touch self after the JS thread has already run complete() and freed it. - wait_for_pending_work_pool_tasks(): block on Futex instead of a yield_now spin; work_pool_task_unref() wakes on the 1->0 transition. Drop the doc claim that pending work is bounded (a napi execute callback is arbitrary addon code). - test: resolve only on the worker's "up" message and reject on early error/exit so an addon-load failure cannot let the loop pass silently.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Running the addon's complete() from the shutdown drain landed after NapiEnv::cleanup() (vm.on_exit() drains cleanup_hooks first), so the addon could observe freed instance data; and complete() can legally call napi_queue_async_work, which would re-schedule onto the WorkPool after the barrier returned and reopen the UAF. Both are paths this PR opened. Scope this back to the barrier alone: wait_for_pending_work_pool_tasks() before teardownJSCVM / VM dealloc keeps the EventLoop and JSC heap live across every in-flight execute+enqueue, closing the enqueue UAF and the ArrayBuffer-freed-under-execute write. The completion itself is left in the queue (re-queued by the existing default arm) and leaked at terminate, which is the pre-PR behavior. Wiring complete() into the env-close ordering (before NapiEnv::cleanup(), as a fixpoint) is a follow-up. Also trims the inline comments flagged by comment-cop.
A Futex::wake after the Release fetch_sub touched &self.work_pool_pending after the waiter may have observed zero and freed the VirtualMachine box this EventLoop lives in. The futex syscalls use the address as a kernel key only, so it was not observable at runtime, but it is a dangling reference per the Futex::wake contract. Instead the waiter futex-waits with a 1ms timeout and re-checks; the pool thread's last access to self is the fetch_sub itself. Also drop the unused napi_env field from the test addon.
With the barrier letting the completion land, the work box was being re-queued into EventLoop::deinit()'s fresh tasks buffer, which the raw dealloc of the worker VM box never frees. Add a NapiAsyncWork arm that unrefs the loop KeepAlive and destroys the box (no complete() call), so the Rust side is reclaimed while JSC is still live. The addon's per-work data pointer still leaks (complete() is not invoked on terminate by design); the test subprocess runs with detect_leaks=0 so the assertion is on the crash, not the bounded leak. Also drop the stale test comment about running complete().
|
CI at 40d869d: Remaining reds are unrelated to this diff:
Diff is ready for review. |
There was a problem hiding this comment.
No new issues found on this pass, but this is concurrency-sensitive worker-shutdown / napi lifetime code that has already iterated through several design changes in review — worth a human sign-off on the barrier shape and the new release_for_shutdown path.
What was reviewed:
work_pool_task_unrefnow ends at thefetch_sub(Release)with no post-releaseselfaccess; the timedFutex::waitwaiter avoids the freed-futex-word hazard.napi_async_work::runcopiesevent_loopbeforeenqueue_task_concurrenton both the started and cancelled branches, so no*selfread follows the ownership transfer.- The new
NapiAsyncWorkarm in__bun_release_task_at_shutdownonly sees entries the pool thread already posted; on the worker path the barrier precedes it, and onglobal_exitpresence in the queue implies the pool callback finished.
Extended reasoning...
Overview
This PR fixes a heap-use-after-free when worker.terminate() races an in-flight napi_async_work execute callback. It adds a new work_pool_pending: AtomicU32 counter on EventLoop with work_pool_task_ref/unref/wait_for_pending_work_pool_tasks, wires it into napi_async_work::schedule/run, blocks WebWorker::shutdown on it before JSC teardown, and adds a NapiAsyncWork arm to __bun_release_task_at_shutdown (release_for_shutdown: unref KeepAlive + free the box, without calling the addon's complete). A new C test addon and subprocess test cover the crash.
Security risks
None in the traditional sense. The change is a memory-safety hardening in native code; the risk surface is introducing new lifetime bugs via the new synchronization primitive or the new free-at-shutdown path, not exposing user-controlled data.
Level of scrutiny
High. This is exactly the "most-blocked category" per REVIEW.md: cross-thread lifetime, Release/Acquire ordering justification, a new field on EventLoop whose liveness contract spans the pool thread and the worker's dealloc(vm_ptr), and a change to the very delicate WebWorker::shutdown step ordering. The PR already went through three rounds of bug-hunting that found real hazards (shutdown-drain complete() re-arming the pool past the barrier; post-enqueue self.event_loop read; post-fetch_sub Futex::wake on a freeable word), each of which reshaped the design. That churn is itself a signal that a maintainer should confirm the final shape.
Other factors
- Design decisions a human should own: (1) the barrier primitive is explicitly intended to be shared with #35155 (node:zlib) and sits alongside the general #34154 design — a maintainer should confirm this is the shape they want to standardize on. (2)
release_for_shutdownchanges behavior from "re-queue and leak with the static-rooted VM box" to "free thenapi_async_workbox, leak only the addon'sdata"; the PR body's "Not in this PR" section is slightly stale on this point. (3)complete()is deliberately not run on the terminate path (backed out mid-review); the follow-up scope is documented but should be acknowledged. - Barrier placement: it sits after
vm.on_exit()/NapiEnv::cleanup(). That is fine for the barrier itself (the addonexecuteis documented no-node-api), but it constrains where a futurecomplete()drain can go — already noted in the PR body. - Test: solid fail-before/pass-after ASAN repro; sets
detect_leaks=0because the addon's per-workcallocis intentionally leaked on the terminate path. Uses fixed 300–450 ms sleeps to open the race window (not a flakiness concern here since the assertion is "did not crash", but worth noting the ~2–3 s per iteration × 3–5 iterations). - All prior review threads are resolved; no outstanding human reviewer comments.
|
#36983 generalizes this PR's counter into EventLoop.outstanding_offthread, covering every off-thread job family, and includes the napi_async_work schedule/run bracketing and the NapiAsyncWork shutdown release arm. The addon-based test here is still the only direct napi coverage (the generalized PR's matrix sticks to JS-drivable families so its fail-before stays deterministic in environments that cannot build addons), so this PR is worth keeping for the test: once #36983 lands it can rebase down to the test and binding.gyp entry. |
|
superseded by #37075, napi async work now goes through the per-vm handle that teardown closes. if the addon test here still fails before that commit feel free to resend it on its own |
3 participants
Problem
worker.terminate()with anapi_async_workstill running itsexecutecallback on the thread pool frees the worker'sVirtualMachine(and its JSC heap) out from under it:EventLoopviaenqueue_task_concurrent(heap-use-after-free / SIGSEGV on stock).executeis still writing anArrayBufferbacking store thatteardownJSCVM'slastChanceToFinalizejust freed.process.exit()inside the worker takes the same shutdown path. A natural worker exit is clean (theKeepAlivekeeps the loop alive untilcompleteruns).Fix
Add a small
work_pool_pendingshutdown barrier onEventLoop:work_pool_task_ref()(JS thread, beforeWorkPool::schedule) /work_pool_task_unref()(pool thread, afterenqueue_task_concurrent, Release) bracket the pool-thread callback's VM accesses.napi_async_work::schedule()takes the ref;run()drops it on both the cancelled and completed paths via a local copy of theBackReftaken before the enqueue (so the trailing unref does not touchselfafter the JS thread may have already freed it). Thefetch_sub(Release)is the pool thread's last access toself.WebWorker::shutdownblocks on the count reaching zero (timedFutex::wait, 1 ms re-check; the pool thread cannotFutex::wakebecause that would touchselfafter the Release) beforeteardownJSCVM/ VMdealloc. The Release/Acquire pair keeps theEventLoop, VM box, and JSC heap live for the whole pool-thread callback.__bun_release_task_at_shutdown) gains aNapiAsyncWorkarm that unrefs the loopKeepAliveand frees thenapi_async_workbox while JSC is still live, so the Rust side is fully reclaimed.Each pending work is one addon
executestep, soterminate()latency is bounded by the slowest in-flight work (same model as Node's env-closeuv_rundrain).Not in this PR
The addon's
completecallback is not invoked on the terminate path; the drain arm frees the Rust-sidenapi_async_workbox but leaves the addon'sdatapointer (whatever it hung offnapi_create_async_work) for the process to reclaim. Runningcomplete()on the shutdown drain was tried here and backed out: it lands aftervm.on_exit()has already runNapiEnv::cleanup()(instance-data / wrap finalizers), so the addon may observe freed per-env state, andcomplete()may legally callnapi_queue_async_workwhich would re-arm the pool after the barrier returned. Placing it correctly (beforeNapiEnv::cleanup(), as a fixpoint) is a follow-up alongside the general shutdown-gate work (#34154).Test
test/napi/napi.test.ts("worker.terminate() with execute callbacks in flight ...") spawns a subprocess that repeatedly creates a worker, queues fournapi_async_works that sleep 300-450 ms and memset a 16 MiBArrayBufferon the pool thread, and terminates the worker while they are in flight. The addon (test/napi/napi-app/test_async_work_worker_terminate.c) uses only public node-api. The subprocess runs withdetect_leaks=0since the addon-side per-workcallocis intentionally leaked on the terminate path.Fail-before / pass-after
rust:check-all: 10/10 target combos OK.Relationship to open PRs
Same barrier primitive as #35155 (node:zlib) applied to the napi producer; when either lands the other becomes a one-line
ref/unrefaddition. #34154 / #35767 are the general designs; this is the minimal subset for napi.