shell(cp): report ENAMETOOLONG instead of panicking on operands longer than the path buffers - #38162
shell(cp): report ENAMETOOLONG instead of panicking on operands longer than the path buffers#38162robobun wants to merge 2 commits into
Conversation
…r than the path buffers The cp builtin joined cwd + operand (and target + basename when copying into a directory) into fixed-size buffers with unchecked joins, so an operand whose resolved path did not fit aborted the process with "range end index N out of range for slice of length 4094". Add shell_join_path / shell_resolve_operand next to the other shared builtin helpers: they join through join_spill into an owned ZBox and fail with ENAMETOOLONG when the normalized result is not shorter than MAX_PATH_BYTES, the bound the node:fs cp implementation requires of the paths it is handed. cp resolves its three paths through them and keeps the resulting ZBoxes as src_absolute / tgt_absolute instead of copying them into Vecs.
|
Warning Review limit reached
Next review available in: 51 minutes 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 (3)
Comment |
|
Status: fix pushed; CI green on every lane that ran (Windows x64 and aarch64, Linux glibc/musl/ASAN, clippy, format). The only macOS test lane never got an agent in the first run, so it was re-triggered once to get the PATH_MAX = 1024 branch of the new test exercised. Reproduced on bun 1.4.0 and on a debug build of main with The new test in |
There was a problem hiding this comment.
LGTM — swaps the unchecked join_z/join_z_buf calls for a spill-capable helper that returns ENAMETOOLONG when the normalized path won't fit MAX_PATH_BYTES, matching the rm/mv fixes.
Checked that join_spill normalizes with the same routine as before and ZBox::from_bytes copies out of the thread-local/spill borrow; the >= MAX_PATH_BYTES bound is correct against PATH_MAX's NUL-inclusive count (test pins both sides). The retyped src_absolute/tgt_absolute fields still satisfy the Windows EBUSY path (StringSet::contains/insert and eql_utf8 all take &[u8], which ZBox derefs to). with_path clones the joined slice, so the error owns its path. Test covers all four join sites, the exact boundary, and per-operand independence.
Extended reasoning...
Overview
Three files: src/runtime/shell/builtin/cp.rs replaces four unchecked resolve_path::join_z/join_z_buf calls in ShellCpTask::run_from_thread_pool_impl with two new helpers and retypes src_absolute/tgt_absolute from Option<Vec<u8>> to Option<ZBox>; src/runtime/shell/interpreter.rs adds shell_join_path and shell_resolve_operand next to the existing shared shell_dup/shell_openat helpers; test/js/bun/shell/commands/cp.test.ts adds a subprocess-based regression test.
Security risks
None. The change tightens input handling — a user-reachable slice-bounds panic on a work-pool thread becomes a caught ENAMETOOLONG that exits 1. No new syscalls, no privilege changes, no untrusted parsing added.
Level of scrutiny
Moderate. The cp builtin runs by default only on Windows (experimental flag on POSIX), and the diff is a mechanical substitution of the join primitive plus a length check — the same class of fix already merged for rm (#37521) and mv (#37527). I traced the helper: join_spill calls the same join/join_string_buf normalization as join_z_buf, just with a Vec fallback when join_needed(parts) > 4096; ZBox::from_bytes copies the borrowed slice into an owned NUL-terminated allocation before the thread-local/spill goes out of scope; bun_sys::Error::with_path boxes its argument, so the error path is owned. The >= MAX_PATH_BYTES bound is correct: on POSIX MAX_PATH_BYTES == PATH_MAX counts the NUL, so a joined.len() of exactly PATH_MAX - 1 (the OS max) passes and PATH_MAX fails — the test asserts both sides. On Windows the bound is 98302 (UTF-8 worst case of the NT wide limit), so paths between the old 4096-byte crash point and that limit now flow through to node:fs's own wide-buffer check.
Other factors
The Option<ZBox> retype touches Windows-only EBUSY code (ignore_ebusy_error_if_possible, on_shell_cp_task_done); I verified StringSet::{contains,insert} take &[u8] and ZBox: Deref<Target=ZStr>: Deref<Target=[u8]>, so .as_ref()/.as_deref() at those sites coerce correctly, and the author ran cargo check --target x86_64-pc-windows-msvc. The test is well-constructed: subprocess isolation so the pre-fix panic surfaces as a failed assertion, Buffer.alloc(n, fill) per harness convention, exact stderr assertions with placeholder normalization, drains stdout/stderr/exited concurrently, asserts stderr before exitCode, and pins the boundary via reported lengths rather than assuming the tempdir's depth. The author confirmed USE_SYSTEM_BUN=1 fails and the debug build passes, plus a 23-case behavior-preservation probe. Helpers are placed in interpreter.rs so touch/mkdir (same bug, called out as follow-ups) can reuse them.
|
Updated 12:50 PM PT - Aug 13th, 2026
✅ @robobun, your commit 6ff124e40da4d58c4740be29e9ccb4938df68b17 passed in 🧪 To try this PR locally: bunx bun-pr 38162That installs a local version of the PR into your bun-38162 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces new shared path-resolution helpers in interpreter.rs (intended for reuse by touch/mkdir/etc.) and retypes fields read by the Windows-only EBUSY pass, a human look would still be worthwhile — particularly on whether the helper placement/signature is what you want the other builtins to converge on.
What was reviewed:
shell_join_pathusesjoin_spill(won't panic on oversized input) and bounds-checks the normalized result againstMAX_PATH_BYTESbeforeZBox::from_bytes, so long operands that normalize down still pass and the error path names the joined path.- The
Vec<u8>→ZBoxretyping ofsrc_absolute/tgt_absolute: all readers (is_dir,CowSlice::init_unchecked, the Windows EBUSYeql_utf8/StringSetsites) take&[u8]/AsRef<[u8]>, whichZBoxderefs to; the ENAMETOOLONG return happens before these fields are set, so it isn't deferred to the EBUSY phase. - The two comment-cop flags on the helper doc comments look like false positives — they're ordinary function contracts, same shape as the neighboring
shell_dup/closefddoc comments, not workaround justifications.
Extended reasoning...
Overview
The PR replaces unchecked resolve_path::join_z/join_z_buf calls in ShellCpTask::run_from_thread_pool_impl with two new shared helpers, shell_join_path and shell_resolve_operand, added to src/runtime/shell/interpreter.rs. These use join_spill (which grows a Vec instead of writing past a fixed buffer) and return ENAMETOOLONG as a bun_sys::Error when the normalized result would not fit MAX_PATH_BYTES. The src_absolute/tgt_absolute fields on ShellCpTask are retyped from Option<Vec<u8>> to Option<ZBox> so the helper's return value is stored directly instead of being copied. A subprocess-based test in cp.test.ts covers a 100 KB source, target, absolute target, a mixed batch (verifying the good source is still copied), and on POSIX a deep directory that pins both sides of the PATH_MAX - 1 boundary for both the non-recursive and -R tgt/basename join sites.
Security risks
None identified. The change tightens input handling — argv-sized paths that previously triggered a bounds panic on a work-pool thread now surface as a recoverable ENAMETOOLONG and exit code 1. No new attack surface is exposed; the length check is applied after normalization, so it does not reject anything that previously worked.
Level of scrutiny
Medium. The cp builtin is only default-enabled on Windows (experimental on POSIX), so blast radius is bounded, but the change is not mechanical: it adds two pub(crate) helpers to interpreter.rs that the PR positions as the convergence point for the same fix in touch, mkdir, rm, and mv. That is a small architectural decision (helper placement, bun_sys::Result<ZBox> shape, taking a bun_sys::Tag for the syscall label) that a maintainer should sign off on before other builtins are pointed at it. The ZBox retyping also flows through the Windows-only EBUSY deferral path (on_shell_cp_task_done, ignore_ebusy_error_if_possible), which is #[cfg(windows)]-gated and easy to miss locally — the PR reports cargo check --target x86_64-pc-windows-msvc passes and Windows CI is green.
Other factors
- The PR description is thorough and includes a 23-case behavior-preservation probe against bun 1.4.0. The test asserts exact stderr, exit code, and the boundary lengths themselves, and verifies the sibling source in a mixed batch is still copied.
- Two
comment-cop(github-actions) inline comments flag the doc comments on the new helpers with the repo's "paragraph-long comment justifying a workaround" rule. Reading the flagged comments (interpreter.rs:2168-2171 and 2185-2186), they are ordinary function-contract doc comments of the same length and shape as the adjacentshell_dup/closefddoc comments — not workaround justifications — so I read these as bot false positives, but they are technically unaddressed. - No prior
claude[bot]review on this PR.
Problem
cpbuiltin (default on Windows,BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1on POSIX) takes the whole process down when a source or destination operand resolves to a path longer than its scratch buffers. In a directory/tmp/cplcontaining a filef, on bun 1.4.0:BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 bun -e 'const r = await Bun.$`cp f ${"a".repeat(5000)}`.nothrow().quiet(); console.log(r.exitCode)'cp: cannot stat '...': File name too longand exits 1.dir/basename(src)reaches PATH_MAX (range end index 4095 out of range for slice of length 4094), with and without-R.ShellCpTask::run_from_thread_pool_impl(src/runtime/shell/builtin/cp.rs) joins cwd + operand withresolve_path::join_z(4096-byte thread-local buffer on every platform, so on Windows even paths NT accepts crashed) andjoin_z_bufinto two stackPathBuffers, then joinstgt + basenameinto the second one. Those joins write the normalized result with unchecked slice indexing, so argv-sized input is a bounds panic on a work-pool thread. Same class as shell(rm): stop panicking on operands longer than the path scratch buffers #37521 (rm) and shell(mv): report ENAMETOOLONG instead of panicking when a source name does not fit the path buffers #37527 (mv).Fix
shell_join_path/shell_resolve_operand(src/runtime/shell/interpreter.rs, next toshell_dup/shell_openat, the other helpers shared by the builtins) join throughresolve_path::join_spill, the existing variant that falls back to aVecwhen the thread-local buffer is too small (used by rm, mv, Glob and readdir), copy the result into an ownedZBox, and returnENAMETOOLONG(as abun_sys::Errortagged with the caller's syscall, naming the joined path) when the normalized result is not shorter thanMAX_PATH_BYTES. They returnbun_sys::Resultso cp wraps at the call site exactly like itsis_dircalls (ShellErris too large to return directly; clippy'sresult_large_erris denied in this workspace).src,tgtand the twotgt/basenamesites through them. The resultingZBoxes becomesrc_absolute/tgt_absolutedirectly (the fields wereVec<u8>copies of the stack buffers; every reader takes&[u8], whichZBoxderefs to), so nothing is copied twice.os_pathcopies them intoMAX_PATH_BYTESbuffers; on POSIX a longerdestbecomes an empty path there (PathLikeExt::slice_z), sodir/basenamehas to be checked before the hand-off, it never reaches a syscall in this function.MAX_PATH_BYTESis PATH_MAX on POSIX (which counts the NUL), so a path of exactlyMAX_PATH_BYTES - 1bytes, the longest the OS accepts, still passes; the test pins both sides of that boundary. On Windows it is the UTF-8 worst case of the NT limit, so anything rejected here could not have fit the wide buffer either, and shorter paths keep going through cp_async's existing wide-buffer check.src/tgtto the kernel: one code path and one message; the length is checked after normalization, so a long operand that normalizes down still works as before.ShellErr::Sysrendering, so it reads like the kernel-reported errors cp already prints (cp: File name too long: /abs/path), exits 1, and the other source operands of the same invocation are still copied (one task per source). It is returned beforesrc_absolute/tgt_absoluteare set, so on Windows it is not held back by the EBUSY pass.join_spillnormalizes with the same functionjoin_z_bufused; only the storage moved from two stackPathBuffers (about 196 KB of stack per task on Windows) plus the thread-local buffer to the heap. A 23-case probe (all three synopses, trailing slashes,..segments, absolute operands, identical files, missing source, non-directory target,-voutput, error messages) gives byte-identical output on the unfixed 1.4.0 binary and this build; list below.rmandmvhave their own open PRs (shell(rm): stop panicking on operands longer than the path scratch buffers #37521, shell(mv): report ENAMETOOLONG instead of panicking when a source name does not fit the path buffers #37527).touch(touch.rs, the same two-branch join into aPathBuffer) andmkdir(mkdir.rs,join_zfor relative operands) still abort on the same input and are being handled separately; they can switch to these helpers in one or two lines each once this lands, which is why the helpers live ininterpreter.rsand take the syscall tag.test/js/bun/shell/commands/cp.test.ts, "operands longer than the path buffers are reported as ENAMETOOLONG". Runs the builtin in a child process (the in-process cp suite in that file is skipped on POSIX;builtinDisabled("cp")does not look at the env var) and covers a 100000-byte source, destination and absolute destination (longer than the buffer on every platform, so these run on Windows too), a long source next to a normal one (normal one copied, exit 1), and on POSIX a directory nested to just below PATH_MAX: a file whosedir/nameis exactly PATH_MAX - 1 bytes is copied, one byte longer fails withENAMETOOLONGnamingdir/name, with and without-R(the two join sites). The fixture reports the lengths, so the boundary itself is asserted.USE_SYSTEM_BUN=1 bun test test/js/bun/shell/commands/cp.test.ts -t "path buffers": fails, the child aborts with the first panic above. Each of the two deep-directory overflow cases also aborts the unfixed binary on its own (second panic above); the at-limit case passes on both builds.bun bd test test/js/bun/shell/commands/cp.test.ts: passes.cat file > x, which hang identically on the unfixed binary (cat builtin, shell(cat): read regular files synchronously in the builtin on POSIX #35337 / shell(cat): finish after a read error instead of cancelling the queued output and hanging #37743).cargo clippy -p bun_runtime --no-deps,cargo fmt --check, andcargo check -p bun_runtime --target x86_64-pc-windows-msvc(the EBUSY code reads the retyped fields) pass.Background
src/runtime/shell/builtin/and run in-process.cpandcatare built everywhere but only dispatched by default on Windows; on POSIXBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1enables them, otherwise the system binary is spawned. The other builtins (mkdir, touch, rm, mv, ...) are on everywhere.ShellCpTaskis created per source operand and runs on a work-pool thread: it resolves the operands to absolute paths, picks the POSIX cp synopsis (file to file,-Rinto a directory, files into a directory), then hands the copy to the node:fsfs.cpimplementation (cp_asyncinnode_fs.rs). That hand-off is why the shell-side paths must satisfy node:fs's buffer bound.resolve_path::join*concatenate their parts and normalize them (.,.., repeated separators; a trailing separator is kept, whichcp dir/relies on) into a caller buffer (join_z_buf), a 4096-byte thread-local (join,join_z), or, for the*_spillvariants, a caller-ownedVecwhen the parts would not fit. The non-spill variants do not bounds-check their output.MAX_PATH_BYTESis the size of bun'sPathBuffer: 4096 on Linux and 1024 on macOS (both equal to PATH_MAX, which includes the NUL, so the longest usable path is one byte less), 98302 on Windows (3 bytes per UTF-16 unit of the 32767-unit NT limit, plus one).ZBoxis bun's owned NUL-terminated byte string, the heap counterpart of the&ZStrthe syscall wrappers take; it derefs toZStrand on to[u8](without the NUL).ShellErr::Sysis how builtins report errno failures; it renders as<cmd>: <coreutils message>: <path>and the builtin's state machine turns it into exit code 1.Behavior-preservation probe (identical output on bun 1.4.0 and this build)
Run with
BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1on Linux; exit code, stdout, stderr and resulting files compared for:cp -v file new,cp -v file existing,cp -v file dir,cp -v file dir/,cp -v file missing/(error),cp file missing(creates it),cp -v a b dir,cp a b file(error),cp a ./aandcp a /abs/a(identical errors),cp -R dir existingdir,cp -R dir newdir,cp -R dir/ newdir,cp -R dir existingdir/,cp -R file dir,cp -R a b missing(error),cp dir xwithout-R(error), missing source (error message with absolute path), absolute source and destination,cp dir/../a dir/./x/../b, an empty operand,cp dir/file .,cp -R dir/. dst.