diff --git a/src/uu/df/src/filesystem.rs b/src/uu/df/src/filesystem.rs index bfd9826460a..2120a173c58 100644 --- a/src/uu/df/src/filesystem.rs +++ b/src/uu/df/src/filesystem.rs @@ -121,19 +121,16 @@ 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 cf53e032388..657a32b492e 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"] } fluent = { workspace = true } indicatif = { workspace = true } +thiserror = { workspace = true } +uucore = { workspace = true, features = [ + "fs", + "fsext", + "parser", + "safe-traversal", +] } [target.'cfg(all(unix, not(target_os = "redox")))'.dependencies] uucore = { workspace = true, features = ["safe-traversal"] } diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 252c723406b..9f030ea1e17 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}; @@ -128,6 +129,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 @@ -154,7 +162,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` @@ -175,7 +183,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, @@ -245,7 +253,17 @@ 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) + .map(|s| s.as_str()) + { + Some("all") => PreserveRoot::YesAll, + _ => PreserveRoot::Default, + } + }, recursive: matches.get_flag(OPT_RECURSIVE), dir: matches.get_flag(OPT_DIR), verbose: matches.get_flag(OPT_VERBOSE), @@ -259,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()); } @@ -351,7 +371,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) @@ -470,7 +493,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. @@ -564,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 @@ -585,7 +628,12 @@ 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 check_and_report_one_fs(path, options) { + 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) @@ -673,9 +721,75 @@ 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_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_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 { + return Err(String::new()); + } + + Ok(()) +} + fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { let mut had_err = false; + if check_and_report_one_fs(path, options) { + return true; + } + let path = clean_trailing_slashes(path); if path_is_current_or_parent_directory(path) { show_error!( @@ -686,9 +800,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 c31b8272599..526b87a9826 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 20d4a935714..4c1fbdcb778 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1151,6 +1151,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 7a9f45c385b..bb1ff5f230b 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -205,6 +205,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