fs.promises.cp: defer AsyncCpTask destruction until all subtasks finish - #30162
Conversation
When a SingleTask copy failed during a recursive fs.promises.cp, the error path called finishConcurrently() which immediately enqueued runFromJSThread -> deinit() -> bun.destroy(this). Other SingleTasks still running on the thread pool then dereferenced the freed parent (args, subtask_count, onCopy). The same UAF existed after _cpAsyncDirectory returned an error while subtasks it had already spawned were still in flight. finishConcurrently() now only records the result (first caller wins). A new onSubtaskDone() decrements subtask_count with acq_rel ordering and only the last caller enqueues runFromJSThread. Every code path -- the main directory-scan task via defer, and every SingleTask on both success and error -- drops exactly one reference, so the parent is never destroyed while a subtask still holds a pointer to it.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Review rate limit: 0/5 reviews remaining, refill in 7 minutes and 41 seconds. Comment |
|
Updated 1:42 AM PT - May 3rd, 2026
✅ @robobun, your commit 1c4339de7d454a99bb88053c9ce64c6b6b2c57ec passed in 🧪 To try this PR locally: bunx bun-pr 30162That installs a local version of the PR into your bun-30162 --bun |
|
Independently arrived at the same fix for this (reported via Slack as a UAF of Minor diff in approach: I kept The test in my branch uses |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
32 files x 20 iterations still triggers the ASAN use-after-poison reliably (10/10 on the unpatched build) and completes in ~2-3s under the default timeout with the fix applied.
…sh (oven-sh#30162) ## What Fixes a use-after-free in `fs.promises.cp(src, dest, { recursive: true })` when one file copy fails while sibling `SingleTask`s are still running on the thread pool. ## Repro Recursive `fs.promises.cp` of a directory where copying one file fails (e.g. its destination path is already a directory → `EISDIR`) while ~100 sibling file copies are in flight. Under ASAN: ``` ==4475==ERROR: AddressSanitizer: use-after-poison on address 0x79676fca08b0 WRITE of size 8 at 0x79676fca08b0 thread T22 (Bun Pool 11) #0 atomic.Value(usize).fetchSub oven-sh#1 NewAsyncCpTask(false).SingleTask.workPoolCallback src/bun.js/node/node_fs.zig:528 ``` ## Cause `finishConcurrently()` used the `has_result` cmpxchg only to ensure the **result** was set once, then immediately enqueued `runFromJSThread` → `deinit()` → `bun.destroy(this)`. It did not wait for `subtask_count` to reach zero, so: - A `SingleTask` that errored called `finishConcurrently(err)` and returned **without** decrementing `subtask_count`. The JS thread then freed the parent while other `SingleTask`s were still dereferencing `cp_task->args` / `cp_task->subtask_count`. - `cpAsync` decremented `subtask_count` after `_cpAsyncDirectory` returned an error, by which time `runFromJSThread` could already have freed `this`. ## Fix - `finishConcurrently(result)` now only records the result (first caller wins). - New `onSubtaskDone()` decrements `subtask_count` with `.acq_rel` ordering; only the caller that drops it to zero enqueues `runFromJSThread`. If no one recorded a result, it defaults to `.success`. - `cpAsync` drops its initial reference via `defer this.onSubtaskDone()`, covering every early return (Windows, non-directory, EISDIR, and the recursive path). - `SingleTask.workPoolCallback` always ends with `this.deinit(); parent.onSubtaskDone();` on both success and error paths. This matches the pattern already used by `AsyncReaddirRecursiveTask`. ## Verification New test in `test/js/node/fs/cp.test.ts` creates a source dir with 128 files plus one whose destination is a pre-existing directory, and runs `fs.promises.cp` 50× in a subprocess. - **Without fix** (`git stash -- src/ && bun bd test`): subprocess aborts with the ASAN `use-after-poison` shown above → test fails. - **With fix** (`bun bd test`): subprocess rejects with `EISDIR` every iteration, exits 0 → test passes. - Full `cp.test.ts` suite: 38 pass, 3 skip (Windows-only), 0 fail. - `zig:check-all` passes on all targets. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
What
Fixes a use-after-free in
fs.promises.cp(src, dest, { recursive: true })when one file copy fails while siblingSingleTasks are still running on the thread pool.Repro
Recursive
fs.promises.cpof a directory where copying one file fails (e.g. its destination path is already a directory →EISDIR) while ~100 sibling file copies are in flight. Under ASAN:Cause
finishConcurrently()used thehas_resultcmpxchg only to ensure the result was set once, then immediately enqueuedrunFromJSThread→deinit()→bun.destroy(this). It did not wait forsubtask_countto reach zero, so:SingleTaskthat errored calledfinishConcurrently(err)and returned without decrementingsubtask_count. The JS thread then freed the parent while otherSingleTasks were still dereferencingcp_task->args/cp_task->subtask_count.cpAsyncdecrementedsubtask_countafter_cpAsyncDirectoryreturned an error, by which timerunFromJSThreadcould already have freedthis.Fix
finishConcurrently(result)now only records the result (first caller wins).onSubtaskDone()decrementssubtask_countwith.acq_relordering; only the caller that drops it to zero enqueuesrunFromJSThread. If no one recorded a result, it defaults to.success.cpAsyncdrops its initial reference viadefer this.onSubtaskDone(), covering every early return (Windows, non-directory, EISDIR, and the recursive path).SingleTask.workPoolCallbackalways ends withthis.deinit(); parent.onSubtaskDone();on both success and error paths.This matches the pattern already used by
AsyncReaddirRecursiveTask.Verification
New test in
test/js/node/fs/cp.test.tscreates a source dir with 128 files plus one whose destination is a pre-existing directory, and runsfs.promises.cp50× in a subprocess.git stash -- src/ && bun bd test): subprocess aborts with the ASANuse-after-poisonshown above → test fails.bun bd test): subprocess rejects withEISDIRevery iteration, exits 0 → test passes.cp.test.tssuite: 38 pass, 3 skip (Windows-only), 0 fail.zig:check-allpasses on all targets.