From af4c25d83c361bd603dcc70f8989e7ca86c34089 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 27 Jan 2026 14:39:21 +0100 Subject: [PATCH] rm: fix --preserve-root detection for nested symlinks with trailing slash --- src/uu/rm/locales/en-US.ftl | 1 + src/uu/rm/locales/fr-FR.ftl | 1 + src/uu/rm/src/rm.rs | 75 ++++++++++++++++++++++++++++--------- tests/by-util/test_rm.rs | 71 +++++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 17 deletions(-) diff --git a/src/uu/rm/locales/en-US.ftl b/src/uu/rm/locales/en-US.ftl index 2d4486ce2fe..8a9a3513eaa 100644 --- a/src/uu/rm/locales/en-US.ftl +++ b/src/uu/rm/locales/en-US.ftl @@ -40,6 +40,7 @@ rm-error-cannot-remove-no-such-file = cannot remove {$file}: No such file or dir rm-error-cannot-remove-permission-denied = cannot remove {$file}: Permission denied rm-error-cannot-remove-is-directory = cannot remove {$file}: Is a directory rm-error-dangerous-recursive-operation = it is dangerous to operate recursively on '/' +rm-error-dangerous-recursive-operation-same-as-root = it is dangerous to operate recursively on '{$path}' (same as '/') rm-error-use-no-preserve-root = use --no-preserve-root to override this failsafe rm-error-refusing-to-remove-directory = refusing to remove '.' or '..' directory: skipping {$path} rm-error-cannot-remove = cannot remove {$file} diff --git a/src/uu/rm/locales/fr-FR.ftl b/src/uu/rm/locales/fr-FR.ftl index 52d881d0266..6052e2f8964 100644 --- a/src/uu/rm/locales/fr-FR.ftl +++ b/src/uu/rm/locales/fr-FR.ftl @@ -40,6 +40,7 @@ rm-error-cannot-remove-no-such-file = impossible de supprimer {$file} : Aucun fi rm-error-cannot-remove-permission-denied = impossible de supprimer {$file} : Permission refusée rm-error-cannot-remove-is-directory = impossible de supprimer {$file} : C'est un répertoire rm-error-dangerous-recursive-operation = il est dangereux d'opérer récursivement sur '/' +rm-error-dangerous-recursive-operation-same-as-root = il est dangereux d'opérer récursivement sur '{$path}' (identique à '/') rm-error-use-no-preserve-root = utilisez --no-preserve-root pour outrepasser cette protection rm-error-refusing-to-remove-directory = refus de supprimer le répertoire '.' ou '..' : ignorer {$path} rm-error-cannot-remove = impossible de supprimer {$file} diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 99cb0baa0d5..b1c7d1dedb8 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (path) eacces inacc rm-r4 unlinkat fstatat +// spell-checker:ignore (path) eacces inacc rm-r4 unlinkat fstatat rootlink use clap::builder::{PossibleValue, ValueParser}; use clap::{Arg, ArgAction, Command, parser::ValueSource}; @@ -17,7 +17,7 @@ use std::os::unix::ffi::OsStrExt; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::MAIN_SEPARATOR; -use std::path::{Path, PathBuf}; +use std::path::Path; use thiserror::Error; use uucore::display::Quotable; use uucore::error::{FromIo, UError, UResult}; @@ -56,7 +56,7 @@ fn verbose_removed_file(path: &Path, options: &Options) { if options.verbose { println!( "{}", - translate!("rm-verbose-removed", "file" => normalize(path).quote()) + translate!("rm-verbose-removed", "file" => uucore::fs::normalize_path(path).quote()) ); } } @@ -66,7 +66,7 @@ fn verbose_removed_directory(path: &Path, options: &Options) { if options.verbose { println!( "{}", - translate!("rm-verbose-removed-directory", "file" => normalize(path).quote()) + translate!("rm-verbose-removed-directory", "file" => uucore::fs::normalize_path(path).quote()) ); } } @@ -229,6 +229,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }) }; + let preserve_root = !matches.get_flag(OPT_NO_PRESERVE_ROOT); + let recursive = matches.get_flag(OPT_RECURSIVE); + let options = Options { force: force_flag, interactive: { @@ -245,8 +248,8 @@ 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), - recursive: matches.get_flag(OPT_RECURSIVE), + preserve_root, + recursive, dir: matches.get_flag(OPT_DIR), verbose: matches.get_flag(OPT_VERBOSE), progress: matches.get_flag(OPT_PROGRESS), @@ -482,6 +485,19 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { for filename in files { let file = Path::new(filename); + // Check if the path (potentially with trailing slash) resolves to root + // This needs to happen before symlink_metadata to catch cases like "rootlink/" + // where rootlink is a symlink to root. + if uucore::fs::path_ends_with_terminator(file) + && options.recursive + && options.preserve_root + && is_root_path(file) + { + show_preserve_root_error(file); + had_err = true; + continue; + } + had_err = match file.symlink_metadata() { Ok(metadata) => { // Create progress bar on first successful file metadata read @@ -668,6 +684,40 @@ fn remove_dir_recursive( } } +/// Check if a path resolves to the root directory. +/// Returns true if the path is root, false otherwise. +fn is_root_path(path: &Path) -> bool { + // Check simple case: literal "/" path + if path.has_root() && path.parent().is_none() { + return true; + } + + // Check if path resolves to "/" after following symlinks + if let Ok(canonical) = path.canonicalize() { + canonical.has_root() && canonical.parent().is_none() + } else { + false + } +} + +/// Show error message for attempting to remove root. +fn show_preserve_root_error(path: &Path) { + let path_looks_like_root = path.has_root() && path.parent().is_none(); + + if path_looks_like_root { + // Path is literally "/" + show_error!("{}", RmError::DangerousRecursiveOperation); + } else { + // Path resolves to root but isn't literally "/" (e.g., symlink to /) + show_error!( + "{}", + translate!("rm-error-dangerous-recursive-operation-same-as-root", + "path" => path.display()) + ); + } + show_error!("{}", RmError::UseNoPreserveRoot); +} + fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { let mut had_err = false; @@ -680,14 +730,13 @@ fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar> return true; } - let is_root = path.has_root() && path.parent().is_none(); + let is_root = is_root_path(path); if options.recursive && (!is_root || !options.preserve_root) { had_err = remove_dir_recursive(path, options, progress_bar); } else if options.dir && (!is_root || !options.preserve_root) { had_err = remove_dir(path, options, progress_bar).bitor(had_err); } else if options.recursive { - show_error!("{}", RmError::DangerousRecursiveOperation); - show_error!("{}", RmError::UseNoPreserveRoot); + show_preserve_root_error(path); had_err = true; } else { show_error!( @@ -935,14 +984,6 @@ fn prompt_descend(path: &Path) -> bool { prompt_yes!("descend into directory {}?", path.quote()) } -fn normalize(path: &Path) -> PathBuf { - // copied from https://github.com/rust-lang/cargo/blob/2e4cfc2b7d43328b207879228a2ca7d427d188bb/src/cargo/util/paths.rs#L65-L90 - // both projects are MIT https://github.com/rust-lang/cargo/blob/master/LICENSE-MIT - // for std impl progress see rfc https://github.com/rust-lang/rfcs/issues/2208 - // TODO: replace this once that lands - uucore::fs::normalize_path(path) -} - #[cfg(not(windows))] fn is_symlink_dir(_metadata: &Metadata) -> bool { false diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index e262e9612b1..c303a4bab66 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -2,6 +2,7 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. +// spell-checker:ignore rootlink #![allow(clippy::stable_sort_primitive)] use std::process::Stdio; @@ -1290,3 +1291,73 @@ fn test_symlink_to_readonly_no_prompt() { assert!(!at.symlink_exists("bar")); } + +/// Test that --preserve-root properly detects symlinks pointing to root. +#[cfg(unix)] +#[test] +fn test_preserve_root_symlink_to_root() { + let (at, mut ucmd) = at_and_ucmd!(); + + // Create a symlink pointing to the root directory + at.symlink_dir("/", "rootlink"); + + // Attempting to recursively delete through this symlink should fail + // because it resolves to the same device/inode as "/" + ucmd.arg("-rf") + .arg("--preserve-root") + .arg("rootlink/") + .fails() + .stderr_contains("it is dangerous to operate recursively on") + .stderr_contains("(same as '/')"); + + // The symlink itself should still exist (we didn't delete it) + assert!(at.symlink_exists("rootlink")); +} + +/// Test that --preserve-root properly detects nested symlinks pointing to root. +#[cfg(unix)] +#[test] +fn test_preserve_root_nested_symlink_to_root() { + let (at, mut ucmd) = at_and_ucmd!(); + + // Create a symlink pointing to the root directory + at.symlink_dir("/", "rootlink"); + // Create another symlink pointing to the first symlink + at.symlink_dir("rootlink", "rootlink2"); + + // Attempting to recursively delete through nested symlinks should also fail + ucmd.arg("-rf") + .arg("--preserve-root") + .arg("rootlink2/") + .fails() + .stderr_contains("it is dangerous to operate recursively on") + .stderr_contains("(same as '/')"); +} + +/// Test that removing the symlink itself (not the target) still works. +#[cfg(unix)] +#[test] +fn test_preserve_root_symlink_removal_without_trailing_slash() { + let (at, mut ucmd) = at_and_ucmd!(); + + // Create a symlink pointing to the root directory + at.symlink_dir("/", "rootlink"); + + // Removing the symlink itself (without trailing slash) should succeed + // because we're removing the link, not traversing through it + ucmd.arg("--preserve-root").arg("rootlink").succeeds(); + + assert!(!at.symlink_exists("rootlink")); +} + +/// Test that literal "/" is still properly protected. +#[test] +fn test_preserve_root_literal_root() { + new_ucmd!() + .arg("-rf") + .arg("--preserve-root") + .arg("/") + .fails() + .stderr_contains("it is dangerous to operate recursively on '/'") + .stderr_contains("use --no-preserve-root to override this failsafe"); +}