shell builtins: remove unsafe from rm/cp/yes/ls/mkdir and EnvStr - #40262
Jarred-Sumner wants to merge 2 commits into
Conversation
|
Updated 11:52 AM PT - Sep 8th, 2026
✅ @autofix-ci[bot], your commit c01429200a4f280b86670252770dae7080c012a4 passed in 🧪 To try this PR locally: bunx bun-pr 40262That installs a local version of the PR into your bun-40262 --bun |
![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. Because it's a substantial redesign of concurrency-sensitive ownership (rm's cross-thread DirTask tree, the Children pending/parked handoff, new event-loop primitives, and EnvStr's refcount model), a human look would still be worthwhile.
What was reviewed:
Children::release/doneordering: the parked store is sequenced before the parent's SeqCstfetch_sub, so the last decrementer always sees the box; the reset-to-1 letsdelete_after_waiting_for_childrenre-release on the ENOTEMPTY re-spawn path.EnvMap::insertunderEnvStr: Drop:get_or_putdrops the duplicate key on found-existing and default-initialises the value slot on insert, so the*value_ptr = valassignment dropsEmpty/the old value correctly.- Per-
DirTaskposter lifecycle: each task isarm()ed viaschedule_no_ref,new_childclones the parent's poster before the parent's ownon_finishconsumes it, and the rootShellRmTaskis armed separately. remove_entry_dir'shanded_offgate keys onself.children.is_some(), which is only set once a child is actually scheduled (theerrored()early-outs precedeget_or_insert_with).
Extended reasoning...
Overview
This PR completes the zero-unsafe programme for src/runtime/shell/ (except subproc.rs): 46→0 in rm.rs, plus cp/yes/ls/mkdir/EnvStr/ParsedShellScript. The largest change is a full redesign of rm -r's cross-thread DirTask tree: the raw-pointer parent/child graph with subtask_count/need_to_wait/pending_main_callbacks atomics is replaced by linear Box<DirTask> ownership handed through an Arc<Children { pending: AtomicUsize, parked: Guarded<Option<Box<DirTask>>> }>, with per-root shared state in Arc<RmTree>. EnvStr becomes a plain enum { Empty, Refcounted(Rc<[u8]>), Slice(BackRef<[u8]>) } with Drop; RefCountedStr.rs is deleted and every manual ref_()/deref() call site is removed. cp/yes move to Option<Box<Task>> taken/put-back. New primitives: EventLoopHandle::enqueue_boxed_after_yield and MiniEventLoop::tick_platform_loop. Also fixes a hang (rm's per-operand stderr chunks now numbered) with a new regression test, plus two incidental fixes (non-Linux ENOTEMPTY re-spawn stranding, and a leaked ref on get_homedir() in Expansion's leading-tilde path).
Security risks
None identified. This is internal ownership/lifetime restructuring; no new user-facing surface, parsing, or trust boundaries. The changes generally reduce risk by removing hand-written unsafe impl Send, raw-pointer container_of recovery, and manual refcount balancing.
Level of scrutiny
High. The rm rewrite replaces a subtle atomic protocol (whose lost-wakeup window already required a stress test) with a different subtle atomic protocol. The Children handoff, the delete_after_waiting_for_children loop that can re-spawn and re-release, the per-DirTask ShellTask poster lifecycle across schedule_no_ref/new_child/on_finish, and the interaction between handed_off and self.children.is_some() under concurrent errored() short-circuits are all exactly the class of code REVIEW.md flags for careful maintainer review. The EnvStr change touches every env-map insert/get site and changes insert from ref-on-insert to move-in semantics.
Other factors
The PR is stacked on #40228 (base claude/shell-zero-unsafe → #40204). Test coverage is solid (476/476 shell builtin tests debug+ASAN, hand-driven diff vs base release, Worker termination mid-walk, rust-check-all for windows/darwin) and instruction-count deltas are within noise. The new closed-pipe test targets the specific hang fixed. But the sheer scope of the ownership redesign — and the fact that it's stacked — means a maintainer should walk the Children protocol and the DirTask state machine before this lands.
3438deb to
80264a4
Compare
fcbbf71 to
4a2c506
Compare
80264a4 to
a09c68f
Compare
4a2c506 to
ae77500
Compare
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Given the scope — a full redesign of rm's concurrent DirTask ownership (the Arc<Children> park/hand-off protocol), the EnvStr refcount → Rc<[u8]>/Drop migration touching every env-map insert/get site, and new event-loop primitives — a human look at the concurrency hand-off and the EnvMap::insert key-leak question would still be worthwhile.
Checked: the Children::release/done sequencing (park-then-fetch_sub, reset to 1) against lost wakeups; delete_after_waiting_for_children now loops on re-spawn (fixes the non-Linux ENOTEMPTY strand noted in the description); EnvMap::insert on an existing key drops the old value via get_or_put overwrite and the passed-in duplicate key drops on move; cp's CowSlice args borrow this.src_absolute/tgt_absolute before this moves into create_for_shell — the borrow ends at the call boundary and the Vecs ride inside the box.
Extended reasoning...
Overview
This PR completes the zero-unsafe programme for src/runtime/shell/ (except subproc.rs), touching 23 files. The largest piece is a ground-up redesign of rm's recursive delete tree: the raw-pointer DirTask graph with subtask_count/need_to_wait/pending_main_callbacks atomics and hand-written unsafe impl Send is replaced by linearly-owned Box<DirTask> handed between threads via an Arc<Children { pending: AtomicUsize, parked: Guarded<Option<Box<DirTask>>> }> parking protocol, with per-root shared state in Arc<RmTree>. EnvStr moves from a packed u128 + intrusive RefCountedStr to a plain enum { Empty, Refcounted(Rc<[u8]>), Slice(BackRef<[u8]>) } with Clone/Drop, and every manual ref_()/deref() call site (EnvMap, cd, which, export, Assigns, Expansion, interpreter's PWD/OLDPWD) is removed. cp and yes switch to Option<Box<Task>> take/put-back ownership. RefCountedStr.rs and ParsedShellScript.this_jsvalue (write-only) are deleted. New event-loop primitives enqueue_boxed_after_yield and MiniEventLoop::tick_platform_loop are added. It also fixes a hang (rm's per-operand stderr chunks now take sequence numbers so a closed pipe calls each back) with a regression test, and a leak in Expansion's leading-tilde path.
Security risks
None identified. This is an internal memory-ownership refactor of shell builtins; no new user-facing surface, no parsing of untrusted input changes, no auth/crypto/permissions code touched. The rm path traversal / NOFOLLOW behaviour is unchanged.
Level of scrutiny
High. The rm redesign replaces a subtle raw-pointer concurrent hand-off protocol with a different subtle safe one — Children::release parks the box under a mutex then does a SeqCst fetch_sub; the last decrementer takes it and resets pending to 1 for the next round. This is production-critical (rm -rf on the thread pool) and a stranded task hangs the process. The EnvStr change alters ownership semantics at every insert/get: EnvMap::insert no longer refs — it moves — and clone() relies on ArrayHashMap::clone deep-cloning EnvStr (which now derives Clone). These are exactly the shapes REVIEW.md calls out under "Reference counts provably balanced on every terminal path" and "Know the thread affinity of every line you touch".
Other factors
The PR is stacked on #40228/#40204, is well-tested (476/476 shell tests, ASAN-clean, instruction-count deltas within noise, cross-target check-all), and the description is thorough. The bug-hunting system flagged nothing. I traced the Children protocol against the old need_to_wait/subtask_count dance and it appears equivalent (the store-into-parked-before-fetch_sub ordering mirrors the old need_to_wait.store(true)-before-fetch_sub), and the delete_after_waiting_for_children loop now correctly re-parks on ENOTEMPTY re-spawn (the old code set do_post_run = false and returned, stranding the dir on non-Linux). But this is a ~1000-line concurrent-ownership redesign with new architectural primitives in bun_event_loop; it warrants maintainer eyes on the hand-off protocol and the EnvMap key lifecycle rather than an automated approval.
a09c68f to
393343a
Compare
ae77500 to
4da140e
Compare
393343a to
550a539
Compare
99e10d9 to
38befe7
Compare
38befe7 to
550b4be
Compare
0ffcbbd to
2dead19
Compare
550b4be to
6cb4a62
Compare
2dead19 to
2288ef8
Compare
6cb4a62 to
4c490c9
Compare
2288ef8 to
c8ea1ef
Compare
4c490c9 to
5f469e8
Compare
rm: the recursive DirTask tree is an owned tree. Each directory is a Box<DirTask>; a directory that spawned child tasks parks its box in an Arc<Children> (its own slot plus one per unfinished child) and whoever takes the count to zero owns the box and deletes the directory. Per-root shared state is Arc<RmTree>; the root DirTask carries the ShellRmTask and posts it back when the tree is done; verbose output hops post the DirTask box itself. On non-Linux, a directory found non-empty after its children finished (and re-spawned) now releases its slot again instead of never completing. rm numbers its per-root error chunks like its verbose chunks, so `rm nope1 nope2 2>&1 | true` no longer waits for callbacks the closed pipe collapsed into one. cp: the fs.cp task owns the ShellCpTask box and hands it back through cp_on_finish. yes: the bounce payload is a Box taken from the builtin, queued through EventLoopHandle::enqueue_boxed_after_yield and put back when it runs. mkdir collects verbose output through a RefCell; ls -l writes the mode/time bytes directly. ParsedShellScript drops a write-only JsRef. EnvStr is an enum (Empty / Rc<[u8]> / BackRef<[u8]> for literals, script text and the dotenv loader) instead of a packed tagged pointer with manual ref/deref; EnvMap looks up by &[u8] and owns what it stores. PWD/OLDPWD hold copies rather than slices of the cwd buffers. Instruction counts vs the base branch (release, perf stat, pinned, interleaved): 1000x `bun run --shell=bun` true -0.02%, 200x rm -rf of a 100-file dir -0.03%, 100x rm -rf of a 131-dir tree +0.09% (-v: +0.04%), 3000x JS $`true` +0.05%.
9d93c46 to
c014292
Compare
5f469e8 to
93a4ba0
Compare
This branch has not been deployed
2 participants
What
Same programme as #40055 … #40261. Stacked on #40228 (base branch
claude/shell-zero-unsafe→ #40204; retarget as those land). With this, every file undersrc/runtime/shell/exceptsubproc.rs(#40204) is at zero.builtin/rm.rs46 → 0,yes.rs6 → 0,cp.rs3 → 0,ls.rs2 → 0,mkdir.rs1 → 0,EnvStr.rs4 → 0 (RefCountedStr.rsdeleted),ParsedShellScript.rs1 → 0.DirTasktree becomes an owned tree — aBox<DirTask>is owned linearly; a directory that spawned children parks its box inArc<Children { pending: AtomicUsize, parked: Guarded<Option<Box<DirTask>>> }>and whoever takespendingto 0 owns it. Per-root shared state isArc<RmTree>; the root carriesOption<Box<ShellRmTask>>and posts it viaShellTask::on_finish; verbose hops post theDirTaskbox (shell_dispatch!(ShellRmDirTask)). No hand-writtenunsafe impl Send.Option<Box<ShellCpTask>>/Option<Box<YesTask>>taken → queued → put back;cp_on_finish(self: Box<Self>).enum { Empty, Refcounted(Rc<[u8]>), Slice(BackRef<[u8]>) }withDrop;EnvMap::get(&[u8]) -> Option<&EnvStr>,inserttakes ownership; all manualref_()/deref()sites removed.PWD/OLDPWDare now copied (the old slices pointed into the mutable cwdVecacross dup'd envs).ParsedShellScript.this_jsvaluewas write-only — removed.EventLoopHandle::enqueue_boxed_after_yield<T: Taskable>(no unsafe);MiniEventLoop::tick_platform_loop(one block under the existingloop_ptr()invariant, same astick_once).Fixes the #40228 review finding: rm's per-operand stderr chunks now take a sequence number (
exec.next_chunk(cmd)), sorm nope1 nope2 2>&1 | trueno longer hangs — new testrm errors for several operands into a closed pipe finish(hangs on 1.4.0 and on the base build, passes here). Also fixed in passing: the non-LinuxENOTEMPTYre-spawn path indelete_after_waiting_for_childrenstranded the directory;Expansion's leading-tilde path leaked a ref onget_homedir().Instruction counts (release,
taskset -c 56-63, interleaved,perf stat -e instructions:u, median): 1000×bun run --shell=buntrue−0.021 %; 200×rm -rfof a 100-file dir −0.025 %; 100×rm -rfof a 131-dir tree +0.090 % (-v+0.037 %); 3000× JS$`true`+0.047 %.Testing
Debug+ASAN: rm/cp/yes/mv/bunshell/exec/export 476/476;
test/js/bun/shell/overall 939 pass with the known sandbox artifacts (root-userlspermission cases, file-io non-existent dir, relative-bunExe ls case) and debug-load timeouts inleak.test.ts/shell-loadthat pass 43/43 on the release build;run-shell+test/regression/issue/*shell*6/6. Hand-driven vs base release output:rm -rfon a 2184-file 5-level tree,rm -rfv | head -1, rm error paths,cp -r/-rv | head,ls -R | head,yes | head -1000,seq | tail,mv,export/assign/cd -/OLDPWD,mkdir -pv— identical modulo concurrent ordering; a Worker terminated midrm -rfvwalk × 6 clean; no ASAN output. clippy clean (bun_runtime, bun_event_loop);rust-check-allwindows-msvc + apple-darwin pass.