threading: don't touch WaitGroup after publishing count==0 in finish() - #34458
Conversation
WaitGroup::finish() decremented raw_count to 0 and then locked/unlocked the mutex and signalled the condvar. Once raw_count is 0 a concurrent wait() can observe it and return, after which the caller is free to drop the WaitGroup, so the trailing mutex.lock()/cond.signal() are writes to freed memory. Observed as a rare ASAN heap-use-after-free on a Bun Pool thread during back-to-back Bun.build() calls (bun-build-api.test.ts, debian 13 x64-asan). The failing pc maps to Mutex::FutexImpl::lock_slow's state.swap(CONTENDED), reached from WaitGroup::finish() acting on the source-map wait groups inside the heap-allocated BundleV2 on the error path of a sourcemap + link-time-error build. finish() now only publishes 0 while holding the mutex, so wait() cannot return (and the caller cannot free the WaitGroup) until after finish()'s unlock(). The fast path for count>1 stays lock-free via a CAS loop.
|
Updated 3:06 AM PT - Jul 17th, 2026
✅ @robobun, your commit 4d06bedc1c3d3fbf77bbf0dfdc08d847dbae3343 passed in 🧪 To try this PR locally: bunx bun-pr 34458That installs a local version of the PR into your bun-34458 --bun |
WalkthroughChangesThe PR revises Concurrency safety and regression coverage
🚥 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/bundler/bun-build-sourcemap-link-error-uaf.test.ts`:
- Around line 45-55: Update the catch block around Bun.build in the loop to
inspect the rejected error and assert that it contains the expected linker
missing-export diagnostic. Continue incrementing failed only after this
validation, so unrelated setup or runtime errors fail the test instead of being
counted as expected failures.
🪄 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: 67600614-f753-45b7-96fd-c7ed537150e6
📒 Files selected for processing (3)
src/install/PackageInstall.rssrc/threading/WaitGroup.rstest/bundler/bun-build-sourcemap-link-error-uaf.test.ts
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)
test/bundler/bun-build-sourcemap-link-error-uaf.test.ts (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a module-scope import in the child fixture.
Replace
require("path")withimport { join } from "node:path";. As per coding guidelines, dynamicimport/requireshould only be used when the test specifically exercises that behavior; otherwise use module-scope imports.🤖 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/bundler/bun-build-sourcemap-link-error-uaf.test.ts` around lines 40 - 42, Replace the module-scope require in the child fixture with a module-scope named import of join from node:path, while leaving the dir and iters argument handling unchanged.Source: Coding guidelines
🤖 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 `@test/bundler/bun-build-sourcemap-link-error-uaf.test.ts`:
- Around line 40-42: Replace the module-scope require in the child fixture with
a module-scope named import of join from node:path, while leaving the dir and
iters argument handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ca64c29-096b-4cf5-a47f-4f7b1a4d3978
📒 Files selected for processing (1)
test/bundler/bun-build-sourcemap-link-error-uaf.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deferring to a human — this rewrites WaitGroup::finish(), a core concurrency primitive shared by the bundler, thread pool, and installer, and the correctness argument (CAS fast path + mutex-held final decrement + signal-before-unlock so the only post-release access is an address-keyed futex_wake) is subtle enough to want maintainer eyes.
Beyond the inline nit, I traced the new ordering against wait() and Mutex::unlock() on all three platform impls: the 0-publish now happens under the mutex so wait() cannot return before finish()'s unlock(), and the only overlap left is unlock()'s trailing Futex::wake (Linux) / __ulock_wake (Darwin) / SRWLock release tail (Windows), none of which dereference user memory — same invariant wait()'s own trailing unlock() already relied on. The fast-path CAS keeps AcqRel on success, matching the old fetch_sub's release of task-completion writes. Concurrent add() racing the slow path just yields a spurious signal(), which wait()'s re-check loop tolerates.
Extended reasoning...
Checked the new finish() against wait() and the three Mutex platform backends (FutexImpl, os_unfair_lock, SRWLock). The fix moves the count-reaches-0 store under the mutex and signals before unlocking, so wait()'s reacquire strictly serializes after every self access in finish() except the futex-wake tail of unlock(), which is address-keyed. The PackageInstall.rs comment update is consistent — the race the old comment worked around is now closed at the primitive. The bug hunter separately examined and ruled out the catch-swallows-all-errors concern in the new test. This is a load-bearing primitive across several subsystems, so leaving the sign-off to a maintainer.
Also switch the child fixture's path import from require() to a module-scope import.
Self-review: the 300-iteration Bun.build() stress loop cannot fail on the unpatched build (29,000 iterations across three configurations didn't fire) so it is CI time with no regression-detection value, and a new standalone test file goes against the 'add to the existing test file' convention. Drop it and add a #[cfg(test)] unit test that drops a boxed WaitGroup immediately after wait() returns; it runs under 'cargo test -p bun_threading' and documents the invariant directly on the primitive.
|
Addressed self-review: dropped the standalone bundler stress test (it cannot fail on the unpatched build, so it was ~11s of CI time per ASAN lane for no regression detection, and the standalone file went against the test-organization convention). Replaced it with a The fail-before/pass-after gate bounces this change because the race is not deterministically reproducible without src/ instrumentation (see the Testing section of the PR body for the stress-probe evidence). The diagnosis rests on the pre-existing in-tree comment in CI failures on builds 74461 and 74478 are all flaky or pre-existing on lanes unrelated to this diff (Windows dev-server deinit and es-module-lexer timeouts, Windows/Ubuntu EPIPE in repl-close/update-interactive/proxy-stress, Ubuntu node-net mimalloc page-count flake, napi.test.ts, worker-message-port-transfer-terminate JSC assertion). None touch Ready for maintainer review. |
There was a problem hiding this comment.
I didn't find any bugs, but this rewrites the completion path of a core threading primitive (WaitGroup::finish) with subtle memory-ordering and drop-after-unlock() reasoning, so it warrants a maintainer's eyes.
What was reviewed:
finish()now only publishesraw_count == 0under the mutex, sowait()(which checks under the same mutex) cannot return before the finisher'sunlock(); the fast-path CAS never writes 0.- Checked
Condition::signalandMutex::unlock(Futex/Darwin/Windows) — the finisher's lastselfaccess is the release store inunlock(), with only a kernelfutex_wakepossibly following, matching the PR's stated guarantee. - Considered concurrent
add()between the CAS loop and the lockedfetch_sub— produces a benign spurious signal thatwait()'s re-check absorbs. PackageInstall.rschange is comment-only; behavior unchanged. The earlier nit about the bundler stress test is moot now that the file was dropped.
Extended reasoning...
Overview
Two files: src/threading/WaitGroup.rs (rewrites finish() from unconditional fetch_sub + trailing lock/unlock/signal to a CAS-while->1 fast path plus a mutex-held fetch_sub/signal/unlock slow path, and adds a #[cfg(test)] drop-after-wait() unit test) and src/install/PackageInstall.rs (shortens a now-stale comment; no code change). The bundler stress test that earlier revisions added has been removed, so my prior inline nit about its timeout no longer applies.
Security risks
None. This is an internal synchronization primitive with no user-controlled input surface. The change tightens a lifetime invariant; it does not relax any check.
Level of scrutiny
High. WaitGroup is used by the bundler's source-map fan-out and the package-install hardlink queue, and finish() is the hot path for every pooled task completion. The fix's soundness rests on: (1) the CAS loop never publishing 0; (2) wait() only observing 0 while holding the mutex, which the finisher held when it published 0; (3) Mutex::unlock() being the finisher's last self access, with any trailing OS wake taking the address as a kernel key rather than dereferencing user memory. I traced (3) through src/threading/Mutex.rs (FutexImpl::unlock does state.swap(UNLOCKED, Release) then Futex::wake) and Condition.rs (signal() runs entirely before unlock()), and it holds — but this is exactly the class of reasoning REVIEW.md flags as the most-blocked category, and a maintainer should confirm it.
Other factors
The PR body is candid that the race is not fail-before reproducible (8k/15k/6k stress iterations did not trip it on the unpatched build); the diagnosis rests on addr2line of a one-off CI ASAN pc landing in FutexImpl::lock_slow plus the pre-existing in-tree comment describing this exact window. The added cargo test -p bun_threading unit test documents the invariant directly on the primitive but is likewise timing-dependent. All prior review-thread items (CodeRabbit's catch-block assertion, my timeout nit) are resolved or moot. Given the primitive's blast radius and the acknowledged lack of a deterministic regression test, deferring to a human is the right call.
…t the release (#38330) ### Problem - `WaitGroup::wait()` returning is what lets the owner free the group, so once the last `finish()` has published the count and released the mutex, the finishing thread must not touch the group at all. #34458 moved the last real access in front of that release. What is left is that `finish(&self)`, `Mutex::unlock(&self)`, the per-OS unlock impls, and the contended-path `Futex::wake(&self.state)` all still hold references into the group through and after the release, and a reference argument asserts the memory for the whole call. - Miri rejects the waiter's free for exactly that reason. `bun run rust:miri -p bun_threading` (the crate is not in the Miri set yet) fails the crate's own test, `wait_group::tests::wait_returning_means_finish_is_done_with_self`, under Tree Borrows with `Undefined Behavior: deallocation through <tag> at alloc[0xc] is forbidden ... transitioned due to a protector release --> src/threading/Mutex.rs:193` (end of `DebugImpl::unlock`; offset `0xc` is padding inside the mutex, so this hits even though every field is an atomic), and under the default Stacked Borrows with `... would remove [SharedReadOnly ...] which is strongly protected`. Every seed I tried (12) fails within 2..200 iterations. - It is not only padding: when the waiter re-locked while the finisher still held the mutex, `FutexImpl::unlock` (src/threading/Mutex.rs:397 on main) forms `&self.state` for the wake after the releasing swap, i.e. possibly after the waiter has freed the group. In an instrumented Miri run that happened in 6 of 1500 iterations. On a real kernel the wake is harmless (a private `FUTEX_WAKE` only uses the address as a key), but the reference is still formed on freed memory. - In-tree callers with that lifetime: `LinkerContext`'s two source-map groups. `generate_chunks_in_parallel` frees the task slab on the line after `wait()` (src/bundler/linker_context/generateChunksInParallel.rs:78), and the `Bun.build` error path waits on both groups and tears down the whole `BundleV2` right after (src/runtime/api/js_bundle_completion_task.rs:1267). That is the path #34458's ASAN report came from. `ThreadPool`'s group is joined before the pool drops and the Windows install queue's group is a `static`, so those two are fine with `&self`. ### Fix - `WaitGroup::finish_raw(this: *const Self)` does the work through raw pointers, fast path included (a fast-path CAS can be the second-to-last decrement, with another finisher letting the waiter free the group while this frame is still live). Its last access to the group is the store that releases the mutex. - `Mutex::unlock_raw(this: *const Self)` and raw-pointer `unlock_raw` impls for the futex, Darwin and Windows backends, so no frame between `finish_raw` and the releasing store holds a reference. `unlock(&self)` delegates to it; `Bun__unlock` calls the impl directly; the `os_unfair_lock_unlock` extern takes the address (the Windows one already did). - `Futex::wake_raw(*const AtomicU32)` for the unlock tail; `wake(&AtomicU32)` delegates to it, so the other callers are unchanged. It is a safe fn because every backend's wake side only keys on the address and never reads the word (Linux `get_futex_key` for private futexes, `__ulock_wake`, `RtlWakeAddress*`, `_umtx_op` private wake, `memory.atomic.notify`); the worst a freed or reused address can produce is a spurious wakeup, which every wait loop already tolerates. - `finish(&self)` stays for groups kept alive by something other than `wait()` and delegates to `finish_raw`; its doc says when it is and is not allowed. The two `LinkerContext` finishers use `finish_raw` through `ParentRef::as_const_ptr`, so the finish is the last statement in the task that touches the context. - Why this is the right shape: the memory-level ordering from #34458 is unchanged; this only changes which pointers are live across the release, which is the thing both aliasing models (and LLVM's `dereferenceable` on reference arguments) reason about. It is the same shape as `UnboundedQueue::push_raw` in #37883 and the tree's `this: *mut Self` convention for functions that end in a free (test/internal/source-lints/self-receiver-reclaim.test.ts). - Linux `Futex` wake no longer panics on `EFAULT` (the FreeBSD backend already tolerated it). A real kernel only returns it for an address outside user space, which no caller can produce (every caller just did an atomic op on the same word), while Miri returns it for a word that has since been freed, which is now a documented-legal input; with the panic kept, the fixed test fails under Miri about once per 250 iterations (the 6/1500 above). The `futex_3arg` SAFETY comment in `bun_sys` is corrected to match (a WAKE only uses `uaddr` as a key). - Verified with: - The crate's own test (`wait_group::tests::wait_returning_means_finish_raw_is_done_with_the_group`), which now finishes through `finish_raw`. `bun_threading` is added to `MIRI_CRATES` and `src/threading/**` to the Miri workflow's paths, so the existing `cargo miri test` CI job is the automated check: it fails on main's `src/threading` (the diagnostic above) and passes here. Under `cfg(miri)` the test runs 500 iterations (about 15s; the unfixed shape fails within 200 on every seed tried), natively still 10,000. Passes on 12 seeds plus a 10,000-iteration run on the default seed, under both Tree Borrows and Stacked Borrows; the full `bun run rust:miri` set passes (`bun_threading` takes 15s, `bun_paths` takes 75s for comparison). - `cargo check -p bun_threading --tests` on linux-gnu, linux-musl, android, both darwin and both windows-msvc triples and freebsd, plus `--release` (the `ReleaseImpl`-direct path) on linux, darwin and windows; `cargo check --workspace` on the host; `cargo clippy --no-deps` on `bun_threading` and `bun_bundler`; the Windows-target clippy finding set is identical to main's. - `bun bd test test/bundler/bun-build-api.test.ts` (52 pass) and a 20-round loop of a 200-module `sourcemap: "external"` build plus a failing sourcemap build under the ASAN debug build, which exercises both `LinkerContext` call sites and the error-path teardown. - Overlap with open PRs: #37883 adds `bun_threading` to the Miri set with this test `#[ignore]`d under Miri; this PR makes it pass, so whichever lands second drops the ignore (the `MIRI_CRATES` and workflow lines are identical). #36481 introduces stack-scoped groups whose `BatchDone` drop calls `(*ptr).finish()` relying on this property; with this change that should be `WaitGroup::finish_raw(ptr)`, noted there. - Deliberately not in this PR: the same shape exists with other primitives in two places outside `bun_threading`, `SingleHTTPChannel::write_item(&self)` in src/http/AsyncHTTP.rs (`send_sync` frees the channel right after `read_item` returns) and `process_http_callback(&mut self)` in src/runtime/webcore/s3/download_stream.rs (`on_response` frees the task once that unlock lands). Both need a raw-unlock path for the guard types rather than `WaitGroup` changes, so they are tracked separately. `ResetEvent::set(&self)` has the same tail but its only user is a process-lifetime `BundleThread`, and the `Condvar` notifiers I looked at (`VmHandle`, `HTTPThread` shutdown) hold an `Arc` or a `static`. ### Background - Protectors: in Rust's aliasing models (Stacked Borrows, and Tree Borrows, which `rust:miri` uses) a reference passed as a function argument is "protected" for the duration of the call: the callee may assume the memory stays valid and unchanged by others until it returns, and freeing memory that a protected reference covers is undefined behavior even if the callee never touches it again. This is what lets rustc mark reference arguments `dereferenceable` for LLVM. A raw pointer argument carries no such assertion, which is why a function whose job ends by letting another thread free the object takes `*const Self`. - Padding: a `&Mutex` covers the struct's padding bytes too, and padding is not interior-mutable, so a struct made only of atomics still gets the strict treatment for those bytes. That is what the `0xc` in the diagnostic is. - Futex wake is address-keyed: `wait` sleeps on an address after comparing the word; `wake` looks the address up in the kernel's (or runtime's) waiter table without reading user memory. This is the property every futex-based mutex relies on so that the thread that acquires the lock next may free it; it is also why a wake on a freed address is harmless and why `wake_raw` needs no `unsafe`. - `WaitGroup::finish` publishes the final decrement under the group's mutex (since #34458), so `wait()`, which checks the count under the same mutex, cannot return before the finisher's unlock; the releasing store inside that unlock is therefore the exact point after which the group may be gone. <details> <summary>Miri diagnostic on main, and the iteration counts</summary> ``` $ bun run rust:miri -p bun_threading test wait_group::tests::wait_returning_means_finish_is_done_with_self ... error: Undefined Behavior: deallocation through <620185> at alloc217837[0xc] is forbidden --> library/alloc/src/boxed.rs:2002:17 = help: the accessed tag <620185> has state Reserved (conflicted) which forbids this deallocation (acting as a child write access) help: the accessed tag <620185> was created here, in the initial state Reserved --> src/threading/WaitGroup.rs:115:17 drop(Box::from_raw(wg)); help: the accessed tag <620185> later transitioned to Reserved (conflicted) due to a protector release (acting as a foreign read access) on every location previously accessed by this tag --> src/threading/Mutex.rs:193:6 (end of DebugImpl::unlock) ``` Depending on the interleaving the same test also fails as `deallocation ... is forbidden ... the accessed tag is foreign to the protected tag <..> (currently Frozen) ... protected tag was created here: Mutex.rs:66 pub fn unlock(&self)`, and under Stacked Borrows (`MIRIFLAGS=""`) as `not granting access to tag <..> because that would remove [SharedReadOnly for <..>] which is strongly protected`. Iteration at which the unfixed shape fails under Miri, by `-Zmiri-seed`: `0: 2, 1: 199, 2: 75, 3: 16, 4: 75, 5: 10, 6: 10, 7: 113, 8: 113, 9: 71, 10: 36, 11: 51`. Instrumented run of the fixed code, 1500 iterations, default seed: 2051 contended unlocks, 6 of whose wakes ran after the waiter had already freed the group (Miri returned `EFAULT`), 0 aliasing reports. </details>
2 participants
Bug
WaitGroup::finish()didOnce
raw_countreaches 0, a concurrentwait()can observe it, return, and the caller can drop theWaitGroup. The trailingmutex.lock()/cond.signal()are then writes to freed memory. The lock/unlock dance correctly prevents a missed-signal deadlock but assumes theWaitGroupoutlives it, which it does not when the waiter is free to drop it oncewait()returns.src/install/PackageInstall.rsalready carried a comment working around exactly this window.Where it fires
Seen once in CI build 74363 (debian 13 x64-asan,
test/bundler/bun-build-api.test.ts) asThe process was killed before ASAN printed the stack. Rebuilding
release-asanat the adjacent commit and mapping the pc via two known LSan anchors in the same binary (warm::{closure#0}at0x9351068andThread::runat0x93584af) lands it on:which is reachable on a
Bun Poolthread only viaWaitGroup::finish() -> Mutex::lock().The failing test is "sourcemap + build error crash case":
sourcemap: "external"plus a "no matching export" link error.LinkerContext::link()schedules source-map tasks (compute_data_for_source_map) and thenscan_imports_and_exportsreturnsErr, sorun_from_js_in_new_threadearly-returns beforegenerateChunksInParallelwould have joined them. The error arm ininit_and_runthen doeswhile a pool thread is still inside
finish()for one of those wait groups. Thelock_slowpc meanstry_lockhad failed (the waiter held the mutex), so the pool thread was futex-parked; the bundler'sunlockwoke it but it did not get scheduled beforeBox<BundleV2>was dropped.Fix
finish()now only publishesraw_count == 0while holding the mutex, sowait()(which checks the count under the same mutex) cannot return untilfinish()'sunlock(), which is the lastselfaccess:The
count > 1case stays lock-free (one relaxed load + one CAS vs the previous singlefetch_sub). This matches what Zig std'sWaitGroupand Go'ssync.WaitGroupguarantee: after the waiter returns, the only thing a finisher may still do is afutex_wake, which takes the address as a kernel hash key and never touches user memory.The now-stale "must not re-assign" comment in
PackageInstall.rsis shortened accordingly; behaviour there is unchanged.Testing
Not fail-before provable via
bun bd test. The window is the gap betweenfetch_subandmutex.lock()(a handful of instructions, or a futex wake-to-run latency in the contended case) vs the bundler'swait()return throughdeinit_without_freeing_arena()to theBox<BundleV2>drop. Stress probes against the unpatched build did not fire:bun bd(debug-asan)release-asanbuildrelease-asanwith 48 CPU-spinning child processes to oversubscribe the schedulerA 300-iteration stress test was tried and dropped after review: it adds ~11s to every ASAN lane and cannot fail on the unpatched build, so it has no regression-detection value.
The added
#[cfg(test)]unit test insrc/threading/WaitGroup.rsdrops a boxedWaitGroupimmediately afterwait()returns in a tight 10k-iteration loop; it runs undercargo test -p bun_threadingand documents the invariant directly on the primitive. The diagnosis above (existing in-tree comment acknowledging the race + addr2line confirmation on the reported pc) is the basis for the fix.