From 8d0a8390767d5ca28d0a419919c6ddbc43e1bf21 Mon Sep 17 00:00:00 2001 From: shskwmt Date: Sat, 6 Dec 2025 19:36:26 +0900 Subject: [PATCH 1/5] feat(rm): Implement --one-file-system and --preserved-root=all This comment enhances the safety of recursive deletion in `rm` by introducing two key features to prevent accidental data loss. `--one-file-system`: When specified, `rm` will not traverse into directories that are on a different file system from the one on which the traversal began, this prevents `rm -r` from accidentally deleting data on mounted volumes. `--preserve-root=all`: The `--preserve-root` option is enhanced to accept an optional argument, `all`. When set to `all`, `rm` will refuse to remove any directory that is a mount point. The default behavior (`--preserve-root` without an argument) remains, protecting only the root directory (`/`). --- src/uu/df/src/filesystem.rs | 14 +-- src/uu/rm/Cargo.toml | 9 +- src/uu/rm/src/rm.rs | 122 +++++++++++++++++++++-- src/uucore/src/lib/features/fsext.rs | 8 +- tests/by-util/test_rm.rs | 138 +++++++++++++++++++++++++++ util/build-gnu.sh | 5 + 6 files changed, 276 insertions(+), 20 deletions(-) diff --git a/src/uu/df/src/filesystem.rs b/src/uu/df/src/filesystem.rs index 25743941db0..fd6b6420309 100644 --- a/src/uu/df/src/filesystem.rs +++ b/src/uu/df/src/filesystem.rs @@ -121,19 +121,15 @@ where impl Filesystem { // TODO: resolve uuid in `mount_info.dev_name` if exists pub(crate) fn new(mount_info: MountInfo, file: Option) -> Option { + #[cfg(unix)] let _stat_path = if mount_info.mount_dir.is_empty() { - #[cfg(unix)] - { - mount_info.dev_name.clone().into() - } - #[cfg(windows)] - { - // On windows, we expect the volume id - mount_info.dev_id.clone().into() - } + mount_info.dev_name.clone().into() } else { mount_info.mount_dir.clone() }; + #[cfg(windows)] + let _stat_path = mount_info.dev_id.clone(); // On windows, we expect the volume id + #[cfg(unix)] let usage = FsUsage::new(statfs(&_stat_path).ok()?); #[cfg(windows)] diff --git a/src/uu/rm/Cargo.toml b/src/uu/rm/Cargo.toml index ccf1bf93e28..7f0198e9730 100644 --- a/src/uu/rm/Cargo.toml +++ b/src/uu/rm/Cargo.toml @@ -18,11 +18,16 @@ workspace = true path = "src/rm.rs" [dependencies] -thiserror = { workspace = true } clap = { workspace = true } -uucore = { workspace = true, features = ["fs", "parser", "safe-traversal"] } fluent = { workspace = true } indicatif = { workspace = true } +thiserror = { workspace = true } +uucore = { workspace = true, features = [ + "fs", + "fsext", + "parser", + "safe-traversal", +] } [target.'cfg(unix)'.dependencies] libc = { workspace = true } diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index a20a57d7f36..83d3010211b 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -21,6 +21,7 @@ use std::path::{Path, PathBuf}; use thiserror::Error; use uucore::display::Quotable; use uucore::error::{FromIo, UError, UResult}; +use uucore::fsext::{MountInfo, read_fs_list}; use uucore::parser::shortcut_value_parser::ShortcutValueParser; use uucore::translate; use uucore::{format_usage, os_str_as_bytes, prompt_yes, show_error}; @@ -126,6 +127,13 @@ impl From<&str> for InteractiveMode { } } +#[derive(PartialEq)] +pub enum PreserveRoot { + Default, + YesAll, + No, +} + /// Options for the `rm` command /// /// All options are public so that the options can be programmatically @@ -152,7 +160,7 @@ pub struct Options { /// `--one-file-system` pub one_fs: bool, /// `--preserve-root`/`--no-preserve-root` - pub preserve_root: bool, + pub preserve_root: PreserveRoot, /// `-r`, `--recursive` pub recursive: bool, /// `-d`, `--dir` @@ -173,7 +181,7 @@ impl Default for Options { force: false, interactive: InteractiveMode::PromptProtected, one_fs: false, - preserve_root: true, + preserve_root: PreserveRoot::Default, recursive: false, dir: false, verbose: false, @@ -242,7 +250,18 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } }, one_fs: matches.get_flag(OPT_ONE_FILE_SYSTEM), - preserve_root: !matches.get_flag(OPT_NO_PRESERVE_ROOT), + preserve_root: if matches.get_flag(OPT_NO_PRESERVE_ROOT) { + PreserveRoot::No + } else { + match matches + .get_one::(OPT_PRESERVE_ROOT) + .unwrap() + .as_str() + { + "all" => PreserveRoot::YesAll, + _ => PreserveRoot::Default, + } + }, recursive: matches.get_flag(OPT_RECURSIVE), dir: matches.get_flag(OPT_DIR), verbose: matches.get_flag(OPT_VERBOSE), @@ -341,7 +360,10 @@ pub fn uu_app() -> Command { Arg::new(OPT_PRESERVE_ROOT) .long(OPT_PRESERVE_ROOT) .help(translate!("rm-help-preserve-root")) - .action(ArgAction::SetTrue), + .value_parser(["all"]) + .default_value("all") + .default_missing_value("all") + .hide_default_value(true), ) .arg( Arg::new(OPT_RECURSIVE) @@ -460,7 +482,6 @@ fn count_files_in_directory(p: &Path) -> u64 { 1 + entries_count } -// TODO: implement one-file-system (this may get partially implemented in walkdir) /// Remove (or unlink) the given files /// /// Returns true if it has encountered an error. @@ -596,7 +617,19 @@ fn remove_dir_recursive( return remove_file(path, options, progress_bar); } - // Base case 2: this is a non-empty directory, but the user + // Base case 2: check if a path is on the same file system + if let Err(additional_reason) = check_one_fs(path, options) { + if !additional_reason.is_empty() { + show_error!("{}", additional_reason); + } + show_error!( + "skipping {}, since it's on a different device", + path.quote() + ); + return true; + } + + // Base case 3: this is a non-empty directory, but the user // doesn't want to descend into it. if options.interactive == InteractiveMode::Always && !is_dir_empty(path) @@ -684,9 +717,82 @@ fn remove_dir_recursive( } } +/// Return a reference to the best matching `MountInfo` whose `mount_dir` +/// is a prefix of the canonicalized `path`. +fn mount_for_path<'a>(path: &Path, mounts: &'a [MountInfo]) -> Option<&'a MountInfo> { + let canonical = path.canonicalize().ok()?; + let mut best: Option<(&MountInfo, usize)> = None; + + // Each `MountInfo` has a `mount_dir` that we compare. + for mi in mounts { + if mi.mount_dir.is_empty() { + continue; + } + let mount_dir = PathBuf::from(&mi.mount_dir); + if canonical.starts_with(&mount_dir) { + let len = mount_dir.as_os_str().len(); + // Pick the mount with the longest matching prefix. + if best.is_none() || len > best.as_ref().unwrap().1 { + best = Some((mi, len)); + } + } + } + + best.map(|(mi, _len)| mi) +} + +/// Check if a path is on the same file system when `--one-file-system` or `--preserve-root=all` options are enabled. +/// Return `OK(())` if the path is on the same file system, +/// or an additional error describing why it should be skipped. +fn check_one_fs(path: &Path, options: &Options) -> Result<(), String> { + // If neither `--one-file-system` nor `--preserve-root=all` is active, + // always proceed + if !options.one_fs && options.preserve_root != PreserveRoot::YesAll { + return Ok(()); + } + + // Read mount information + let fs_list = read_fs_list().map_err(|err| format!("cannot read mount info: {err}"))?; + + // Canonicalize the path + let child_canon = path + .canonicalize() + .map_err(|err| format!("cannot canonicalize {}: {err}", path.quote()))?; + + // Get parent path, handling root case + let parent_canon = child_canon.parent().ok_or("")?.to_path_buf(); + + // Find mount points for child and parent + let child_mount = mount_for_path(&child_canon, &fs_list).ok_or("")?; + let parent_mount = mount_for_path(&parent_canon, &fs_list).ok_or("")?; + + // Check if child and parent are on the same device + if child_mount.dev_id != parent_mount.dev_id { + return Err(String::new()); + } + + Ok(()) +} + fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { let mut had_err = false; + if let Err(additional_reason) = check_one_fs(path, options) { + if !additional_reason.is_empty() { + show_error!("{}", additional_reason); + } + show_error!( + "skipping {}, since it's on a different device", + path.quote() + ); + + if options.preserve_root == PreserveRoot::YesAll { + show_error!("and --preserve-root=all is in effect"); + } + + return true; + } + let path = clean_trailing_slashes(path); if path_is_current_or_parent_directory(path) { show_error!( @@ -697,9 +803,9 @@ fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar> } let is_root = path.has_root() && path.parent().is_none(); - if options.recursive && (!is_root || !options.preserve_root) { + if options.recursive && (!is_root || options.preserve_root == PreserveRoot::No) { had_err = remove_dir_recursive(path, options, progress_bar); - } else if options.dir && (!is_root || !options.preserve_root) { + } else if options.dir && (!is_root || options.preserve_root == PreserveRoot::No) { had_err = remove_dir(path, options, progress_bar).bitor(had_err); } else if options.recursive { show_error!("{}", RmError::DangerousRecursiveOperation); diff --git a/src/uucore/src/lib/features/fsext.rs b/src/uucore/src/lib/features/fsext.rs index 78dfcceb23d..c8391a08a10 100644 --- a/src/uucore/src/lib/features/fsext.rs +++ b/src/uucore/src/lib/features/fsext.rs @@ -324,12 +324,18 @@ impl MountInfo { let mount_root = to_nul_terminated_wide_string(&mount_root); GetDriveTypeW(mount_root.as_ptr()) }; + + let mount_dir = Path::new(&mount_root) + .canonicalize() + .unwrap_or_default() + .into_os_string(); + Some(Self { dev_id: volume_name, dev_name, fs_type: fs_type.unwrap_or_default(), mount_root: mount_root.into(), // TODO: We should figure out how to keep an OsString here. - mount_dir: OsString::new(), + mount_dir, mount_option: String::new(), remote, dummy: false, diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 38230f2ad36..6c0fb69b881 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1150,6 +1150,144 @@ fn test_rm_directory_not_writable() { assert!(!at.dir_exists("b/d")); // Should be removed } +#[test] +#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd"))] +fn test_rm_one_file_system() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // Test must be run as root (or with `sudo -E`) + if scene.cmd("whoami").run().stdout_str() != "root\n" { + println!("Skipping test_rm_one_file_system: must be run as root"); + return; + } + + // Define paths for temporary files and directories + let img_path = "fs.img"; + let mount_point = "fs"; + let remove_dir = "a"; + let bind_mount_point = "a/b"; + + at.touch(img_path); + + // Create filesystem image + scene + .cmd("dd") + .args(&[ + "if=/dev/zero", + &format!("of={img_path}"), + "bs=1M", + "count=50", + ]) + .succeeds(); + + // Create ext4 filesystem + scene.cmd("/sbin/mkfs.ext4").arg(img_path).succeeds(); + + // Prepare directory structure + at.mkdir_all(mount_point); + at.mkdir_all(bind_mount_point); + + // Mount as loop device + scene + .cmd("mount") + .args(&["-o", "loop", img_path, mount_point]) + .succeeds(); + + // Create test directory + at.mkdir_all(&format!("{mount_point}/x")); + + // Create bind mount + scene + .cmd("mount") + .args(&["--bind", mount_point, bind_mount_point]) + .succeeds(); + + // Run the test + scene + .ucmd() + .args(&["--one-file-system", "-rf", remove_dir]) + .fails() + .stderr_contains(format!("rm: skipping '{bind_mount_point}'")); + + // Cleanup + let _ = scene.cmd("umount").arg(bind_mount_point).run(); + let _ = scene.cmd("umount").arg(mount_point).run(); + let _ = scene + .cmd("rm") + .args(&["-rf", mount_point, bind_mount_point]) + .run(); +} + +#[test] +#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd"))] +fn test_rm_preserve_root() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // Test must be run as root (or with `sudo -E`) + if scene.cmd("whoami").run().stdout_str() != "root\n" { + println!("Skipping test_rm_one_file_system: must be run as root"); + return; + } + + // Define paths for temporary files and directories + let img_path = "fs.img"; + let mount_point = "fs"; + let bind_mount_point = "a/b"; + + at.touch(img_path); + + // Create filesystem image + scene + .cmd("dd") + .args(&[ + "if=/dev/zero", + &format!("of={img_path}"), + "bs=1M", + "count=50", + ]) + .succeeds(); + + // Create ext4 filesystem + scene.cmd("/sbin/mkfs.ext4").arg(img_path).succeeds(); + + // Prepare directory structure + at.mkdir_all(mount_point); + at.mkdir_all(bind_mount_point); + + // Mount as loop device + scene + .cmd("mount") + .args(&["-o", "loop", img_path, mount_point]) + .succeeds(); + + // Create test directory + at.mkdir_all(&format!("{mount_point}/x")); + + // Create bind mount + scene + .cmd("mount") + .args(&["--bind", mount_point, bind_mount_point]) + .succeeds(); + + // Run the test + scene + .ucmd() + .args(&["--preserve-root=all", "-rf", bind_mount_point]) + .fails() + .stderr_contains(format!("rm: skipping '{bind_mount_point}'")) + .stderr_contains("rm: and --preserve-root=all is in effect"); + + // Cleanup + let _ = scene.cmd("umount").arg(bind_mount_point).run(); + let _ = scene.cmd("umount").arg(mount_point).run(); + let _ = scene + .cmd("rm") + .args(&["-rf", mount_point, bind_mount_point]) + .run(); +} + #[test] fn test_progress_flag_short() { let (at, mut ucmd) = at_and_ucmd!(); diff --git a/util/build-gnu.sh b/util/build-gnu.sh index 223fc895b6c..2a7506fc341 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -203,6 +203,11 @@ grep -rlE '/usr/local/bin/\s?/usr/local/bin' init.cfg tests/* | xargs -r "${SED} "${SED}" -i -e "s|removed directory 'a/'|removed directory 'a'|g" tests/rm/v-slash.sh +if test "$(grep -c 'rm: skipping ' tests/rm/one-file-system.sh)" -eq 1; then + # Do it only once. + sed -i -e "s/ >> exp/ > exp/g" -e "s|rm: and --preserve-root=all is in effect|rm: skipping 'a/b', since it's on a different device\nrm: and --preserve-root=all is in effect|g" tests/rm/one-file-system.sh +fi + # 'rel' doesn't exist. Our implementation is giving a better message. "${SED}" -i -e "s|rm: cannot remove 'rel': Permission denied|rm: cannot remove 'rel': No such file or directory|g" tests/rm/inaccessible.sh From a0df78a4e0ba51c486214701b00a417020fc9589 Mon Sep 17 00:00:00 2001 From: shskwmt Date: Tue, 20 Jan 2026 10:04:11 +0900 Subject: [PATCH 2/5] refactor(rm): improve preserve-root parsing safety - Replace `.unwrap()` with `.map()` to handle options safely without risking panics. - Update the abbreviation check to compare against the `PreserveRoot` enum directly. --- src/uu/rm/src/rm.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index bf9c9efe811..c113359d9f8 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -258,10 +258,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } else { match matches .get_one::(OPT_PRESERVE_ROOT) - .unwrap() - .as_str() + .map(|s| s.as_str()) { - "all" => PreserveRoot::YesAll, + Some("all") => PreserveRoot::YesAll, _ => PreserveRoot::Default, } }, @@ -278,7 +277,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // manually parse all args to verify --no-preserve-root did not get abbreviated (clap does // allow this) - if !options.preserve_root && !args.iter().any(|arg| arg == "--no-preserve-root") { + if options.preserve_root == PreserveRoot::No + && !args.iter().any(|arg| arg == "--no-preserve-root") + { return Err(RmError::MayNotAbbreviateNoPreserveRoot.into()); } From 2617af7968cf11089244d5087ab2e61351a1fe13 Mon Sep 17 00:00:00 2001 From: shskwmt Date: Tue, 20 Jan 2026 10:30:22 +0900 Subject: [PATCH 3/5] refactor(rm): improve error context in check_one_fs Replace empty error strings with descriptive messages when path parent or mount point resolution fails. This aids in debugging by identifying specifically which path caused the resolution failure during the one-file-system check. --- src/uu/rm/src/rm.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index c113359d9f8..134e1c8faeb 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -750,11 +750,16 @@ fn check_one_fs(path: &Path, options: &Options) -> Result<(), String> { .map_err(|err| format!("cannot canonicalize {}: {err}", path.quote()))?; // Get parent path, handling root case - let parent_canon = child_canon.parent().ok_or("")?.to_path_buf(); + let parent_canon = child_canon + .parent() + .ok_or_else(|| format!("cannot get parent of {}", child_canon.quote()))? + .to_path_buf(); // Find mount points for child and parent - let child_mount = mount_for_path(&child_canon, &fs_list).ok_or("")?; - let parent_mount = mount_for_path(&parent_canon, &fs_list).ok_or("")?; + let child_mount = mount_for_path(&child_canon, &fs_list) + .ok_or_else(|| format!("cannot find mount point for {}", child_canon.quote()))?; + let parent_mount = mount_for_path(&parent_canon, &fs_list) + .ok_or_else(|| format!("cannot find mount point for {}", parent_canon.quote()))?; // Check if child and parent are on the same device if child_mount.dev_id != parent_mount.dev_id { From f1c343640450e8fdb66ba199dd02395addd7cfc9 Mon Sep 17 00:00:00 2001 From: shskwmt Date: Tue, 20 Jan 2026 10:44:11 +0900 Subject: [PATCH 4/5] refactor(rm): extract one-fs and root preservation checks to helper Introduce `check_and_report_one_fs` to encapsulate the logic for verifying filesystem boundaries (`-x`) and root preservation. This removes code duplication between `remove_dir_recursive` and `handle_dir`, ensuring consistent error reporting for "different device" skips and `--preserve-root=all` violations. --- src/uu/rm/src/rm.rs | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 134e1c8faeb..9f030ea1e17 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -586,6 +586,27 @@ fn is_writable_metadata(_metadata: &Metadata) -> bool { true } +/// Helper function to check fs and report errors if necessary. +/// Returns true if the operation should be skipped/returned (i.e., on error). +fn check_and_report_one_fs(path: &Path, options: &Options) -> bool { + if let Err(additional_reason) = check_one_fs(path, options) { + if !additional_reason.is_empty() { + show_error!("{}", additional_reason); + } + show_error!( + "skipping {}, since it's on a different device", + path.quote() + ); + + if options.preserve_root == PreserveRoot::YesAll { + show_error!("and --preserve-root=all is in effect"); + } + + return true; + } + false +} + /// Recursively remove the directory tree rooted at the given path. /// /// If `path` is a file or a symbolic link, just remove it. If it is a @@ -608,14 +629,7 @@ fn remove_dir_recursive( } // Base case 2: check if a path is on the same file system - if let Err(additional_reason) = check_one_fs(path, options) { - if !additional_reason.is_empty() { - show_error!("{}", additional_reason); - } - show_error!( - "skipping {}, since it's on a different device", - path.quote() - ); + if check_and_report_one_fs(path, options) { return true; } @@ -772,19 +786,7 @@ fn check_one_fs(path: &Path, options: &Options) -> Result<(), String> { fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { let mut had_err = false; - if let Err(additional_reason) = check_one_fs(path, options) { - if !additional_reason.is_empty() { - show_error!("{}", additional_reason); - } - show_error!( - "skipping {}, since it's on a different device", - path.quote() - ); - - if options.preserve_root == PreserveRoot::YesAll { - show_error!("and --preserve-root=all is in effect"); - } - + if check_and_report_one_fs(path, options) { return true; } From 6918d8432bb6fb37b9e69bf3b23342b930666293 Mon Sep 17 00:00:00 2001 From: shskwmt Date: Tue, 20 Jan 2026 11:45:42 +0900 Subject: [PATCH 5/5] refactor(df): separate platform-specific logic for stat_path Split the initialization of `stat_path` in `Filesystem::new` into distinct `#[cfg(unix)]` and `#[cfg(windows)]` blocks. This improves readability and ensures that Windows builds consistently use the volume ID (`dev_id`). --- src/uu/df/src/filesystem.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/uu/df/src/filesystem.rs b/src/uu/df/src/filesystem.rs index 69d4cd5ec90..5124c00f9c8 100644 --- a/src/uu/df/src/filesystem.rs +++ b/src/uu/df/src/filesystem.rs @@ -121,21 +121,15 @@ where impl Filesystem { // TODO: resolve uuid in `mount_info.dev_name` if exists pub(crate) fn new(mount_info: MountInfo, file: Option) -> Option { + #[cfg(unix)] let stat_path = if mount_info.mount_dir.is_empty() { - #[cfg(unix)] - { - mount_info.dev_name.clone().into() - } - #[cfg(windows)] - { - // On windows, we expect the volume id - mount_info.dev_id.clone().into() - } + mount_info.dev_name.clone().into() } else { mount_info.mount_dir.clone() }; + #[cfg(windows)] - let _stat_path = mount_info.dev_id.clone(); // On windows, we expect the volume id + let stat_path = mount_info.dev_id.clone(); // On windows, we expect the volume id #[cfg(unix)] let usage = FsUsage::new(statfs(&stat_path).ok()?);