From 5a9a6cf7e9f0ec8a9a237c5e5eab14b177dd05e1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 3 Aug 2026 21:47:59 +0000 Subject: [PATCH 1/6] napi: block worker shutdown on in-flight napi_async_work (UAF) 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. --- src/jsc/event_loop.rs | 44 ++++++- src/jsc/web_worker.rs | 8 ++ src/runtime/dispatch.rs | 13 ++ src/runtime/napi/napi_body.rs | 39 +++++- test/napi/napi-app/binding.gyp | 11 ++ .../test_async_work_worker_terminate.c | 113 ++++++++++++++++++ test/napi/napi.test.ts | 56 +++++++++ 7 files changed, 282 insertions(+), 2 deletions(-) create mode 100644 test/napi/napi-app/test_async_work_worker_terminate.c diff --git a/src/jsc/event_loop.rs b/src/jsc/event_loop.rs index 204affdd8d02..da0eece25b02 100644 --- a/src/jsc/event_loop.rs +++ b/src/jsc/event_loop.rs @@ -10,7 +10,7 @@ //! poll deadline). See PORTING.md §Dispatch. use core::ptr::NonNull; -use core::sync::atomic::{AtomicI32, AtomicPtr, Ordering}; +use core::sync::atomic::{AtomicI32, AtomicPtr, AtomicU32, Ordering}; use bun_io::{self as Async, Waker}; use bun_uws as uws; @@ -88,6 +88,15 @@ pub struct EventLoop { pub entered_event_loop_count: isize, pub concurrent_ref: AtomicI32, + /// Count of `WorkPool` jobs scheduled from this VM's JS thread whose + /// pool-thread callback has not yet finished its last access to this + /// `EventLoop` (typically `enqueue_task_concurrent`). `WebWorker::shutdown` + /// spins on this reaching zero before freeing the JSC heap and the + /// `VirtualMachine` box that this `EventLoop` is a field of; without that + /// barrier the pool thread's callback (and its completion post) are + /// use-after-free. Bracket with [`Self::work_pool_task_ref`] / + /// [`Self::work_pool_task_unref`]. + pub work_pool_pending: AtomicU32, /// Atomic nullable pointer to the next-due `WTFTimer`. /// /// Note (§Dispatch): payload is `*mut ()` — the real @@ -128,6 +137,7 @@ impl Default for EventLoop { uws_loop: (), entered_event_loop_count: 0, concurrent_ref: AtomicI32::new(0), + work_pool_pending: AtomicU32::new(0), imminent_gc_timer: AtomicPtr::new(core::ptr::null_mut()), #[cfg(unix)] signal_handler: None, @@ -989,6 +999,38 @@ impl EventLoop { self.wakeup(); } + /// JS-thread: call immediately before `WorkPool::schedule` for a task whose + /// pool-thread callback will dereference this `EventLoop` / the owning + /// `VirtualMachine` / the JSC heap (e.g. to post a completion via + /// [`Self::enqueue_task_concurrent`]). Paired with + /// [`Self::work_pool_task_unref`] on the pool thread; see + /// [`Self::work_pool_pending`]. + #[inline] + pub fn work_pool_task_ref(&self) { + self.work_pool_pending.fetch_add(1, Ordering::Relaxed); + } + + /// Pool-thread: call as the last `EventLoop`/VM access in the `WorkPool` + /// callback (after [`Self::enqueue_task_concurrent`]). The `Release` store + /// pairs with [`Self::wait_for_pending_work_pool_tasks`]'s `Acquire` load + /// so `WebWorker::shutdown` cannot observe zero until every prior access + /// is visible, making the subsequent VM dealloc safe. + #[inline] + pub fn work_pool_task_unref(&self) { + self.work_pool_pending.fetch_sub(1, Ordering::Release); + } + + /// Worker-thread shutdown barrier. Spins (yielding) until every + /// outstanding [`Self::work_pool_task_ref`] has been matched by + /// [`Self::work_pool_task_unref`]. Each pending task is one bounded + /// `execute`/compression/IO step, so the wait is bounded; `terminate()` + /// already runs user exit handlers before this. + pub fn wait_for_pending_work_pool_tasks(&self) { + while self.work_pool_pending.load(Ordering::Acquire) > 0 { + std::thread::yield_now(); + } + } + pub fn ref_concurrently(&self) { let _ = self.concurrent_ref.fetch_add(1, Ordering::SeqCst); self.wakeup(); diff --git a/src/jsc/web_worker.rs b/src/jsc/web_worker.rs index 7f30e0f913e2..4c428c9351db 100644 --- a/src/jsc/web_worker.rs +++ b/src/jsc/web_worker.rs @@ -1303,6 +1303,14 @@ impl WebWorker { // or observes m_isShuttingDown under m_lock and drops. Idempotent; // teardownJSCVM sets it again. Bun__JSCTaskScheduler__markShuttingDown(vm.global()); + // Wait for in-flight WorkPool jobs scheduled from this VM + // (napi_async_work today; see `work_pool_pending`). The pool-thread + // callback reads this VM's `EventLoop` and JSC-heap-backed buffers + // the addon may hold; both are freed below (teardownJSCVM / step-5 + // dealloc). Runs before the drain so the completion each job posts + // is picked up by `__bun_release_task_at_shutdown` while JSC is + // still live. + vm.event_loop_shared().wait_for_pending_work_pool_tasks(); // Reclaim queued CppTasks (the per-worker stdio/messaging // MessagePort drain tasks that can be in self.tasks mid-tick when // terminate() lands, and any Worker dispatchExit close task from a diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index fb6f5506de64..de130cd25d4b 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -1331,6 +1331,19 @@ fn __bun_release_task_at_shutdown(task: bun_event_loop::Task) -> bool { unsafe { Bun__deleteDeferredWorkTask(task.ptr.cast::()) }; true } + // A `napi_async_work` completion that reached the queue after the + // worker's last tick (the `work_pool_pending` barrier guarantees the + // post lands before this drain). Run the addon's `complete` callback + // so it can free its per-work native state; JSC is still live here. + task_tag::NapiAsyncWork => { + // SAFETY: tag identifies pointee; the pool-thread callback ran + // (it posted this entry and `work_pool_task_unref()`'d), so the + // threadpool no longer holds the embedded `task` field. + unsafe { + napi_async_work::run_from_js_for_shutdown(task.ptr.cast::()) + }; + true + } // Same reclaim `drop_concurrent_cpp_tasks` performs, but for tasks // that were already batch-moved into `self.tasks`. Must run before // JSC teardown: a Worker `dispatchExit` lambda's `~Ref` walks diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index bb0506b0b16d..af9bd517e2a3 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -1758,7 +1758,10 @@ impl napi_async_work { env: unsafe { NapiEnvRef::clone_from_raw(env.as_mut_ptr()) }, execute, // SAFETY: `event_loop()` is the live JS-thread loop (non-null, - // stable address) and outlives every napi_async_work. + // stable address). Liveness across a worker `terminate()` is + // guaranteed by `schedule()`'s `work_pool_task_ref()`: the worker + // shutdown barrier waits for `run()` to `work_pool_task_unref()` + // before the VM box (and this `EventLoop`) are freed. event_loop: unsafe { bun_ptr::BackRef::from_raw(global.bun_vm().event_loop()) }, complete, data, @@ -1783,6 +1786,10 @@ impl napi_async_work { } self.scheduled = true; self.poll_ref.ref_(bun_io::js_vm_ctx()); + // Hold the worker-shutdown barrier open until the pool thread has + // finished `execute` and posted the completion; matched by + // `work_pool_task_unref()` at the end of `run()`. + self.event_loop.work_pool_task_ref(); WorkPool::schedule(&raw mut self.task); } @@ -1808,6 +1815,9 @@ impl napi_async_work { self.concurrent_task .from(self_ptr, AutoDeinit::ManualDeinit), )); + // Last EventLoop/VM access; pairs with `work_pool_task_ref()` + // in `schedule()` and releases `WebWorker::shutdown`'s barrier. + self.event_loop.work_pool_task_unref(); return; } } @@ -1822,6 +1832,33 @@ impl napi_async_work { self.concurrent_task .from(self_ptr, AutoDeinit::ManualDeinit), )); + // Last EventLoop/VM access; pairs with `work_pool_task_ref()` in + // `schedule()` and releases `WebWorker::shutdown`'s barrier. + self.event_loop.work_pool_task_unref(); + } + + /// Shutdown-drain counterpart of [`Self::run_from_js`] for a completion + /// that reached the queue after the worker thread stopped ticking (the + /// `work_pool_pending` barrier in `WebWorker::shutdown` guarantees the + /// post lands before this drain). Runs on the worker's JS thread with the + /// JSC heap and VM still live (before `teardownJSCVM`), so it is safe to + /// invoke the addon's `complete` callback here. Node.js does the same via + /// its env-close `uv_run` drain: `complete` runs with the status `execute` + /// left, so the addon can free its per-work native state. + /// + /// # Safety + /// `this` must be the live heap `napi_async_work` whose embedded + /// `concurrent_task` was popped from the shutdown drain; the pool thread + /// no longer holds it. + pub(crate) unsafe fn run_from_js_for_shutdown(this: *mut napi_async_work) { + // `complete` may `napi_delete_async_work(self)`, so copy the global + // handle out before forming `&mut *this` (matches the normal dispatch + // arm, which passes `vm`/`global` from the loop, not from `self`). + // SAFETY: see fn contract. + let global: GlobalRef = unsafe { (*this).global }; + // SAFETY: see fn contract; `run_from_js` is documented to tolerate + // `self` being freed by the user's `complete`. + unsafe { (*this).run_from_js(global.bun_vm().as_mut(), &global) }; } pub(crate) fn cancel(&mut self) -> bool { diff --git a/test/napi/napi-app/binding.gyp b/test/napi/napi-app/binding.gyp index 94f07ed09603..be26636aa0c0 100644 --- a/test/napi/napi-app/binding.gyp +++ b/test/napi/napi-app/binding.gyp @@ -286,5 +286,16 @@ "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1", ], }, + { + "target_name": "test_async_work_worker_terminate", + "sources": ["test_async_work_worker_terminate.c"], + "include_dirs": [" +#include +#include +#include + +#ifdef _WIN32 +#include +static void sleep_ms(unsigned ms) { Sleep(ms); } +#else +#include +static void sleep_ms(unsigned ms) { usleep(ms * 1000); } +#endif + +#define CHECK(env, call) \ + do { \ + napi_status s_ = (call); \ + if (s_ != napi_ok) { \ + napi_throw_error((env), NULL, #call " failed"); \ + return NULL; \ + } \ + } while (0) + +typedef struct { + napi_env env; + napi_async_work work; + napi_ref buf_ref; + napi_ref cb_ref; + unsigned char *data; + size_t len; + unsigned sleep_ms; +} work_t; + +static void exec_cb(napi_env env, void *arg) { + work_t *w = (work_t *)arg; + sleep_ms(w->sleep_ms); + // Touch the ArrayBuffer backing store. Before the fix, worker.terminate() + // could free it (via JSC VM teardown running the ArrayBuffer finalizer) + // while this callback is still running on the pool thread. + if (w->len > 0) { + volatile unsigned char first = w->data[0]; + (void)first; + memset(w->data, 0xab, w->len); + } +} + +static void done_cb(napi_env env, napi_status status, void *arg) { + work_t *w = (work_t *)arg; + napi_value cb = NULL, undef = NULL, argv[1]; + if (w->cb_ref != NULL) { + napi_get_reference_value(env, w->cb_ref, &cb); + } + napi_get_undefined(env, &undef); + napi_create_int32(env, (int)status, &argv[0]); + if (cb != NULL) { + napi_call_function(env, undef, cb, 1, argv, NULL); + } + napi_delete_reference(env, w->buf_ref); + if (w->cb_ref != NULL) napi_delete_reference(env, w->cb_ref); + napi_delete_async_work(env, w->work); + free(w); +} + +// queueWork(arrayBuffer, ms[, cb]) -> undefined +static napi_value queue_work(napi_env env, napi_callback_info info) { + size_t argc = 3; + napi_value argv[3]; + CHECK(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); + if (argc < 2) { + napi_throw_error(env, NULL, "expected (arrayBuffer, ms[, cb])"); + return NULL; + } + + work_t *w = (work_t *)calloc(1, sizeof(*w)); + w->env = env; + + void *data = NULL; + size_t len = 0; + CHECK(env, napi_get_arraybuffer_info(env, argv[0], &data, &len)); + w->data = (unsigned char *)data; + w->len = len; + + int32_t ms = 0; + CHECK(env, napi_get_value_int32(env, argv[1], &ms)); + w->sleep_ms = (unsigned)(ms < 0 ? 0 : ms); + + CHECK(env, napi_create_reference(env, argv[0], 1, &w->buf_ref)); + if (argc >= 3) { + napi_valuetype t; + CHECK(env, napi_typeof(env, argv[2], &t)); + if (t == napi_function) { + CHECK(env, napi_create_reference(env, argv[2], 1, &w->cb_ref)); + } + } + + napi_value name; + CHECK(env, napi_create_string_utf8(env, "test_async_work_worker_terminate", + NAPI_AUTO_LENGTH, &name)); + CHECK(env, napi_create_async_work(env, NULL, name, exec_cb, done_cb, w, + &w->work)); + CHECK(env, napi_queue_async_work(env, w->work)); + + napi_value undef; + CHECK(env, napi_get_undefined(env, &undef)); + return undef; +} + +NAPI_MODULE_INIT() { + napi_value fn; + CHECK(env, napi_create_function(env, "queueWork", NAPI_AUTO_LENGTH, + queue_work, NULL, &fn)); + CHECK(env, napi_set_named_property(env, exports, "queueWork", fn)); + return exports; +} diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 63902deafb10..64a8faed1c85 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -518,6 +518,62 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { expect(output).toContain("success!"); expect(output).not.toContain("failure!"); }); + // worker.terminate() while execute callbacks are still running on the + // thread pool must not free the worker's VirtualMachine (the pool-thread + // completion would enqueue into a freed EventLoop) or its JSC heap (the + // ArrayBuffer backing store would be finalized while the addon is still + // writing it). WebWorker::shutdown now waits for every queued + // napi_async_work's pool-thread callback to finish and then runs + // complete() on the shutdown drain, matching Node.js. The positive + // assertion on stdout catches every crash mode (panic, ASAN abort, + // SIGSEGV) without matching on "panic" in stderr; on an unfixed build + // the subprocess aborts before printing PASS. + it("worker.terminate() with execute callbacks in flight waits for them and does not UAF", async () => { + const addon = join(__dirname, "napi-app/build/Debug/test_async_work_worker_terminate.node"); + const workerSrc = /* js */ ` + const { parentPort, workerData } = require("node:worker_threads"); + const addon = require(workerData.addon); + const keep = []; + for (let i = 0; i < 4; i++) { + const ab = new ArrayBuffer(16 << 20); + keep.push(ab); + addon.queueWork(ab, 300 + i * 50, () => {}); + } + parentPort.postMessage("up"); + setInterval(() => {}, 1000); + `; + const script = /* js */ ` + const { Worker } = require("node:worker_threads"); + (async () => { + for (let r = 0; r < ${isASAN ? 3 : 5}; r++) { + const w = new Worker(process.env.WORKER_SRC, { + eval: true, + workerData: { addon: process.env.ADDON }, + }); + let err; + w.on("error", e => { err = e; }); + await new Promise(resolve => { + w.once("message", resolve); + w.once("exit", resolve); + }); + if (err) throw err; + await w.terminate(); + } + console.log("PASS"); + })().catch(e => { + console.error(String(e)); + process.exit(1); + }); + `; + await using proc = spawn({ + cmd: [bunExe(), "-e", script], + env: { ...bunEnv, ADDON: addon, WORKER_SRC: workerSrc }, + 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: "PASS", stderr: "", exitCode: 0 }); + }, 30_000); }); describe("napi_threadsafe_function", () => { From 830ecdf2a12c7bfb7ffcd68e962b603fc586acf0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:04:08 +0000 Subject: [PATCH 2/6] address review: copy event_loop before enqueue; futex wait; tighten test - 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. --- src/jsc/event_loop.rs | 23 +++++++++++++++-------- src/runtime/napi/napi_body.rs | 27 +++++++++++++++------------ test/napi/napi.test.ts | 8 +++----- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/jsc/event_loop.rs b/src/jsc/event_loop.rs index da0eece25b02..9b49e5918f7b 100644 --- a/src/jsc/event_loop.rs +++ b/src/jsc/event_loop.rs @@ -1017,17 +1017,24 @@ impl EventLoop { /// is visible, making the subsequent VM dealloc safe. #[inline] pub fn work_pool_task_unref(&self) { - self.work_pool_pending.fetch_sub(1, Ordering::Release); + if self.work_pool_pending.fetch_sub(1, Ordering::Release) == 1 { + bun_threading::Futex::wake(&self.work_pool_pending, u32::MAX); + } } - /// Worker-thread shutdown barrier. Spins (yielding) until every - /// outstanding [`Self::work_pool_task_ref`] has been matched by - /// [`Self::work_pool_task_unref`]. Each pending task is one bounded - /// `execute`/compression/IO step, so the wait is bounded; `terminate()` - /// already runs user exit handlers before this. + /// Worker-thread shutdown barrier. Blocks until every outstanding + /// [`Self::work_pool_task_ref`] has been matched by + /// [`Self::work_pool_task_unref`]. The wait is bounded only by the + /// slowest in-flight pool callback (for `napi_async_work` that is + /// arbitrary addon code); Node.js's env-close `uv_run` drain has the + /// same `terminate()` latency model. pub fn wait_for_pending_work_pool_tasks(&self) { - while self.work_pool_pending.load(Ordering::Acquire) > 0 { - std::thread::yield_now(); + loop { + let n = self.work_pool_pending.load(Ordering::Acquire); + if n == 0 { + return; + } + let _ = bun_threading::Futex::wait(&self.work_pool_pending, n, None); } } diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index af9bd517e2a3..b9836afc67fc 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -1801,6 +1801,11 @@ impl napi_async_work { fn run(&mut self) { let self_ptr: *mut Self = self; + // After `enqueue_task_concurrent` the JS thread may pick this work up, + // run `complete`, and `napi_delete_async_work` it before we reach the + // `work_pool_task_unref()` below; copy the handle out so that last + // access does not touch `self`. + let event_loop = self.event_loop; if let Err(state) = self.status.compare_exchange( AsyncWorkStatus::Pending as u32, AsyncWorkStatus::Started as u32, @@ -1810,14 +1815,13 @@ impl napi_async_work { if state == AsyncWorkStatus::Cancelled as u32 { // `concurrent_task` is the live inline field of this heap work; // the queue takes ownership of its `next` link. - self.event_loop - .enqueue_task_concurrent(core::ptr::NonNull::from( - self.concurrent_task - .from(self_ptr, AutoDeinit::ManualDeinit), - )); + event_loop.enqueue_task_concurrent(core::ptr::NonNull::from( + self.concurrent_task + .from(self_ptr, AutoDeinit::ManualDeinit), + )); // Last EventLoop/VM access; pairs with `work_pool_task_ref()` // in `schedule()` and releases `WebWorker::shutdown`'s barrier. - self.event_loop.work_pool_task_unref(); + event_loop.work_pool_task_unref(); return; } } @@ -1827,14 +1831,13 @@ impl napi_async_work { // `concurrent_task` is the live inline field of this heap work; the // queue takes ownership of its `next` link. - self.event_loop - .enqueue_task_concurrent(core::ptr::NonNull::from( - self.concurrent_task - .from(self_ptr, AutoDeinit::ManualDeinit), - )); + event_loop.enqueue_task_concurrent(core::ptr::NonNull::from( + self.concurrent_task + .from(self_ptr, AutoDeinit::ManualDeinit), + )); // Last EventLoop/VM access; pairs with `work_pool_task_ref()` in // `schedule()` and releases `WebWorker::shutdown`'s barrier. - self.event_loop.work_pool_task_unref(); + event_loop.work_pool_task_unref(); } /// Shutdown-drain counterpart of [`Self::run_from_js`] for a completion diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index 64a8faed1c85..f71c76648675 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -550,13 +550,11 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { eval: true, workerData: { addon: process.env.ADDON }, }); - let err; - w.on("error", e => { err = e; }); - await new Promise(resolve => { + await new Promise((resolve, reject) => { w.once("message", resolve); - w.once("exit", resolve); + w.once("error", reject); + w.once("exit", code => reject(new Error("worker exited before queueing work, code " + code))); }); - if (err) throw err; await w.terminate(); } console.log("PASS"); From c2dfcc6e413e3f49e880a1dac42ad6a257092f79 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:23:58 +0000 Subject: [PATCH 3/6] drop shutdown-drain complete(): barrier-only fix 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. --- src/jsc/web_worker.rs | 10 +++------- src/runtime/dispatch.rs | 13 ------------- src/runtime/napi/napi_body.rs | 32 +------------------------------- 3 files changed, 4 insertions(+), 51 deletions(-) diff --git a/src/jsc/web_worker.rs b/src/jsc/web_worker.rs index 4c428c9351db..24913b51ab2a 100644 --- a/src/jsc/web_worker.rs +++ b/src/jsc/web_worker.rs @@ -1303,13 +1303,9 @@ impl WebWorker { // or observes m_isShuttingDown under m_lock and drops. Idempotent; // teardownJSCVM sets it again. Bun__JSCTaskScheduler__markShuttingDown(vm.global()); - // Wait for in-flight WorkPool jobs scheduled from this VM - // (napi_async_work today; see `work_pool_pending`). The pool-thread - // callback reads this VM's `EventLoop` and JSC-heap-backed buffers - // the addon may hold; both are freed below (teardownJSCVM / step-5 - // dealloc). Runs before the drain so the completion each job posts - // is picked up by `__bun_release_task_at_shutdown` while JSC is - // still live. + // Join in-flight WorkPool jobs (napi_async_work; see + // `work_pool_pending`) whose pool-thread callback reads this VM's + // EventLoop / JSC heap, both freed below. vm.event_loop_shared().wait_for_pending_work_pool_tasks(); // Reclaim queued CppTasks (the per-worker stdio/messaging // MessagePort drain tasks that can be in self.tasks mid-tick when diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index de130cd25d4b..fb6f5506de64 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -1331,19 +1331,6 @@ fn __bun_release_task_at_shutdown(task: bun_event_loop::Task) -> bool { unsafe { Bun__deleteDeferredWorkTask(task.ptr.cast::()) }; true } - // A `napi_async_work` completion that reached the queue after the - // worker's last tick (the `work_pool_pending` barrier guarantees the - // post lands before this drain). Run the addon's `complete` callback - // so it can free its per-work native state; JSC is still live here. - task_tag::NapiAsyncWork => { - // SAFETY: tag identifies pointee; the pool-thread callback ran - // (it posted this entry and `work_pool_task_unref()`'d), so the - // threadpool no longer holds the embedded `task` field. - unsafe { - napi_async_work::run_from_js_for_shutdown(task.ptr.cast::()) - }; - true - } // Same reclaim `drop_concurrent_cpp_tasks` performs, but for tasks // that were already batch-moved into `self.tasks`. Must run before // JSC teardown: a Worker `dispatchExit` lambda's `~Ref` walks diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index b9836afc67fc..9b5005157976 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -1786,9 +1786,7 @@ impl napi_async_work { } self.scheduled = true; self.poll_ref.ref_(bun_io::js_vm_ctx()); - // Hold the worker-shutdown barrier open until the pool thread has - // finished `execute` and posted the completion; matched by - // `work_pool_task_unref()` at the end of `run()`. + // Matched by `work_pool_task_unref()` at the end of `run()`. self.event_loop.work_pool_task_ref(); WorkPool::schedule(&raw mut self.task); } @@ -1819,8 +1817,6 @@ impl napi_async_work { self.concurrent_task .from(self_ptr, AutoDeinit::ManualDeinit), )); - // Last EventLoop/VM access; pairs with `work_pool_task_ref()` - // in `schedule()` and releases `WebWorker::shutdown`'s barrier. event_loop.work_pool_task_unref(); return; } @@ -1835,35 +1831,9 @@ impl napi_async_work { self.concurrent_task .from(self_ptr, AutoDeinit::ManualDeinit), )); - // Last EventLoop/VM access; pairs with `work_pool_task_ref()` in - // `schedule()` and releases `WebWorker::shutdown`'s barrier. event_loop.work_pool_task_unref(); } - /// Shutdown-drain counterpart of [`Self::run_from_js`] for a completion - /// that reached the queue after the worker thread stopped ticking (the - /// `work_pool_pending` barrier in `WebWorker::shutdown` guarantees the - /// post lands before this drain). Runs on the worker's JS thread with the - /// JSC heap and VM still live (before `teardownJSCVM`), so it is safe to - /// invoke the addon's `complete` callback here. Node.js does the same via - /// its env-close `uv_run` drain: `complete` runs with the status `execute` - /// left, so the addon can free its per-work native state. - /// - /// # Safety - /// `this` must be the live heap `napi_async_work` whose embedded - /// `concurrent_task` was popped from the shutdown drain; the pool thread - /// no longer holds it. - pub(crate) unsafe fn run_from_js_for_shutdown(this: *mut napi_async_work) { - // `complete` may `napi_delete_async_work(self)`, so copy the global - // handle out before forming `&mut *this` (matches the normal dispatch - // arm, which passes `vm`/`global` from the loop, not from `self`). - // SAFETY: see fn contract. - let global: GlobalRef = unsafe { (*this).global }; - // SAFETY: see fn contract; `run_from_js` is documented to tolerate - // `self` being freed by the user's `complete`. - unsafe { (*this).run_from_js(global.bun_vm().as_mut(), &global) }; - } - pub(crate) fn cancel(&mut self) -> bool { self.status .compare_exchange( From 6b08a71123f8374441d4fa38195fa29928efe93d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:39:35 +0000 Subject: [PATCH 4/6] unref: drop post-release Futex::wake; timed wait in the barrier 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. --- src/jsc/event_loop.rs | 14 +++++++++----- .../napi-app/test_async_work_worker_terminate.c | 2 -- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/jsc/event_loop.rs b/src/jsc/event_loop.rs index 9b49e5918f7b..e9cf5b56f3f5 100644 --- a/src/jsc/event_loop.rs +++ b/src/jsc/event_loop.rs @@ -1014,12 +1014,12 @@ impl EventLoop { /// callback (after [`Self::enqueue_task_concurrent`]). The `Release` store /// pairs with [`Self::wait_for_pending_work_pool_tasks`]'s `Acquire` load /// so `WebWorker::shutdown` cannot observe zero until every prior access - /// is visible, making the subsequent VM dealloc safe. + /// is visible, making the subsequent VM dealloc safe. This must be the + /// last access to `self`: once the waiter observes zero it may free the + /// `VirtualMachine` box this `EventLoop` lives in. #[inline] pub fn work_pool_task_unref(&self) { - if self.work_pool_pending.fetch_sub(1, Ordering::Release) == 1 { - bun_threading::Futex::wake(&self.work_pool_pending, u32::MAX); - } + self.work_pool_pending.fetch_sub(1, Ordering::Release); } /// Worker-thread shutdown barrier. Blocks until every outstanding @@ -1034,7 +1034,11 @@ impl EventLoop { if n == 0 { return; } - let _ = bun_threading::Futex::wait(&self.work_pool_pending, n, None); + // Timed wait: the pool thread cannot `Futex::wake` here because + // its last safe access to `self` is the `fetch_sub` above, after + // which this `EventLoop` may be freed. 1ms re-check adds at most + // 1ms to `terminate()`. + let _ = bun_threading::Futex::wait(&self.work_pool_pending, n, Some(1_000_000)); } } diff --git a/test/napi/napi-app/test_async_work_worker_terminate.c b/test/napi/napi-app/test_async_work_worker_terminate.c index 6d45e9533475..90a36aba1598 100644 --- a/test/napi/napi-app/test_async_work_worker_terminate.c +++ b/test/napi/napi-app/test_async_work_worker_terminate.c @@ -21,7 +21,6 @@ static void sleep_ms(unsigned ms) { usleep(ms * 1000); } } while (0) typedef struct { - napi_env env; napi_async_work work; napi_ref buf_ref; napi_ref cb_ref; @@ -71,7 +70,6 @@ static napi_value queue_work(napi_env env, napi_callback_info info) { } work_t *w = (work_t *)calloc(1, sizeof(*w)); - w->env = env; void *data = NULL; size_t len = 0; From 342c0101bfcd426819f7581cbc4c0bd18b68e8a1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:49:59 +0000 Subject: [PATCH 5/6] shutdown drain: free the napi_async_work box instead of re-queueing 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(). --- src/runtime/dispatch.rs | 6 ++++++ src/runtime/napi/napi_body.rs | 14 ++++++++++++++ test/napi/napi.test.ts | 12 ++++++------ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index fb6f5506de64..c164ab6aeb9c 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -1331,6 +1331,12 @@ fn __bun_release_task_at_shutdown(task: bun_event_loop::Task) -> bool { unsafe { Bun__deleteDeferredWorkTask(task.ptr.cast::()) }; true } + task_tag::NapiAsyncWork => { + // SAFETY: tag identifies pointee; the pool-thread callback already + // posted this entry (`work_pool_pending` barrier). + unsafe { napi_async_work::release_for_shutdown(task.ptr.cast::()) }; + true + } // Same reclaim `drop_concurrent_cpp_tasks` performs, but for tasks // that were already batch-moved into `self.tasks`. Must run before // JSC teardown: a Worker `dispatchExit` lambda's `~Ref` walks diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index 9b5005157976..18b90aba636d 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -1780,6 +1780,20 @@ impl napi_async_work { drop(unsafe { bun_core::heap::take(this) }); } + /// Shutdown-drain release: unref the loop `KeepAlive` taken in + /// `schedule()` and free the box. Does not call `complete` (it would run + /// after `NapiEnv::cleanup()`); the addon's `data` is left for the + /// process to reclaim. + /// + /// # Safety + /// `this` must be the heap work popped from the shutdown drain; the pool + /// thread no longer holds it (`work_pool_pending` barrier). + pub(crate) unsafe fn release_for_shutdown(this: *mut napi_async_work) { + // SAFETY: see fn contract. + unsafe { core::mem::take(&mut (*this).poll_ref) }.unref(bun_io::js_vm_ctx()); + Self::destroy(this); + } + pub(crate) fn schedule(&mut self) { if self.scheduled { return; diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index f71c76648675..fff620117dd0 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -523,11 +523,8 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { // completion would enqueue into a freed EventLoop) or its JSC heap (the // ArrayBuffer backing store would be finalized while the addon is still // writing it). WebWorker::shutdown now waits for every queued - // napi_async_work's pool-thread callback to finish and then runs - // complete() on the shutdown drain, matching Node.js. The positive - // assertion on stdout catches every crash mode (panic, ASAN abort, - // SIGSEGV) without matching on "panic" in stderr; on an unfixed build - // the subprocess aborts before printing PASS. + // napi_async_work's pool-thread callback to finish before teardown. On an + // unfixed build the subprocess aborts before printing PASS. it("worker.terminate() with execute callbacks in flight waits for them and does not UAF", async () => { const addon = join(__dirname, "napi-app/build/Debug/test_async_work_worker_terminate.node"); const workerSrc = /* js */ ` @@ -565,7 +562,10 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { `; await using proc = spawn({ cmd: [bunExe(), "-e", script], - env: { ...bunEnv, ADDON: addon, WORKER_SRC: workerSrc }, + // complete() is not invoked on the terminate path (see PR body), so the + // addon's per-work calloc leaks by design. LSan stays off so the crash + // assertion below is what decides pass/fail. + env: { ...bunEnv, ADDON: addon, WORKER_SRC: workerSrc, ASAN_OPTIONS: "detect_leaks=0:allow_user_segv_handler=1" }, stdout: "pipe", stderr: "pipe", }); From 40d869d6917bc4422f8f76d26bc04161a4bbac62 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:52:20 +0000 Subject: [PATCH 6/6] [autofix.ci] apply automated fixes --- test/napi/napi.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index fff620117dd0..3c3267ca84c5 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -565,7 +565,12 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => { // complete() is not invoked on the terminate path (see PR body), so the // addon's per-work calloc leaks by design. LSan stays off so the crash // assertion below is what decides pass/fail. - env: { ...bunEnv, ADDON: addon, WORKER_SRC: workerSrc, ASAN_OPTIONS: "detect_leaks=0:allow_user_segv_handler=1" }, + env: { + ...bunEnv, + ADDON: addon, + WORKER_SRC: workerSrc, + ASAN_OPTIONS: "detect_leaks=0:allow_user_segv_handler=1", + }, stdout: "pipe", stderr: "pipe", });