shell: free the cp builtin's task when its fs.cp completion is released unrun - #38502
shell: free the cp builtin's task when its fs.cp completion is released unrun#38502robobun wants to merge 4 commits into
Conversation
…ed unrun On the success path the shell's cp builtin hands the copy to a ShellAsyncCpTask, and that task's completion is what continues (and frees) the ShellCpTask via cp_on_finish. When a worker tears down while the copy is in flight, the completion is released unrun instead, and ShellAsyncCpTask::release_unrun only destroyed itself, leaking the ShellCpTask and the keep-alive it took when it was scheduled. Release the shell task from there as well, the same way a ShellCpTask posted directly (the error path) is released. The worker-late-completion rows now run their host under LeakSanitizer on ASAN builds themselves instead of relying on the CI runner's environment, so the "$ cp -R" row reports this; the file's per-test timeout is raised since every test in it starts a debug bun and a worker.
|
Warning Review limit reached
Next review available in: 50 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: reproduced and fixed; ready for review. Reproduced on a debug (ASAN) build of main at 032b8db with CI (build 96022): every lane that has run is green (the only annotations are retried-and-passed flakes in unrelated files). The two |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The change looks correct — ParentRef is Copy so the shell-task pointer is read out before Self::destroy(this), the non-shell instantiation keeps shelltask = None so its behaviour is unchanged, and the release order (fs task first, then shell task) matches the borrow direction of args. That said, this is unsafe cross-thread ownership/lifecycle code in the memory-safety category, so a human look at the ownership argument (particularly that nothing else — e.g. the Windows EBUSY list — can still hold the ShellCpTask when release_unrun runs) would be worthwhile.
What was reviewed
NewAsyncCpTask::release_unrun: confirmedshelltaskisNonewhen!IS_SHELL(set viaParentRef::from_nullable_mutinschedule_new), so the JSfs.cppath is untouched.<ShellCpTask as Taskable>::release_unrunatcp.rs:754already handles keep-alive unref + box drop; delegating to it mirrors the error-path release.- Test:
leaksan.suppexists at the referenced path;setDefaultTimeout(90_000)is file-wide because every row spawns a debug/ASAN subprocess concurrently — matches what CI already applied.
Extended reasoning...
Overview
The PR fixes a leak of the shell cp builtin's ShellCpTask when a worker tears down while an fs.cp-backed copy is in flight. The native change is ~10 lines in src/runtime/node/node_fs.rs: <NewAsyncCpTask<IS_SHELL> as Taskable>::release_unrun now reads the shelltask back-pointer (an Option<ParentRef<ShellCpTask, Mut>>, Copy) before destroying the fs task, then — if Some — forwards to <ShellCpTask as Taskable>::release_unrun, which unrefs the event-loop keep-alive and drops the heap box. The test file gains a per-row LeakSanitizer environment on ASAN builds and a 90s file default timeout.
Security risks
None. This is internal task-lifecycle cleanup during worker teardown; no user-facing input handling, parsing, or protocol surface is touched.
Level of scrutiny
High. The repo's review guidance calls out native memory safety as the most-blocked category, and this change reasons about exactly-once release of a heap box across the pool→JS-thread completion boundary in unsafe Rust. The mechanism is well-argued in the description (posted only after the last subtask, run-path never touches the shell task after handoff, Windows EBUSY list only populated on the run path), and the code reads correctly to me: ParentRef is Copy (src/ptr/parent_ref.rs:169), so the pointer survives Self::destroy(this); the non-shell instantiation has shelltask = None so behaviour is byte-identical there; and the fs-task-first destroy order respects the args → shell-task path borrow. But confirming that no other holder of the ShellCpTask pointer exists at this point — the invariant that makes this a free rather than a UAF — is exactly the kind of ownership claim a maintainer familiar with the shell builtin lifecycle should sign off on.
Other factors
The test changes are sound: test/leaksan.supp exists, ASAN_OPTIONS is composed with any inherited value, and the 90s setDefaultTimeout is justified (every row spawns a debug/ASAN child concurrently; the description notes CI already ran this file at 90s/270s). The PR demonstrates the test fails on the unfixed build and passes with the fix, and test/js/node/fs/cp.test.ts still passes. No prior human review comments to address.
|
On the ownership question (who else can hold the
|
A leak: suppression matches any frame of the recorded allocation stack, and test/leaksan.supp suppresses Bun::evaluateCommonJSModuleOnce, which on the release ASAN build is still within the 30 frames recorded for what a row allocates from the worker's module body. The rows' leak check therefore only bit on debug builds; starting the work from an immediate keeps module evaluation out of every row's allocation stacks.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// its promise handle, protected arguments and keep-alive. The shell's | ||
| /// variant is also the completion of the `ShellCpTask` that handed it | ||
| /// the copy (`run_from_js_thread` → `cp_on_finish`), so that task is | ||
| /// released unrun here too; nothing else frees it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Shortened in 5264b79: the doc comment now just notes that for the shell this completion is also what frees the ShellCpTask, and the SAFETY comment is down to the two facts the body relies on (the pool is done with both tasks; args borrows the shell task's paths, which fixes the order).
There was a problem hiding this comment.
Reworked again in the latest push: the doc comment is back to what main has, and the one fact this change adds (the shell task whose completion this task carries) now lives in the SAFETY comment, where it is part of the argument for freeing it.
|
Second commit (d2d31fa) changes only the test: each row's work is now started from a Reason: a Re-verified on the debug build with this host: with the |
| /// its promise handle, protected arguments and keep-alive. For the | ||
| /// shell this completion is also what frees the `ShellCpTask` | ||
| /// (`cp_on_finish`), so that is released unrun here as well. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
…in the safety comment
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because the fix reasons about cross-thread ownership of ShellCpTask inside an unsafe release_unrun path — where a mistake is a double-free rather than a leak — a human look at the ownership argument would still be worthwhile.
What was reviewed:
- Traced
shelltaskthroughcreate_for_shell→ParentRef::from_nullable_mut→as_mut_ptr; the non-shell instantiation staysNone, sofs.cpbehavior is unchanged. - Confirmed
ShellCpTask::release_unrun(cp.rs:754) doesunref_unrun+heap::take, matching what the run path'scp_on_finishwould have released; destroy order (fs task first) matches theargs-borrows-paths constraint. - Checked the test's
leakCheckEnvcomposes withbunEnv.ASAN_OPTIONSand that thesetImmediatewrapper keeps the two-turn exit sequencing intact.
Extended reasoning...
Overview
The PR fixes a leak of the shell cp builtin's ShellCpTask when a worker tears down while a $ cp -R is in flight. The Rust change is ~12 lines in NewAsyncCpTask::release_unrun (src/runtime/node/node_fs.rs:1506): after destroying the fs task, it now also calls <ShellCpTask as Taskable>::release_unrun on the back-pointer the fs task carried, which unrefs the keep-alive and drops the box (cp.rs:754-759). The test change makes every row of worker-late-completion.test.ts run its host under LeakSanitizer on ASAN builds, wraps each row's work in setImmediate to escape the evaluateCommonJSModuleOnce suppression frame, and raises the file's default timeout to 90s.
Security risks
None. This is internal task-lifecycle cleanup on a debug-gated teardown path; no user-controlled input reaches the changed code, and no auth/crypto/permission surface is touched.
Level of scrutiny
High. Per the repo's review guidance, native memory safety is the most-blocked category. The correctness of this change rests on an ownership argument spanning two files and multiple threads: that when release_unrun fires, (a) the pool is done with both tasks (on_subtask_done posts only at count zero), (b) nothing else holds the ShellCpTask (the error path posts the shell task itself and never creates the fs task; the Windows EBUSY list is only populated by the run path), and (c) the destroy order respects the args-borrows-shell-task-paths dependency. The PR thread lays this out carefully and I did not find a hole in it, but a mistake here is a double-free/UAF rather than a leak, so it deserves a second set of eyes on the argument rather than an auto-approval.
Other factors
- The fix mirrors the run path exactly:
run_from_js_thread(node_fs.rs:1702) readsshelltask, hands it tocp_on_finish(which frees it), then destroysself.release_unrunnow does the release-path equivalent in the safe order. - The non-shell
AsyncCpTaskinstantiation hasshelltask == None(created withnull_mut()at node_fs.rs:1557), so plainfs.cpbehavior is unchanged. - Test coverage is strong: LeakSanitizer is now enabled per-row, the
$ cp -Rrow demonstrably fails without the fix and passes with it, and thesetImmediatechange is well-justified (avoids a suppression that hid the leak on the release-ASAN lane). - The file-wide
setDefaultTimeout(90_000)is broader than the repo's usual per-test-outlier guidance, though the comment explains why (every test spawns a debug bun + worker; a failing row's LSAN symbolization alone exceeds 5s) and CI already ran this file at 90s/270s. - The comment-cop bot fired twice on earlier revisions; the final SAFETY comment is now concise and states the two facts the body relies on.
1 participant
Problem
cp -Rrun through the shell'scpbuiltin in a worker, with the copy still in flight when the worker exits, leaks the builtin's task. LeakSanitizer on the$ cp -Rrow oftest/js/web/workers/worker-late-completion.test.ts:Direct leak of 440 byte(s) in 1 object(s)allocated at<bun_runtime::shell::builtins::cp::ShellCpTask>::create src/runtime/shell/builtin/cp.rs:422(viaCp::nextcp.rs:174,Builtin::start). Reproduces 6/6 on a debug build of main (032b8db).ShellCpTaskhands the copy to aShellAsyncCpTask(cp.rs:730,run_from_thread_pool_impl) and drops its own poster. TheShellAsyncCpTask's completion is then the only thing that reaches theShellCpTaskagain:run_from_js_thread(node_fs.rs:1690) callscp_on_finish, which continues the builtin and frees the task.<ShellAsyncCpTask as Taskable>::release_unrun(node_fs.rs:1506) only destroyed the fs task. TheShellCpTaskit pointed at, and the keep-aliveShellCpTask::scheduletook, were never released.ShellCpTaskis posted itself and<ShellCpTask as Taskable>::release_unrunfrees it. The other pool builtins (ls,rm,mv,mkdir,touch, glob) post their own task and theirrelease_unrunimpls mirror their run paths;cpis the one whose completion is carried by another type.Fix
release_unrunofNewAsyncCpTasknow also releases the shell task it carries the completion for (Someonly in theIS_SHELLinstantiation), through<ShellCpTask as Taskable>::release_unrun: unref the keep-alive, drop the box. The fs task is destroyed first because itsargsborrow the shell task's absolute paths.on_subtask_doneposts only after the last pool subtask is done with both tasks (thecp_on_copycallers), the builtin's own pool callback never touches the task after handing the copy over, and nothing else holds a pointer to it on POSIX (the Windows EBUSY list is only filled by the run path).release_unrunruns on the task's own JS thread with the loop alive, which is whatShellCpTask::release_unrunalready assumes on the error path.test/js/web/workers/worker-late-completion.test.ts. Its rows now run the host under LeakSanitizer themselves on ASAN builds (detect_leaks=1,test/leaksan.supp,BUN_DESTRUCT_VM_ON_EXIT=1, the same environment the CI runner supplies on the ASAN lane), so a release path that leaks fails its row under a plainbun bd testtoo. Every test in the file starts a debug bun plus a worker and the last one waits out the 2s ticket report, so the file's default timeout is raised to 90s (passing rows take about 3s on a debug build here, and the failing row about 7s once LeakSanitizer has symbolized its report, past the 5s default; CI already ran the file with a 90s/270s timeout).bun bd test test/js/web/workers/worker-late-completion.test.ts: withsrc/stashed, only$ cp -Rfails, with the report above; with the fix, 33/33 pass (twice).bun bd test test/js/node/fs/cp.test.ts: passes; a manualcp -Rv/missing-source run with the builtin enabled under LeakSanitizer exits clean.Background
cpbuiltin:Cp::nextheap-allocates oneShellCpTaskper source operand and schedules it on the work pool. On the pool it resolves paths and hands the actual copying to node:fs's async cp implementation,ShellAsyncCpTask(NewAsyncCpTask<true>innode_fs.rs), which keeps a back-pointer (shelltask) to it. The fs task's subtasks report each copied file to the shell task (cp_on_copy), and its completion continues the builtin (cp_on_finish).Taskable::release_unrun: pool work posts a completion task into its VM's queue to run on the JS thread. Since One door out of a VM's thread: tickets + a teardown that waits #38299 a VM being torn down (a worker exiting or being terminated) waits for everything it sent off-thread to come back, and completions arriving during that wait are not run; they are handed to their type'srelease_unrun, which has to free whatever running them would have freed.ShellCpTask::schedulerefs the event loop's keep-alive so the loop stays alive while the task is out; the run path unrefs it inShellTask::run_from_main_thread, the release path inShellTask::unref_unrun.BUN_DEBUG_TEST_WORKER_TEARDOWN_GATE(debug-assertion builds): holds a cross-thread post until the worker's teardown is already waiting, so each row of the test exercises its release path deterministically. Withdetect_leaks=1, LeakSanitizer makes the host exit non-zero with a report if that path leaks;BUN_DESTRUCT_VM_ON_EXIT=1destroys the main VM at exit so what it still owns is not reported as well.