-
Notifications
You must be signed in to change notification settings - Fork 5.1k
shell: fail an empty operand with ENOENT instead of acting on the cwd #38002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ use bun_sys::{E, FdExt, dir_iterator}; | |
| use crate::shell::ExitCode; | ||
| use crate::shell::builtin::{Builtin, IoKind, Kind}; | ||
| use crate::shell::interpreter::{ | ||
| EventLoopHandle, Interpreter, NodeId, ShellTask, WorkPoolTask, shell_openat, | ||
| EventLoopHandle, Interpreter, NodeId, ShellTask, WorkPoolTask, reject_empty_path, shell_openat, | ||
| }; | ||
| use crate::shell::io_writer::{ChildPtr, WriterTag}; | ||
| use crate::shell::yield_::Yield; | ||
|
|
@@ -181,6 +181,10 @@ impl Rm { | |
|
|
||
| for i in args_start..argc { | ||
| let path = Builtin::of(interp, cmd).arg_bytes(i); | ||
| // Joined below, `""` would resolve to the cwd itself. | ||
| if path.is_empty() { | ||
| continue; | ||
| } | ||
| let resolved: &[u8] = if Platform::AUTO.is_absolute(path) { | ||
| path | ||
| } else { | ||
|
|
@@ -1221,7 +1225,9 @@ impl ShellRmTask { | |
| vtable: &mut V, | ||
| ) -> bun_sys::Maybe<()> { | ||
| let dirfd = self.cwd; | ||
| match bun_sys::unlinkat_with_flags(dirfd, path, 0) { | ||
| 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 => { | ||
|
Comment on lines
+1228
to
1233
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 pre-existing, not blocking: Scripts running Why this was flaggedTrigger: Verification: Pre-existing: on POSIX the base already prints the same lone newline by the same route. Trigger: |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2416,6 +2416,14 @@ pub(crate) fn shell_lstatat(dir: Fd, path_: &bun_core::ZStr) -> bun_sys::Result< | |
| } | ||
| } | ||
|
|
||
| /// Resolved by the shell (cwd join, or the Windows `*at()` emulation), `""` would name the cwd. | ||
| pub(crate) fn reject_empty_path(path: &[u8], syscall: bun_sys::Tag) -> bun_sys::Result<()> { | ||
| if path.is_empty() { | ||
| return Err(bun_sys::Error::from_code(bun_sys::E::ENOENT, syscall)); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// POSIX: `bun_sys::openat` with the error tagged `.with_path(path)`. | ||
| /// Windows: for `O_DIRECTORY` opens, rewrite POSIX-absolute paths via | ||
| /// `shell_get_path` and use `openDirAtWindowsA(.iterable=true)` + | ||
|
|
@@ -2427,6 +2435,7 @@ pub(crate) fn shell_openat( | |
| flags: i32, | ||
| perm: bun_sys::Mode, | ||
| ) -> bun_sys::Result<Fd> { | ||
| reject_empty_path(path.as_bytes(), bun_sys::Tag::open)?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): the Why this was flaggedsrc/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 Verification: nit. Triggering condition: any future change to the Windows |
||
| #[cfg(windows)] | ||
| { | ||
| use bun_sys::FdExt; | ||
|
|
||
There was a problem hiding this comment.
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 .orrm -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 andnormalize_string_bufcollapses it back to/cwd, so the operand reaches the worker, wheredir_iterator::iterate(fd)(1048) unlinks every entry. The base commit has identical handling, so merging makes nothing worse.