diff --git a/src/runtime/shell/builtin/rm.rs b/src/runtime/shell/builtin/rm.rs index 9adb735274e3..43c976e8196e 100644 --- a/src/runtime/shell/builtin/rm.rs +++ b/src/runtime/shell/builtin/rm.rs @@ -30,7 +30,6 @@ pub enum RmState { Done { exit_code: ExitCode, }, - WaitingWriteErr, Err(ExitCode), } @@ -38,6 +37,8 @@ pub struct ExecState { /// Index into argv where filepath args start. pub args_start: usize, pub total_tasks: usize, + /// An operand was refused up front; folded into [`exit_code`]. + pub had_refusal: bool, pub err: Option, pub error_signal: AtomicBool, pub output_done: AtomicUsize, @@ -51,6 +52,15 @@ impl ExecState { fn output_drained(&self) -> bool { self.output_done.load(Ordering::SeqCst) >= self.output_count.load(Ordering::SeqCst) } + + #[inline] + fn exit_code(&self) -> ExitCode { + if self.err.is_some() || self.had_refusal { + 1 + } else { + 0 + } + } } #[derive(Clone, Copy)] @@ -111,7 +121,6 @@ impl Rm { Exec, Done(ExitCode), Err(ExitCode), - WaitErr, } let tag = match &Self::state_mut(interp, cmd).state { RmState::Idle => Tag::Idle, @@ -122,10 +131,8 @@ impl Rm { RmState::Exec(_) => Tag::Exec, RmState::Done { exit_code } => Tag::Done(*exit_code), RmState::Err(c) => Tag::Err(*c), - RmState::WaitingWriteErr => Tag::WaitErr, }; match tag { - Tag::WaitErr => return Yield::suspended(), Tag::Idle => { Self::state_mut(interp, cmd).state = RmState::ParseOpts { idx: 0, @@ -171,87 +178,10 @@ impl Rm { } let args_start = idx as usize; - - // Check that none of the paths will delete the root. - { - let mut buf = bun_paths::PathBuffer::uninit(); - let cwd = match bun_sys::getcwd_z(&mut buf) { - Ok(c) => c.as_bytes().to_vec(), - Err(err) => { - let msg = - err.msg().map(bstr::BStr::new).unwrap_or_else(|| { - bstr::BStr::new(b"failed to get cwd") - }); - let buf = Builtin::fmt_error_arena( - interp, - cmd, - Some(Kind::Rm), - format_args!("getcwd: {}", msg), - ) - .to_vec(); - return Self::write_failing_error(interp, cmd, &buf, 1); - } - }; - - for i in args_start..argc { - let path = Builtin::of(interp, cmd).arg_bytes(i); - let resolved: &[u8] = if Platform::AUTO.is_absolute(path) { - path - } else { - resolve_path::join::(&[&cwd, path]) - }; - let normalized = resolve_path::normalize_string::< - false, - platform::Auto, - >(resolved); - let dirname = - resolve_path::dirname::(normalized); - if dirname.is_empty() { - // Copy resolved before - // re-borrowing `interp` mutably. - let resolved_owned = resolved.to_vec(); - if let Some(safeguard) = - Builtin::of(interp, cmd).stderr.needs_io() - { - Self::state_mut(interp, cmd).state = - RmState::ParseOpts { - idx, - wait_write_err: true, - }; - let child = ChildPtr::new(cmd, WriterTag::Builtin); - return Builtin::of_mut(interp, cmd) - .stderr - .enqueue_fmt( - child, - Some(Kind::Rm), - format_args!( - "\"{}\" may not be removed\n", - bstr::BStr::new(&resolved_owned) - ), - safeguard, - ); - } - let buf = Builtin::fmt_error_arena( - interp, - cmd, - Some(Kind::Rm), - format_args!( - "\"{}\" may not be removed\n", - bstr::BStr::new(&resolved_owned) - ), - ) - .to_vec(); - let _ = - Builtin::write_no_io(interp, cmd, IoKind::Stderr, &buf); - return Builtin::done(interp, cmd, 1); - } - } - } - - let total_tasks = argc - args_start; Self::state_mut(interp, cmd).state = RmState::Exec(ExecState { args_start, - total_tasks, + total_tasks: argc - args_start, + had_refusal: false, err: None, error_signal: AtomicBool::new(false), output_done: AtomicUsize::new(0), @@ -304,7 +234,8 @@ impl Rm { _ => unreachable!(), }; if !started { - let cwd = Builtin::cwd(interp, cmd); + let cwd_fd = Builtin::cwd(interp, cmd); + let cwd_path = Builtin::shell(interp, cmd).cwd().to_vec(); let evtloop = Builtin::event_loop(interp, cmd); let opts = Self::state_mut(interp, cmd).opts; let interp_ptr: *mut Interpreter = interp.as_ctx_ptr(); @@ -316,6 +247,49 @@ impl Rm { e.started = true; (e.args_start, e.args_start + e.total_tasks) }; + + let mut refused_msg = Vec::::new(); + let mut args_to_run = Vec::::with_capacity(argc - args_start); + for i in args_start..argc { + use std::io::Write as _; + let path = Builtin::of(interp, cmd).arg_bytes(i); + let base = resolve_path::basename(path); + if base == b"." || base == b".." { + let _ = writeln!( + &mut refused_msg, + "rm: refusing to remove '.' or '..' directory: skipping '{}'", + bstr::BStr::new(path), + ); + continue; + } + let resolved: &[u8] = if Platform::AUTO.is_absolute(path) { + path + } else { + resolve_path::join::(&[&cwd_path, path]) + }; + let normalized = + resolve_path::normalize_string::(resolved); + let dirname = resolve_path::dirname::(normalized); + if dirname.is_empty() { + let _ = writeln!( + &mut refused_msg, + "rm: \"{}\" may not be removed", + bstr::BStr::new(resolved), + ); + continue; + } + args_to_run.push(i); + } + + let total_tasks = args_to_run.len(); + let had_refusal = !refused_msg.is_empty(); + { + let RmState::Exec(e) = &mut Self::state_mut(interp, cmd).state else { + unreachable!() + }; + e.total_tasks = total_tasks; + e.had_refusal = had_refusal; + } let (sig, out_count) = match &Self::state_mut(interp, cmd).state { RmState::Exec(e) => ( bun_ptr::BackRef::new(&e.error_signal), @@ -323,14 +297,34 @@ impl Rm { ), _ => unreachable!(), }; - for i in args_start..argc { + for i in args_to_run { let root = Builtin::of(interp, cmd).arg_bytes(i); let task = ShellRmTask::create( - cmd, opts, root, cwd, sig, out_count, evtloop, interp_ptr, + cmd, opts, root, cwd_fd, sig, out_count, evtloop, interp_ptr, ); // SAFETY: freshly heap-allocated. unsafe { ShellRmTask::schedule(task) }; } + if !refused_msg.is_empty() { + if let Some(safeguard) = Builtin::of(interp, cmd).stderr.needs_io() { + if let RmState::Exec(e) = &mut Self::state_mut(interp, cmd).state { + e.output_count.fetch_add(1, Ordering::SeqCst); + } + let child = ChildPtr::new(cmd, WriterTag::Builtin); + return Builtin::of_mut(interp, cmd).stderr.enqueue( + child, + &refused_msg, + safeguard, + ); + } + let _ = Builtin::write_no_io(interp, cmd, IoKind::Stderr, &refused_msg); + } + if total_tasks == 0 { + Self::state_mut(interp, cmd).state = RmState::Done { + exit_code: if had_refusal { 1 } else { 0 }, + }; + continue; + } } return Yield::suspended(); } @@ -355,23 +349,6 @@ impl Rm { Builtin::done(interp, cmd, 1) } - fn write_failing_error( - interp: &Interpreter, - cmd: NodeId, - buf: &[u8], - exit_code: ExitCode, - ) -> Yield { - if let Some(safeguard) = Builtin::of(interp, cmd).stderr.needs_io() { - Self::state_mut(interp, cmd).state = RmState::WaitingWriteErr; - let child = ChildPtr::new(cmd, WriterTag::Builtin); - return Builtin::of_mut(interp, cmd) - .stderr - .enqueue(child, buf, safeguard); - } - let _ = Builtin::write_no_io(interp, cmd, IoKind::Stderr, buf); - Builtin::done(interp, cmd, exit_code) - } - pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, @@ -382,7 +359,7 @@ impl Rm { RmState::Exec(exec) => { exec.output_done.fetch_add(1, Ordering::SeqCst); if exec.tasks_done >= exec.total_tasks && exec.output_drained() { - Some(if exec.err.is_some() { 1 } else { 0 }) + Some(exec.exit_code()) } else { None } @@ -456,13 +433,7 @@ impl Rm { }; if tasks_done >= total && all_out { let code = match &Self::state_mut(interp, cmd).state { - RmState::Exec(exec) => { - if exec.err.is_some() { - 1 - } else { - 0 - } - } + RmState::Exec(exec) => exec.exit_code(), _ => 0, }; Self::state_mut(interp, cmd).state = RmState::Done { exit_code: code }; @@ -511,13 +482,7 @@ impl Rm { }; if done { let code = match &Self::state_mut(interp, cmd).state { - RmState::Exec(exec) => { - if exec.err.is_some() { - 1 - } else { - 0 - } - } + RmState::Exec(exec) => exec.exit_code(), _ => 0, }; return Builtin::done(interp, cmd, code); diff --git a/test/js/bun/shell/commands/rm.test.ts b/test/js/bun/shell/commands/rm.test.ts index 3cc7e9d3da6c..3babfcb94ece 100644 --- a/test/js/bun/shell/commands/rm.test.ts +++ b/test/js/bun/shell/commands/rm.test.ts @@ -6,8 +6,8 @@ */ import { $ } from "bun"; import { beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; -import { existsSync, mkdirSync, renameSync, symlinkSync, writeFileSync } from "node:fs"; +import { bunEnv, bunExe, isWindows, tempDir } from "harness"; +import { existsSync, mkdirSync, readdirSync, renameSync, symlinkSync, writeFileSync } from "node:fs"; import path from "path"; import { createTestBuilder, sortedShellOutput } from "../util"; const TestBuilder = createTestBuilder(import.meta.path); @@ -212,6 +212,102 @@ foo/ }, 120_000); }); +// POSIX: an operand whose last path component is `.` or `..` must be refused +// with a diagnostic and not processed (GNU rm: "refusing to remove '.' or +// '..' directory"). Previously these were recursed into, so the directory was +// emptied before the final rmdir failed with EINVAL. +describe.concurrent("rm refuses '.' and '..' operands", () => { + const ops = [".", "..", "./", "../", "d/.", "d/..", "d/./"] as const; + test.each(ops)("rm -rf %s", async op => { + using dir = tempDir("rm-dot", { "d/a.txt": "A", "d/s/c.txt": "C" }); + const r = await $`cd d && rm -rf ${op}`.cwd(String(dir)).nothrow().quiet(); + expect({ + stderr: r.stderr.toString(), + exitCode: r.exitCode, + remaining: readdirSync(path.join(String(dir), "d")).sort(), + }).toEqual({ + stderr: `rm: refusing to remove '.' or '..' directory: skipping '${op}'\n`, + exitCode: 1, + remaining: ["a.txt", "s"], + }); + }); + + test("skips the refused operand and still removes the others", async () => { + using dir = tempDir("rm-dot-mixed", { "keep.txt": "K", "go.txt": "G" }); + const r = await $`rm -rf . go.txt`.cwd(String(dir)).nothrow().quiet(); + expect({ + stderr: r.stderr.toString(), + exitCode: r.exitCode, + remaining: readdirSync(String(dir)).sort(), + }).toEqual({ + stderr: "rm: refusing to remove '.' or '..' directory: skipping '.'\n", + exitCode: 1, + remaining: ["keep.txt"], + }); + }); + + test("does not refuse dotfiles", async () => { + using dir = tempDir("rm-dot-ok", { ".hidden": "H", ".h2/x.txt": "X" }); + const r = await $`rm -rf .hidden .h2`.cwd(String(dir)).nothrow().quiet(); + expect({ stderr: r.stderr.toString(), exitCode: r.exitCode, remaining: readdirSync(String(dir)) }).toEqual({ + stderr: "", + exitCode: 0, + remaining: [], + }); + }); + + // Win32 path normalization strips trailing periods from path components, so a + // directory literally named `...` does not round-trip through the harness. + test.skipIf(isWindows)("does not refuse '...'", async () => { + using dir = tempDir("rm-dots-ok", { ".../x.txt": "X" }); + const r = await $`rm -rf ...`.cwd(String(dir)).nothrow().quiet(); + expect({ stderr: r.stderr.toString(), exitCode: r.exitCode, remaining: readdirSync(String(dir)) }).toEqual({ + stderr: "", + exitCode: 0, + remaining: [], + }); + }); + + test.skipIf(isWindows)("still refuses '/'", async () => { + using dir = tempDir("rm-root", {}); + const r = await $`rm -rf /`.cwd(String(dir)).nothrow().quiet(); + expect({ exitCode: r.exitCode, stderr: r.stderr.toString() }).toEqual({ + exitCode: 1, + stderr: 'rm: "/" may not be removed\n', + }); + }); +}); + +// The preserve-root guard resolves relative operands against the shell +// instance's cwd (set via `$.cwd()` / `cd`), not the host process cwd. +// With the process launched from `/`, a relative operand used to resolve to a +// top-level path and be spuriously refused. +test.skipIf(isWindows)("rm preserve-root guard resolves against the shell cwd", async () => { + using dir = tempDir("rm-guard-cwd", { "sub/a.txt": "A" }); + const fixture = ` + import { $ } from "bun"; + $.nothrow(); + const r = await $\`rm -rf sub\`.cwd(${JSON.stringify(String(dir))}).quiet(); + console.log(JSON.stringify({ cwd: process.cwd(), exitCode: r.exitCode, stderr: r.stderr.toString() })); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + cwd: "/", + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect({ ...JSON.parse(stdout.trim()), subExists: existsSync(path.join(String(dir), "sub")) }).toEqual({ + cwd: "/", + exitCode: 0, + stderr: "", + subExists: false, + }); + expect(exitCode).toBe(0); +}); + function packagejson() { return `{ "name": "dummy",