Repository navigation
Conversation
Replace the raw void* impl_ pointer in Worker.cpp with a ref-counted WebWorkerLifecycleHandle that sits between the C++ Worker (parent thread) and the Zig WebWorker (worker thread). The handle contains an atomic nullable pointer to the WebWorker. Termination atomically swaps it to null, so only one caller can trigger termination — preventing double-free and use-after-free when terminate() is called concurrently or after the worker has already exited. Both the C++ Worker and the Zig WebWorker hold a ref on the handle. The handle is only freed when both refs are released.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReworked Worker lifetime and termination: replaced C FFI notify with handle-oriented Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/webcore/Worker.cpp (1)
193-220:⚠️ Potential issue | 🟠 MajorMove the Zig-owned
Workerref below the null-handle check.Line 212 increments the
Workerrefcount before Line 216 verifies thatWebWorkerLifecycleHandle__createWebWorker()succeeded. When creation fails, the localRefonly drops back to 1 on return, so the failedWorkerstays leaked and never leavesallWorkers().💡 Proposed fix
- // now referenced by Zig - worker->ref(); - preloadModuleStrings.clear(); if (!lifecycleHandle) { return Exception { TypeError, errorMessage.toWTFString(BunString::ZeroCopy) }; } + // now referenced by Zig + worker->ref(); worker->lifecycleHandle_ = lifecycleHandle;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/webcore/Worker.cpp` around lines 193 - 220, The code currently calls worker->ref() before verifying the result of WebWorkerLifecycleHandle__createWebWorker(), which leaks Worker objects when lifecycleHandle is null; change the sequence so you only call worker->ref() after confirming lifecycleHandle is non-null and after assigning worker->lifecycleHandle_ (i.e., move the worker->ref() line to just after the if (!lifecycleHandle) check and the assignment), ensuring preloadModuleStrings.clear() remains where it is and that the function returns the Exception without touching refcount when creation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/web_worker.zig`:
- Around line 69-79: The code frees the Zig WebWorker while the C++
WebWorkerLifecycleHandle::worker pointer can still reference it; before any path
that calls worker.deref() or otherwise destroys the WebWorker, atomically detach
the handle by storing null into handle.worker (e.g., handle.worker.store(null,
.release)) so the C++ side cannot observe a pointer to freed memory. Update
WebWorker__updatePtr and all destruction paths that call worker.deref() (the
same codepaths that free the WebWorker) to perform an atomic swap/store-to-null
on handle.worker prior to calling worker.deref() or freeing the worker, and
ensure memory ordering (.release/.acquire) is used so other threads see the
detach.
In `@test/js/web/workers/worker-lifecycle-handle.test.ts`:
- Around line 29-50: Add a sibling test case in worker-lifecycle-handle.test.ts
that covers the natural exit path (worker finishes normally) rather than forcing
process.exit(), because the current test only exercises
WebWorker.exit()/requestTermination() and misses the exitAndDeinit() path;
specifically, spawn a worker that runs to completion (e.g., returns or posts a
message and exits) and await its normal exit before calling Bun.gc(true), then
assert stdout contains "ok" and exitCode is 0 so the test verifies handle.worker
is cleared after a natural exit as well.
---
Outside diff comments:
In `@src/bun.js/bindings/webcore/Worker.cpp`:
- Around line 193-220: The code currently calls worker->ref() before verifying
the result of WebWorkerLifecycleHandle__createWebWorker(), which leaks Worker
objects when lifecycleHandle is null; change the sequence so you only call
worker->ref() after confirming lifecycleHandle is non-null and after assigning
worker->lifecycleHandle_ (i.e., move the worker->ref() line to just after the if
(!lifecycleHandle) check and the assignment), ensuring
preloadModuleStrings.clear() remains where it is and that the function returns
the Exception without touching refcount when creation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d03ecd9-a157-439c-b425-25f304f35717
📒 Files selected for processing (4)
src/bun.js/bindings/webcore/Worker.cppsrc/bun.js/bindings/webcore/Worker.hsrc/bun.js/web_worker.zigtest/js/web/workers/worker-lifecycle-handle.test.ts
Address coderabbit review: - Atomically swap handle.worker to null in exitAndDeinit() and the thread-spawn failure path, preventing the C++ side from observing a dangling pointer after the WebWorker is freed. - Move worker->ref() after the lifecycleHandle null check to avoid leaking the Worker when creation fails. - Add natural-exit test case (worker event loop drains without process.exit) to cover the exitAndDeinit path.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- destroy(): swap handle.worker to null before freeing the WebWorker struct, preventing dangling pointer on natural exit paths where requestTermination() was never called. - notifyNeedTermination(): use parent_poll_ref.unrefConcurrently() instead of setRefInternal(false) since this may be called from either thread via requestTermination().
…ctly on WebWorker The intermediate handle struct with its own refcount and atomic pointer is unnecessary. ThreadSafeRefCount on WebWorker directly achieves the same UAF protection: C++ holds one ref (released in ~Worker), worker thread holds one ref (released in exitAndDeinit). The struct survives until both release. The 'first terminate wins' semantics is a simple atomic bool swap (the requested_terminate that was removed), not an atomic pointer swap. - Worker.h: back to void* impl_ (no net change from main) - Worker.cpp: terminate() calls requestTermination but keeps the ref; only ~Worker releases. setKeepAlive just derefs impl_. - web_worker.zig: requested_terminate atomic bool, initExactRefs(2), parent_poll_ref.unref in exitAndDeinit (not destroy — C++ may hold its ref until GC and we don't want to block parent exit on that). Net +45 lines vs main (was +229).
alii
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/bun.js/web_worker.zig (1)
83-86:⚠️ Potential issue | 🔴 CriticalCall
requestTermination()on spawn failure.This raw
store(true)bypassesnotifyNeedTermination(). Whenstd.Thread.spawn()fails, the later C++WebWorker__requestTermination()sees the flag already set and never dropsparent_poll_ref, so failed worker creation can keep the parent loop alive.🔧 Proposed fix
) catch { - // Spawn failed — release the worker thread's ref. C++ still - // holds its ref and will release via WebWorker__release. - worker.requested_terminate.store(true, .release); + // Spawn failed — run the normal termination path so the parent + // keep-alive is unwound even though the thread never started. + worker.requestTermination(); worker.deref(); return false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/web_worker.zig` around lines 83 - 86, Replace the raw flag write on Worker.requested_terminate with the official termination path: call worker.requestTermination() (or the helper that invokes notifyNeedTermination()) instead of worker.requested_terminate.store(true, .release) in the spawn-failure branch so that notifyNeedTermination()/WebWorker__requestTermination() runs and parent_poll_ref is properly dropped when std.Thread.spawn() fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/webcore/Worker.cpp`:
- Around line 218-220: The extra worker->ref() (currently called right after
Worker::create()) is leaked if impl.updatePtr() fails before the Zig thread
starts; either move the worker->ref() to after impl.updatePtr() succeeds (so Zig
owns the ref only when the thread/startup is confirmed) or add an explicit
unwind that calls worker->unref() (and clears worker->impl_) on the false-return
path from impl.updatePtr() in the startup sequence; adjust the block around
Worker::create(), worker->impl_ = impl, and the impl.updatePtr() call, ensuring
symmetry with WebWorker__dispatchExit() so the Zig-owned ref is always balanced.
In `@src/bun.js/web_worker.zig`:
- Around line 383-397: deinit() currently frees unresolved_specifier and
preloads but never frees the heap buffer allocated for the worker name
(allocated via allocPrintSentinel), causing a leak for named workers; update
deinit() to check for and free the name buffer (e.g., call
bun.default_allocator.free(this.name) or
bun.default_allocator.free(this.worker_name) if non-null) before returning so
the named-worker allocation is released when destroy() calls deinit().
---
Duplicate comments:
In `@src/bun.js/web_worker.zig`:
- Around line 83-86: Replace the raw flag write on Worker.requested_terminate
with the official termination path: call worker.requestTermination() (or the
helper that invokes notifyNeedTermination()) instead of
worker.requested_terminate.store(true, .release) in the spawn-failure branch so
that notifyNeedTermination()/WebWorker__requestTermination() runs and
parent_poll_ref is properly dropped when std.Thread.spawn() fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ca14ed4-46c6-4eb6-a5e2-875227f1808d
📒 Files selected for processing (2)
src/bun.js/bindings/webcore/Worker.cppsrc/bun.js/web_worker.zig
- Use requestTermination() instead of raw store on spawn failure so parent_poll_ref is properly unwound. - Free the worker name buffer in deinit() for named workers. - Deref and null impl_ in updatePtr() failure path to balance the ref() from create() when the thread never starts.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/webcore/Worker.cpp`:
- Around line 145-148: When WebWorker__updatePtr fails you currently null impl_
then call deref(), leaking the Zig WebWorker because the C++ ref is never
released; to fix, call WebWorker__release(impl_) while impl_ is still valid (and
only if impl_ != nullptr) before setting impl_ = nullptr and before calling
deref(), so the C++ ref count is balanced (ensure this change is applied in the
failure branch where WebWorker__updatePtr returns false, referencing impl_,
WebWorker__updatePtr, WebWorker__release, deref(), and ~Worker).
In `@src/bun.js/web_worker.zig`:
- Around line 607-617: notifyNeedTermination() and exitAndDeinit() both call
parent_poll_ref.unrefConcurrently(this.parent), which can result in a
double-unref; add a concise comment next to the unrefConcurrently call(s) (or at
the parent_poll_ref field) stating that double-unref is expected and safe
because Bun.Async.KeepAlive tracks internal state and permits
idempotent/unordered unrefs, and mention that the requested_terminate flag
ensures no further unrefs are triggered after termination; reference
notifyNeedTermination(), exitAndDeinit(), and
parent_poll_ref.unrefConcurrently() when adding the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eec62163-5521-4f23-94a9-c52637622ebb
📒 Files selected for processing (2)
src/bun.js/bindings/webcore/Worker.cppsrc/bun.js/web_worker.zig
… path - Move unrefConcurrently in exitAndDeinit to after WebWorker__dispatchExit so the parent doesn't exit before seeing the exit event. - Remove unref from notifyNeedTermination — exitAndDeinit is the sole place that unrefs, eliminating the double-unref race on the non-atomic KeepAlive.status field. - Add explicit unref in spawn failure path since exitAndDeinit never runs.
There was a problem hiding this comment.
The bugs I flagged in previous rounds have all been addressed — the concurrency model now looks correct (single unref site in exitAndDeinit after dispatchExit, idempotent requestTermination via atomic swap). However, this is complex multi-threaded worker lifecycle code with CI build failures across all platforms, so it warrants human sign-off.
Extended reasoning...
Overview
This PR adds ThreadSafeRefCount to WebWorker to prevent use-after-free when the C++ Worker object outlives the Zig WebWorker struct. It modifies Worker.cpp (C++ bindings), web_worker.zig (Zig implementation), and adds 4 regression tests. The core change is initializing the ref count to 2 (one for the worker thread, one for C++), with exitAndDeinit releasing the worker ref and ~Worker releasing the C++ ref via WebWorker__release.
Security risks
The PR is specifically fixing a memory safety bug (use-after-free). The ref counting and atomic operations look correct in the current iteration. No new injection, auth, or data exposure risks.
Level of scrutiny
This deserves careful human review because: (1) it manages object lifetimes across two threads with atomic operations — subtle ordering bugs can cause UAF, double-free, or event loss; (2) it touches the Worker lifecycle which is a critical code path; (3) CI is failing across all platforms (build failures on musl, aarch64, x64, macOS), though it is unclear if the latest commits resolve those; (4) the PR has the claude label indicating AI authorship.
Other factors
The PR went through multiple rounds of review (CodeRabbit + my previous reviews) and has iterated on several real bugs: dangling handle pointers, thread-unsafe unref, spawn failure ref leaks, name buffer leaks, and ordering regressions. All identified issues appear resolved in the current code. The 4 new tests cover double-terminate, terminate+GC, natural-exit+GC, and immediate-terminate scenarios. The test coverage is good but does not exercise the spawn-failure or setRef race paths.
notifyNeedTermination must unref parent_poll_ref so the parent event loop doesn't hang waiting for a terminated worker. Use setRefInternal (same-thread safe) in notifyNeedTermination, and unrefConcurrently (thread-safe) in exitAndDeinit after dispatchExit. The KeepAlive status check makes the double-unref safe — the second sees .inactive and is a no-op.
destroy() only runs when refcount=0, which may be after vm.deinit() if the C++ Worker hasn't been GC'd. The struct itself can wait (it's ~100 bytes), but the name/source-specifier/preloads can be large for eval workers — free them synchronously at exit time like main did. releaseResources() is idempotent so destroy() calling it for early- failure paths (thread spawn failure) is safe.
free() on empty string literals or zero-length slices is UB. Guard unresolved_specifier and preloads frees with len > 0 checks, matching the existing name guard.
The concurrent Promise.all([terminate(), terminate()]) test was timing-dependent and failing on slower CI runners. The remaining 3 tests cover the important lifecycle scenarios.
Call terminate(), await it, then fire terminate() again (fire-and-forget). The second call hits threadId === -1 and returns immediately. This is deterministic — no timing dependency.
Tests the racy case where terminate() is called twice without awaiting. Uses the exit event instead of awaiting promises to avoid the pre-existing JS-side bug where the second terminate() promise hangs.
|
Added a separate |
notifyNeedTermination was unreffing the parent's event loop keep-alive immediately on terminate(). This let the parent drain its event loop before the worker's 'exit' event was delivered — the dispatchExit posted task needs the parent loop alive to process. Node.js semantics: terminate() returns a Promise that resolves with the exit code. This requires the 'exit' event to actually fire, which requires the parent loop to stay alive. The unref now happens only in exitAndDeinit, after dispatchExit posts the close-event task. Also simplified the double-terminate test to not depend on the terminate() Promise resolution (which is a separate JS-side issue) — just verify two terminate() calls don't crash and 'exit' fires once.
Add direct parent_poll_ref.unrefConcurrently before requestTermination in the spawn failure handler, so the parent keep-alive is guaranteed to be released even if notifyNeedTermination's setRefInternal path has edge cases.
This reverts commit 42ed136.
|
Eventually we will want this to work but this introduces crashes right now. |
2 participants
What does this PR do?
Adds
ThreadSafeRefCountto the ZigWebWorkerstruct so both the C++Worker(parent thread) and the worker thread hold a ref. The struct survives until both release, preventing UAF when the C++ side callsterminate()orsetKeepAlive()after the worker thread frees itself.The problem
Worker.cppheld a rawvoid* impl_to the ZigWebWorkerstruct. The worker thread frees this inexitAndDeinit(), but the C++Workerlives on the parent thread and can still deref — viaterminate(),setKeepAlive(), or the destructor. Use-after-free.Calling
terminate()twice also callednotifyNeedTerminationon the pointer twice with no synchronization.The fix
ThreadSafeRefCountonWebWorker,initExactRefs(2)at creation — one ref for the worker thread (released inexitAndDeinit), one for C++ (released in~WorkerviaWebWorker__release)requested_terminate: atomic boolfor idempotent termination —requestTermination()atomically swaps to true, only the first caller triggersnotifyNeedTerminationterminate()keeps the ref, only~Workerreleases — sosetKeepAlive()is safe between terminate() and destructionparent_poll_ref.unrefmoved fromdestroytoexitAndDeinit— C++ may hold its ref until GC; do not block parent exit on thatThis is a reimplementation of #19940 approach (closed due to scope), without the intermediate
LifecycleHandlewrapper.Verification
Four regression tests: double-terminate, terminate+GC, natural-exit+GC, immediate-terminate.
Verification (robobun): CI build #40327 on commit 9c30d51 is still running (Lint JS passed, buildkite building). Diff is clean — no TODO/FIXME/HACK, no unrelated changes. Refcount logic traced through all paths (creation, spawn failure, normal exit, terminate, double-terminate, destructor) — all balanced correctly. Four regression tests exercise the exact crash scenarios (double terminate, terminate+GC, natural exit+GC, immediate terminate) via subprocess spawning so segfaults surface as non-zero exit codes. CodeRabbit and claude review issues (spawn failure leak, name leak, worker->ref() ordering, natural exit test coverage) all addressed in subsequent commits. Concurrent-terminate feedback addressed — test fires two terminate() calls without awaiting.\n\n---\n\nVerification (robobun, iteration 15): CI build #40388 on commit d254d52 — Format pass, Lint JS pass, Buildkite compiling. Diff clean: no TODO/FIXME/HACK. Traced ref-counting through all 5 lifecycle paths (normal exit, spawn failure, double terminate, terminate+destructor, unhandled rejection) — all balanced at 2 to 0. Tests spawn subprocesses so segfaults surface as non-zero exit codes; confirmed these exercise the exact UAF scenario. CodeRabbit Major/Critical issues (ref ordering, spawn failure path) both addressed in current code. Claude reviewer confirmed concurrency model is correct.