Skip to content

shell: fail an empty operand with ENOENT instead of acting on the cwd - #38002

Open
robobun wants to merge 1 commit into
mainfrom
farm/88acb2b4/shell-touch-mkdir-empty-operand
Open

robobun wants to merge 1 commit into
mainfrom
farm/88acb2b4/shell-touch-mkdir-empty-operand

Conversation

@robobun

@robobun robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • An empty operand makes the shell builtins act on the current directory instead of failing. On every platform: touch "" exits 0 and sets the cwd's mtime to now; mkdir "" prints mkdir: /path/to/cwd: File exists and mkdir -p "" exits 0; with the cp builtin (the default on Windows, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 elsewhere) cp "" out prints cp: is a directory (not copied), cp f "" exits 0 and cp -R "" out copies the cwd into itself until the worker thread overflows its stack (SIGSEGV); and when the cwd is a top-level directory such as /tmp, rm -f "" exits 1 with rm: "/tmp" may not be removed.
  • On Windows the fd-relative builtins do it too: ls "" lists the cwd and exits 0, rm -r "" (and rm -rf "") deletes the files in the cwd, rm -d "" tries to remove the cwd, mv f "" and mv f g "" exit 0 without moving anything, mv "" h tries to rename the cwd (EBUSY, exit 16), cat "" exits 21 silently.
  • coreutils fails all of these with exit 1 (touch: cannot touch '': No such file or directory, rm -f '' exits 0), as does every syscall given "".
  • Cause, first group: touch, mkdir and cp build an absolute path string by joining the operand onto the shell cwd (touch.rs, mkdir.rs, cp.rs, each task's run_from_thread_pool), and rm's refuse-to-remove-the-root check does the same (rm.rs, Rm::next); join(cwd, "") is cwd.
  • Cause, second group: ls, cat, rm -r and mv's target check open their operand through shell_openat, and rm/mv unlink and rename it through bun_sys's *at() wrappers. On POSIX the kernel returns ENOENT for an empty name, but the Windows emulation of these passes the name to NtCreateFile relative to the cwd handle, and an empty NT name means the handle's own directory. bun_sys relies on that itself (DeleteFileBun rewrites . to an empty name), so it cannot be changed there.

Fix

  • One helper, reject_empty_path (src/runtime/shell/interpreter.rs), returns ENOENT for an empty operand. It is applied at the lowest point the shell owns on each path: shell_openat (covers ls, cat, rm -r, mv's target check), rm's operand classification in remove_entry_file (the single entry point for every rm operand, so the existing ENOENT handling and -f apply to it unchanged), mv's do_rename (the single rename point of all three move modes), and the three builtins that resolve operands as strings. rm's root check skips an empty operand and leaves it to the task.
  • Why this is the right result: ENOENT is what the syscall returns for "", so each builtin now behaves as if it had not rewritten the operand, and exit codes match coreutils. Every guard is keyed on the operand being empty, so no non-empty operand changes behavior; the existing per-operand error paths produce the messages, which are the ones the POSIX builds already printed for ls "" / rm "". The one message that changes on POSIX is mv f "", which used to blame f (mv: f: No such file or directory) and now prints mv: No such file or directory.
  • Not changed, on purpose: cd "" joins onto the cwd and stays there, which is what bash does; the [[ -f "" ]] / -d tests already check for an empty operand before calling shell_statat; redirections to "" are rejected earlier as an ambiguous redirect; the three string-resolving builtins keep their own resolution code (they differ in buffers and in whether absolute operands are normalized, and unifying that would change mkdir's handling of absolute paths, which this bug does not need). The cp -R crash itself is the self-copy recursion tracked in shell(cp): refuse copying a directory into itself and walk trees without recursion #37927; an empty operand no longer reaches it.
  • Tests: new commands/touch.test.ts and commands/mkdir.test.ts (commands/ has one file per builtin and these had none), and new cases in commands/cp.test.ts (runs cp in a child bun with the env var), commands/ls.test.ts, commands/rm.test.ts and commands/mv.test.ts. They cover the literal and interpolated empty word, -p/-v/-R/-r/-d/-f, both operand positions of cp and mv, the top-level-cwd case for rm, and that the non-empty operands of the same command are still processed while nothing else in the directory changes (the touch test pins the cwd's mtime first).
  • Verified: on Linux the touch, mkdir and cp cases (15), the rm top-level-cwd case and the mv a "" case fail on main and pass with this change; the remaining ls/rm/mv cases pass on Linux either way and pin what Windows now matches. On a Windows x64 machine, every new ls, rm, mv, touch and mkdir case (22) fails on the released build and all six files pass with this change (91 tests, including the existing cp builtin suite, which only runs on Windows). Also ran bunshell.test.ts, exec.test.ts, leak.test.ts and commands/* on Linux: the only failures need the network or a non-root user, or are stress tests that exceed their timeout on this debug build, identically without this change.

Background

  • A builtin such as touch a b runs one task per operand on the worker pool; a task records either its output or a bun_sys::Error, the builtin formats it as <cmd>: <path>: <message> (leaving the path out when it is empty) and exits 1 once every task is done if any failed. mv uses the failing errno as its exit code, hence exit 2 for ENOENT.
  • The shell has its own cwd ($.cwd(), cd), which can differ from the process cwd. Builtins therefore either pass operands to fd-relative calls (openat, unlinkat, renameat) against the shell's cwd fd, or, where the underlying implementation wants a path string (utimes, the node:fs mkdir and cp implementations), join the operand onto the cwd string with bun_paths::resolve_path::join, which, like path.join, returns the base unchanged when a component is empty.
  • shell_openat is the shell's wrapper around openat; on Windows it emulates the fd-relative open either by resolving the operand against the directory's path (also a join) or by calling NtCreateFile with the directory handle as RootDirectory.
  • cp and cat are the two builtins the shell enables by default only on Windows; on POSIX they fall through to the system binaries unless BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 is set, which is why a Linux reproduction only shows the touch and mkdir symptoms.
Reproduction

Linux, bun 1.4.0, in an empty directory:

import { $ } from "bun";
import { statSync, utimesSync } from "node:fs";
$.nothrow();
const past = new Date("2000-01-01T00:00:00Z");
utimesSync(".", past, past);
const t = await $`touch ""`.quiet();
console.log("touch", t.exitCode, JSON.stringify(t.stderr.toString()), statSync(".").mtime.toISOString());
const m = await $`mkdir ""`.quiet();
console.log("mkdir", m.exitCode, JSON.stringify(m.stderr.toString()));
const p = await $`mkdir -p ""`.quiet();
console.log("mkdir -p", p.exitCode, JSON.stringify(p.stderr.toString()));
const r = await $`rm -f ""`.cwd("/tmp").quiet();
console.log("rm -f (cwd /tmp)", r.exitCode, JSON.stringify(r.stderr.toString()));
touch 0 "" 2026-08-13T01:28:52.087Z
mkdir 1 "mkdir: /tmp/t5: File exists\n"
mkdir -p 0 ""
rm -f (cwd /tmp) 1 "rm: \"/tmp\" may not be removed\n"

Windows x64, bun 1.4.0, in a directory containing f, g and sub/inner (each line is one fresh directory):

ls ""        -> 0   prints f g sub
ls -R ""     -> 0   prints f g sub sub: inner
rm ""        -> 1   rm: Is a directory
rm -r ""     -> 1   rm: \sub: Invalid argument      f and g are gone
rm -rf ""    -> 1   same                            f and g are gone
rm -d ""     -> 1   rm: Directory not empty
mv f ""      -> 0
mv f g ""    -> 0
mv "" h      -> 16  mv: Device or resource busy
cat ""       -> 21
touch ""     -> 0   cwd mtime updated
mkdir ""     -> 1   mkdir: C:\...\cwd: File exists
mkdir -p ""  -> 0
cp "" out    -> 1   cp:  is a directory (not copied)
cp f ""      -> 0

With this change every line above exits 1 (mv: 2) with <cmd>: No such file or directory (mv f g "": mv: : No such file or directory) and the directory is untouched, on both platforms.

Rebase notes

Rebased onto main (faac63e). The four commits are now one commit, because main changed the same lines that each of them touched.

  • cp.rs, touch.rs, mkdir.rs: main takes the path buffers from the pool or from a spill vector. The empty-operand check runs before that code.
  • interpreter.rs: reject_empty_path sits after the new shell_lstatat.
  • mkdir.test.ts and touch.test.ts exist on main now (the long operand tests). The empty operand tests are added to those files.
  • Without the fix, on a debug build of main 4b02e10, the 17 new tests fail (mkdir 6, cp 6, touch 3, mv 1, rm 1).
  • With this head (debug build, ASAN): the six files in test/js/bun/shell/commands/ have 81 pass and 3 fail. The 3 failures are ls tests that also fail on main on this machine: two permission denied tests (the machine runs the tests as root) and recursive > node_modules (5 s timeout).

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/commands/touch.test.ts, test/js/bun/shell/commands/rm.test.ts, test/js/bun/shell/commands/mkdir.test.ts, test/js/bun/shell/commands/ls.test.ts, test/js/bun/shell/commands/cp.test.ts

@coderabbitai

coderabbitai Bot commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 12 billable files and costs up to $3.00.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 59 seconds for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: dacd05ce-80c5-4833-af69-c4666605376a

📥 Commits

Reviewing files that changed from the base of the PR and between bc7a813 and ee11892.

📒 Files selected for processing (12)
  • src/runtime/shell/builtin/cp.rs
  • src/runtime/shell/builtin/mkdir.rs
  • src/runtime/shell/builtin/mv.rs
  • src/runtime/shell/builtin/rm.rs
  • src/runtime/shell/builtin/touch.rs
  • src/runtime/shell/interpreter.rs
  • test/js/bun/shell/commands/cp.test.ts
  • test/js/bun/shell/commands/ls.test.ts
  • test/js/bun/shell/commands/mkdir.test.ts
  • test/js/bun/shell/commands/mv.test.ts
  • test/js/bun/shell/commands/rm.test.ts
  • test/js/bun/shell/commands/touch.test.ts
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review at c85c79b.

CI (build 94522): every lane that ran passed (179 of 181 jobs; the handful of test failures listed on the build are all marked flaky and passed on retry or alone, none of them in the shell). The two remaining jobs are the darwin 14 test lanes, which expired without ever getting an agent; the same two lanes are expiring or queued for hours on every build in the pipeline right now, so this is not specific to this change. The new ls/rm/mv/touch/mkdir cases were additionally run by hand on Windows x64 (fail on the released build, pass with this change), and the macOS-relevant code paths are the POSIX ones exercised by the Linux lanes.

Reproduced on Linux with bun 1.4.0 in an empty directory: touch "" exited 0 and moved the directory's mtime from 2000-01-01 to now, mkdir "" printed mkdir: <cwd>: File exists, mkdir -p "" exited 0, and with the cwd set to /tmp, rm -f "" exited 1 with rm: "/tmp" may not be removed. With BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1, cp "" out printed cp: is a directory (not copied), cp f "" exited 0 and cp -R "" out died with SIGSEGV after nesting out/out/....

Reproduced on Windows x64 with bun 1.4.0 that the fd-relative builtins have the same problem there: ls "" listed the cwd, rm -r "" deleted the files in the cwd, mv f "" exited 0, mv "" h tried to rename the cwd.

The new cases in test/js/bun/shell/commands/{touch,mkdir,cp,ls,rm,mv}.test.ts fail on the released build (touch/mkdir/cp plus the rm and mv message cases on Linux; every ls/rm/mv/touch/mkdir case on Windows) and pass with this change on both; details in the PR description.

@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:46 AM PT - Oct 2nd, 2026

❌ @robobun, your commit ee11892 has 3 failures in Build #122908 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 38002

That installs a local version of the PR into your bun-38002 executable, so you can run:

bun-38002 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — a narrow, well-tested guard that makes touch/mkdir/cp fail an empty operand with ENOENT instead of resolving it to the shell cwd.

What was reviewed:

  • The three is_empty() guards sit before the cwd join and route through each builtin's existing per-operand error path (touch/mkdir set this.err for the ShellTask trampoline; cp returns Some(ShellErr) which work_pool_callback already handles for every other early return in that function).
  • Checked that cp's error branch still posts back via enqueue_to_event_loop (no double-enqueue, no missed embedded_work_finished — same path as the other return Some(...) sites).
  • Tests cover literal vs interpolated "", -p/-v/-R, both cp operand positions, and that non-empty siblings still complete; the touch test pins cwd mtime beforehand so the old behavior would fail it. Harness conventions (tempDir/using, bunEnv spread, concurrent pipe drain) are followed.
Extended reasoning...

Overview

Three shell builtins (touch, mkdir, cp) join each relative operand onto the shell's cwd string before calling the underlying syscall. join(cwd, "") yields the cwd itself, so an empty operand silently became the current directory — touch "" bumped the cwd's mtime, mkdir -p "" succeeded, and cp -R "" out recursively copied the cwd into itself until stack overflow. This PR adds an is_empty() early return in each task's run_from_thread_pool that records ENOENT before the join, matching what utimensat("")/mkdir("")/stat("") return and what coreutils does. Two new test files and a new block in cp.test.ts cover the fix.

Security risks

None. The change strictly narrows behavior: an empty operand that previously acted on the cwd now fails with ENOENT. No new syscalls, no new trust boundaries, no user input reaching a new sink.

Level of scrutiny

Low-to-medium. The Rust change is three near-identical ~9-line early returns, each placed immediately before the offending join and using the same error-reporting machinery every other error in those functions already uses. For cp specifically I traced that returning Some(ShellErr) from run_from_thread_pool_impl goes through work_pool_callback's error branch (enqueue_to_event_loop), which is the established path for the six other return Some(...) sites in that function — so no risk of a hung task or double bounce-back. The guard is keyed on is_empty() only, so no other operand's behavior changes.

Other factors

  • The PR description enumerates the sibling builtins (ls, rm, mv, cat, cd) and explains why each already handles "" correctly (fd-relative *at() calls get ENOENT from the kernel; cd "" is a bash-compatible no-op) — the "whole bug class" was checked.
  • Tests follow harness conventions: tempDir with using, {...bunEnv, ...} spread, Promise.all([stdout, stderr, exited]), describe.concurrent for independent subprocess tests, exact stderr/exit-code assertions rather than toContain. The cp tests spawn a child with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 because that env var is read once per process and the builtin is off by default on POSIX.
  • The touch test pins the cwd mtime to a fixed past date and asserts it is unchanged, which directly detects the pre-fix behavior; the multi-operand tests verify non-empty operands still succeed while the command exits 1.
  • No CODEOWNERS entry covers these paths, no outstanding reviewer comments, and the change is already at HEAD (a444ef6).

Comment thread src/runtime/shell/builtin/rm.rs Outdated
Comment thread src/runtime/shell/builtin/rm.rs Outdated
Comment thread src/runtime/shell/interpreter.rs Outdated
Comment thread src/runtime/shell/interpreter.rs Outdated
@robobun robobun changed the title shell: fail an empty operand with ENOENT in touch, mkdir and cp shell: fail an empty operand with ENOENT instead of acting on the cwd Aug 13, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — focused fix that rejects empty operands with ENOENT before they resolve to the shell cwd, matching coreutils and the kernel.

What was reviewed:

  • Traced every shell_openat caller (cd, cat, ls, mv, rm, redirects) — the new guard only changes Windows behavior from "silently operate on cwd" to ENOENT; cd "" is unaffected because the path is joined before reaching shell_openat.
  • Confirmed E::ENOENT == 2 on Linux/macOS/Windows so the mv .exitCode(2) assertions are portable.
  • Checked that each builtin's guard routes through its existing per-operand error path, so non-empty operands in the same command are still processed (covered by tests).
  • shell_statat (used by [[ -e ]]) was flagged as a possible sibling and ruled out.
Extended reasoning...

Overview

This PR fixes a bug class where an empty operand ("") passed to shell builtins was joined onto the shell cwd and silently resolved to the cwd itself. It adds a small reject_empty_path() helper in interpreter.rs and calls it from six places: the shared shell_openat shim, plus the per-operand worker tasks in touch, mkdir, cp, mv (do_rename), and rm (remove_entry_file and the refuse-to-remove-root loop). New tests are added for touch, mkdir, cp, ls, mv, and rm covering the literal/interpolated empty word, flag variants, both operand positions, multi-operand commands, and — critically — that the unwanted side effect (touching cwd, copying cwd into itself, emptying cwd) does not happen.

Security risks

None introduced. If anything this closes a mild correctness/safety hole: on Windows the fd-relative emulation could previously turn rm -r "" into "empty the cwd" and cp -R "" out into a stack-overflowing self-copy. The guard is a pure early-return with no allocation and no user-controlled data flowing anywhere new.

Level of scrutiny

Medium. The mechanism is trivial (6-line helper + call sites), but shell_openat has many callers so I traced each one. cd "" remains a no-op because change_cwd_impl joins onto the cwd before calling shell_openat, so the argument is never empty there. Redirects (> "") and cat "" already returned ENOENT on POSIX; on Windows they now match. The mv exit-code assertions (2 /* ENOENT */) are portable — verified E::ENOENT = 2 in src/errno/{linux,darwin,windows}_errno.rs.

Other factors

  • The comment-cop bot flagged verbose comments in earlier commits; the author shortened them in cf67bfe/c85c79b and all threads are resolved.
  • Test quality is high: uses tempDir, asserts stderr/stdout before exitCode, verifies the negative contract (readdirSync unchanged, cwd mtime pinned then re-checked), uses describe.concurrent, and the cp tests correctly spawn a child with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 since that flag is read once per process.
  • The bug hunting system found no issues; a candidate about shell_statat lacking the same guard was examined and ruled out.
  • The PR description explains the mechanism, why the guard sits where it does, and confirms all 15 new tests fail on main.

Jarred-Sumner pushed a commit that referenced this pull request Aug 18, 2026
…tead of aborting (#38379)

### Problem
- `Bun.$` `mkdir <operand>` with a relative operand longer than the join
buffer, and `touch <operand>` with any operand longer than a path
buffer, abort the process: `panic: range end index 5004 out of range for
slice of length 4094` (cwd `/tmp`, 5000-byte operand; same for `mkdir
-p` and for an absolute `touch` operand).
- `mkdir` with an absolute operand that does not fit a path buffer
reports the wrong error: `mkdir: /aaa...: No such file or directory`
instead of `File name too long`.
- Cause, crash: `ShellMkdirTask::run_from_thread_pool`
(`src/runtime/shell/builtin/mkdir.rs`) joins a relative operand onto the
cwd with `resolve_path::join_z`, which writes into a fixed 4096-byte
thread-local buffer; `ShellTouchTask::run_from_thread_pool` (`touch.rs`)
joins both kinds of operand with `join_z_buf` into a stack `PathBuffer`.
Neither join bounds-checks its output (`normalize_string_generic_tz` in
`src/paths/resolve_path.rs`), and the operand comes straight from the
user.
- Cause, wrong error: mkdir hands the joined path to the node:fs mkdir
implementation as a `PathLike`. For JS callers,
`Valid::path_string_length` (`src/runtime/node/types.rs`) rejects paths
of `MAX_PATH_BYTES` or more up front; for anything longer,
`PathLike::slice_z` returns `""` and `mkdir("")` fails with ENOENT. The
shell builds the `PathLike` itself and skipped that check.

### Fix
- Both builtins join through `join_z_spill`, the existing variant that
falls back to a caller-owned `Vec` when the parts would not fit (the
same helper `fs.readdir`, `Bun.Glob` and the open `rm` fix #37521 use).
The result is the same normalized path as before for every operand that
used to work.
- `mkdir` then reports `ENAMETOOLONG` itself, naming the path, when the
path it is about to create is `MAX_PATH_BYTES` or longer, i.e. the bound
node:fs assumes. For a relative operand that is the joined, normalized
path, so a long `././.../x` spelling is still created; an absolute
operand is passed on as written (as before: normalizing it would change
what `..` means across a symlink), so it is bounded as written, which is
also what the kernel and coreutils do with such an operand. Both cases
are in the test.
- `touch` needs no check of its own: it calls `bun_sys::utimens` /
`open` with the string as is, and the OS reports `ENAMETOOLONG` for it
like for any other operand. It also no longer puts a `PathBuffer` on the
worker's stack (about 96 KB on Windows).
- Verified with `test/js/bun/shell/commands/mkdir.test.ts` and
`touch.test.ts` (the `commands/` directory has one file per builtin;
these two had none). Each runs the builtin in a child bun and covers a
5000-byte relative and absolute operand, `mkdir -p`, a 100000-byte
operand (longer than the buffer on Windows too), a long operand next to
a normal one that must still be created, and a 6000-byte `./` spelling
that must still succeed (for mkdir also the absolute form of it, which
must be refused as written; POSIX only, since on Windows it fits the
buffer).
- `USE_SYSTEM_BUN=1 bun test test/js/bun/shell/commands/mkdir.test.ts
test/js/bun/shell/commands/touch.test.ts`: both fail, the child aborts
with the panic above. Without the new mkdir check, the mkdir cases fail
on the ENOENT message instead.
- `bun bd test test/js/bun/shell/commands/mkdir.test.ts
test/js/bun/shell/commands/touch.test.ts`: pass. `bunshell.test.ts`,
`file-io.test.ts`, `commands/rm.test.ts` pass as well; `cargo check` for
the Windows and macOS targets is clean.
- Windows: the released build aborts on the 5000-byte relative `mkdir`
operand there too (the join buffer is 4096 bytes on every platform), and
creates the directory for the absolute `././.../x` spelling (the branch
this change does not touch below 98302 bytes), which is what the Windows
side of the test asserts; both checked on a Windows x64 machine. The
Windows lanes of this PR's CI runs pass both test files.
- Related open PRs: `rm` (#37521), `mv` (#37527) and `cp` (#38162) fix
the same pattern in their builtins; #38162 adds `shell_join_path`
helpers in `interpreter.rs` that touch could switch to in one line once
either PR lands. #38002 (empty operands) also creates
`commands/mkdir.test.ts` and `touch.test.ts`; whichever lands second
appends its test to the other's file. `ls -R` joins real directory
entries through the same thread-local buffer, but that needs an on-disk
tree within PATH_MAX whose entries join past 4096 bytes rather than a
long operand, and is left alone here.

### Background
- Shell builtins run in-process; `mkdir` and `touch` schedule one task
per operand on the worker pool. A task either succeeds or stores a
`bun_sys::Error`, which the builtin prints as `<cmd>: <path>: <coreutils
message>` and turns into exit code 1 once every task has finished, so
one failing operand does not stop the others.
- The shell has its own cwd (`$.cwd()`, `cd`), which can differ from the
process cwd, so these builtins make relative operands absolute by
joining them onto the shell cwd as strings before calling an
implementation that takes a path.
- `resolve_path::join_z` and `join_z_buf` concatenate and normalize
their parts (`.`, `..`, repeated separators) into a fixed buffer without
checking that the result fits; the `*_spill` variants take a `Vec` to
grow into when the unnormalized length would not fit, and otherwise
behave identically.
- `PathBuffer` is `[u8; MAX_PATH_BYTES]`: 4096 bytes on Linux, 1024 on
macOS, 98302 on Windows. The node:fs layer copies every path it receives
into one, which is why its JS entry points reject longer paths before
calling it.

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 0 · Platform-specific test(s) that do not
run on this machine. Deferring to CI, which covers all platforms:
test/js/bun/shell/commands/mkdir.test.ts
test/js/bun/shell/commands/touch.test.ts

<!-- robobun:evidence:end -->
touch, mkdir and cp resolve each operand by joining it onto the shell's
cwd before acting on it, and joining an empty component yields the cwd
itself. So `touch ""` updated the cwd's timestamps and exited 0,
`mkdir ""` reported the cwd as existing (`mkdir -p ""` exited 0), and
`cp -R "" out` copied the cwd into itself while `cp f ""` exited 0.

On Windows the shell's fd-relative emulation (shell_openat, and bun_sys's
unlinkat/renameat, whose empty NT name means the directory handle itself)
resolved an empty operand to the cwd as well: `ls ""` listed the cwd,
`rm -r ""` deleted its contents, `mv a ""` exited 0 and `mv "" b` tried to
rename the cwd. rm's refuse-to-remove-the-root check also joined "" onto
the cwd, so in a top-level directory `rm -f ""` failed on every platform.

Report ENOENT for an empty operand before resolving it, which is what
the underlying syscalls return for "", and what coreutils prints. The
check lives in one helper, reject_empty_path, applied in shell_openat,
rm's operand classification, mv's rename and the three builtins that
resolve operands as strings. The remaining operands are still processed.
@robobun
robobun force-pushed the farm/88acb2b4/shell-touch-mkdir-empty-operand branch from c85c79b to ee11892 Compare October 2, 2026 15:31

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 src/runtime/shell/interpreter.rs — On Windows the shell's stat helpers still resolve an empty operand to the cwd, so any present or future caller that stats "" before opening it gets the cwd's stat instead of ENOENT. shell_statat at src/runtime/shell/interpreter.rs:2396 and shell_lstatat at interpreter.rs:2409 go through shell_get_path, which at interpreter.rs:2382-2385 joins "" onto the directory path and returns the directory itself. The new reject_empty_path guard was added only to shell_openat at interpreter.rs:2438, not to these two siblings in the same file. Fix: call reject_empty_path at the top of shell_statat and shell_lstatat (tag stat/lstat) so every fd-relative shell helper rejects "" uniformly, rather than relying on each caller to open first or check is_empty itself.

    Why this was flagged

    Mechanism: shell_get_path at src/runtime/shell/interpreter.rs:2350-2385 treats "" as relative and returns join(dirpath, "") which is dirpath, so on Windows shell_statat (interpreter.rs:2396-2407) and shell_lstatat (interpreter.rs:2409-2420) report the cwd for an empty operand. The diff guards only shell_openat at interpreter.rs:2438. Today the two callers survive by accident: ls.rs:459 opens first and list_non_directory_operand at ls.rs:515-519 happens to re-report the open error when the bogus stat says ISDIR, and CondExpr at src/runtime/shell/states/CondExpr.rs:88-93 checks is_empty before posting the stat task with an explicit comment that Windows stat("") returns the cwd. That is the same bug class the PR claims to close at the lowest shell-owned layer, left open in two sibling helpers. Any new caller silently inherits the cwd-for-"" behavior. Remedy: add reject_empty_path to shell_statat and shell_lstatat.

    Verification: On Windows shell_get_path (src/runtime/shell/interpreter.rs:2380-2385) treats "" as relative and returns join_z_buf(&[dirpath, ""]), i.e. the cwd path, and shell_statat (2396-2397) / shell_lstatat (2410-2411) call bun_sys::stat/lstat on that result, so "" yields the cwd's stat rather than ENOENT. The diff adds reject_empty_path only to shell_openat (2438); shell_statat/shell_lstatat are untouched.

  • 🟣 src/runtime/shell/builtin/mv.rs — Users running mv "" a d (e.g. mv "$maybe" a d) get an ENOENT error and a is never moved into d, while coreutils reports the bad source and still moves a. move_multiple_into_dir at src/runtime/shell/builtin/mv.rs:672-680 stores the first error and returns, abandoning the remaining sources of the batch, and the error_signal check at mv.rs:662-668 makes every other batch stop too. The diff makes "" an always-failing source at mv.rs:489, so this now hits every script that passes a possibly-empty variable before real sources. Fix: continue past a failing source in move_multiple_into_dir (record the error, keep moving the rest, exit non-zero at the end) so a bad operand does not cancel the moves of its siblings, as the new rm "" f test already expects for rm.

    Why this was flagged

    Trigger: mv "" a d with d an existing directory, through the Bun shell on any platform. Rm's new test at test/js/bun/shell/commands/rm.test.ts:512-523 pins that the other operands are still processed when one is empty, but mv does the opposite. move_multiple_into_dir at src/runtime/shell/builtin/mv.rs:658-681 loops over sources; for "" move_in_dir -> do_rename hits reject_empty_path at mv.rs:489 and returns ENOENT, so mv.rs:679-680 set self.err and return before a is attempted, and sibling batches observe error_signal at mv.rs:662-668 and abort as well. The user gets exit 2 with mv: d: No such file or directory and a still in the cwd. On POSIX the base behaved the same because the kernel returned ENOENT for ""; on Windows the base moved a (and treated "" as the cwd), so for Windows users the diff newly cancels the move of a. Remedy: keep iterating after a failed source and report the error at the end.

    Verification: Pre-existing. Trigger: multi-source mv where any source fails (e.g. mv "" a d) with d an existing directory. move_multiple_into_dir at src/runtime/shell/builtin/mv.rs:672-681 sets self.err and returns on the first failing source; the remaining sources are never attempted. The only change to mv.rs is reject_empty_path at mv.rs:489-490; on POSIX the base already failed "" identically (libc::renameat returns ENOENT).

flags: i32,
perm: bun_sys::Mode,
) -> bun_sys::Result<Fd> {
reject_empty_path(path.as_bytes(), bun_sys::Tag::open)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): the cat builtin's empty-operand behavior that this guard changes is not pinned by any test. The PR lists cat "" (Windows: exit 21, silent) as fixed through shell_openat, but the new tests only cover ls, rm, mv, cp, mkdir and touch; cat's error path at src/runtime/shell/builtin/cat.rs:200 (task_error_to_string + write_failing_error(..., 1)) is never exercised with "". Fix: add a cat "" case (child bun with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 like the cp tests, or under the existing cat builtin suite) asserting stderr cat: No such file or directory\n, exit 1, and that a following non-empty operand is still printed.

Why this was flagged

src/runtime/shell/interpreter.rs:2438 adds reject_empty_path to shell_openat, which is the open cat uses at src/runtime/shell/builtin/cat.rs:200. The PR description names cat "" as one of the fixed commands (on Windows it exited 21 silently). No test in the diff runs cat with an empty operand; test/js/bun/shell/commands/ has no cat.test.ts and bunshell.test.ts is untouched. A later change to cat's error formatting or to the Windows open emulation could regress cat "" without any test failing. Repository review rules require every sibling entry point receiving the same fix to ship a test.

Verification: nit. Triggering condition: any future change to the Windows shell_openat emulation or cat's error path regresses cat "" silently. cat opens every operand through shell_openat at src/runtime/shell/builtin/cat.rs:200. The diff stat shows test changes only in cp/ls/mkdir/mv/rm/touch test files; a grep of test/js/bun/shell/ for cat "" finds no match.

Comment on lines +184 to +186
// Joined below, `""` would resolve to the cwd itself.
if path.is_empty() {
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: Users running rm -r . or rm -r .. in Bun's shell still have the cwd (or its parent) emptied on Linux, while coreutils refuses both. The new empty-operand skip at rm.rs:184-186 fixes only the "" spelling of operand-names-the-cwd; the root check joins ./.. onto the cwd, normalizes them away, and lets them reach the worker, where unlinkat/openat act on the directory itself. Fix: the root check must reject every operand that resolves to the cwd or an ancestor by name — "", ., .., and ./-style spellings — with "refusing to remove '.' or '..' directory" like coreutils, rather than special-casing "" alone.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Trigger: $\rm -rf .`orrm -rf ..via Bun's shell, reaching Rm::next at src/runtime/shell/builtin/rm.rs:182-204. The loop now skips "" (rm.rs:184-186) but for.it joins cwd+"." (rm.rs:190-193), normalize_string_buf collapses it back to the cwd, dirname is non-empty unless the cwd is top-level, so the check passes and the worker opens.and recursively unlinks every child (rm.rs:1019-1085). The existing test at test/js/bun/shell/commands/rm.test.ts:400-419 documents that on Linuxrm -rf .deletes file.txt and sub. The PR frames its bug as an operand that names the cwd; REVIEW.md asks for the whole input class (empty, lone.) in the same PR, and ./..are the sibling inputs with the identical consequence (cwd contents destroyed). Remedy: in the root-check loop, refuse any operand whose last component is.or..` (and the empty one) before scheduling.

Verification: pre-existing. A user runs rm -r . (or ..) in Bun's shell on Linux. The root check at src/runtime/shell/builtin/rm.rs:182-208 only skips ""; for . it joins onto the cwd and normalize_string_buf collapses it back to /cwd, so the operand reaches the worker, where dir_iterator::iterate(fd) (1048) unlinks every entry. The base commit has identical handling, so merging makes nothing worse.

Comment on lines +1228 to 1233
match reject_empty_path(path.as_bytes(), bun_sys::Tag::unlink)
.and_then(|()| bun_sys::unlinkat_with_flags(dirfd, path, 0))
{
Ok(()) => self.verbose_deleted(parent_dir_task, path.as_bytes()),
Err(e) => match e.get_errno() {
E::ENOENT => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: Scripts running rm -fv "$x" with an unset $x get a bare empty line on stdout as if something were deleted, and exit 0. At src/runtime/shell/builtin/rm.rs:1228 reject_empty_path turns "" into ENOENT, and the ENOENT arm at rm.rs:1235 calls verbose_deleted with the empty name when -f is set, which appends "" plus a newline to the deleted list. coreutils prints nothing for a missing operand under -fv. Fix: under -f, skip verbose_deleted for ENOENT (nothing was removed) so no operand, empty or missing, is reported as deleted; at minimum never emit a blank line for "". The new rm tests cover -f and -rf but not -v, so this variant is unpinned.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Trigger: rm -fv "" (commonly rm -fv "$unset_var") through the Bun shell. remove_entry_file at src/runtime/shell/builtin/rm.rs:1228 now returns ENOENT for the empty operand; the ENOENT arm at rm.rs:1233-1235 sees opts.force and returns self.verbose_deleted(parent_dir_task, ""), and verbose_deleted at rm.rs:852-872 pushes the empty slice and a newline into deleted_entries, which is flushed to stdout. The user sees a lone "\n" on stdout with exit 0, i.e. a deletion report for nothing. On the base branch this population did not reach that arm: on Windows the *at() emulation acted on the cwd and errored, and in a top-level cwd on every platform the root check at rm.rs:183 refused with exit 1; the diff routes both into the -f/-v report path. The new tests at test/js/bun/shell/commands/rm.test.ts:496-500 cover -f and -rf without -v, so the blank line is neither pinned nor excluded. Remedy: do not call verbose_deleted on the ENOENT+force path (coreutils reports nothing), or at least not for an empty operand.

Verification: Pre-existing: on POSIX the base already prints the same lone newline by the same route. Trigger: rm -fv "" in a non-top-level cwd. In src/runtime/shell/builtin/rm.rs the E::ENOENT arm at 1233-1236 calls verbose_deleted under force, so stdout gets "\n" and the exit code is 0. The base commit's remove_entry_file has the identical arm, and unlinkat(cwd, "") already returns ENOENT.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant