From a16630fdedc472a7625a28ec2f380737d7902f86 Mon Sep 17 00:00:00 2001 From: Anirban Halder Date: Wed, 4 Dec 2024 03:21:03 +0530 Subject: [PATCH] rm: fix the usage of '/..' '/.' with -rf options fix the test tests/rm/r-4 --------- Co-authored-by: Julian Beltz Co-authored-by: Sylvestre Ledru --- Cargo.lock | 2 +- src/uu/rm/src/rm.rs | 88 ++++++++++++++++++++++++++++++++++++++-- tests/by-util/test_rm.rs | 62 ++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 41b1a1224..60db20ca6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3751,7 +3751,7 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" dependencies = [ - "windows-sys 0.48.0", + "windows-sys 0.52.0", ] [[package]] diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index a89ba6db6..ad9c942a8 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 +// spell-checker:ignore (path) eacces inacc rm-r4 use clap::{builder::ValueParser, crate_version, parser::ValueSource, Arg, ArgAction, Command}; use std::collections::VecDeque; @@ -11,10 +11,15 @@ use std::ffi::{OsStr, OsString}; use std::fs::{self, File, Metadata}; use std::io::ErrorKind; use std::ops::BitOr; +#[cfg(not(windows))] +use std::os::unix::ffi::OsStrExt; +use std::path::MAIN_SEPARATOR; use std::path::{Path, PathBuf}; use uucore::display::Quotable; use uucore::error::{UResult, USimpleError, UUsageError}; -use uucore::{format_usage, help_about, help_section, help_usage, prompt_yes, show_error}; +use uucore::{ + format_usage, help_about, help_section, help_usage, os_str_as_bytes, prompt_yes, show_error, +}; use walkdir::{DirEntry, WalkDir}; #[derive(Eq, PartialEq, Clone, Copy)] @@ -290,6 +295,7 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { for filename in files { let file = Path::new(filename); + had_err = match file.symlink_metadata() { Ok(metadata) => { if metadata.is_dir() { @@ -300,6 +306,7 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { remove_file(file, options) } } + Err(_e) => { // TODO: actually print out the specific error // TODO: When the error is not about missing files @@ -326,6 +333,15 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { fn handle_dir(path: &Path, options: &Options) -> bool { let mut had_err = false; + let path = clean_trailing_slashes(path); + if path_is_current_or_parent_directory(path) { + show_error!( + "refusing to remove '.' or '..' directory: skipping '{}'", + path.display() + ); + return true; + } + let is_root = path.has_root() && path.parent().is_none(); if options.recursive && (!is_root || !options.preserve_root) { if options.interactive != InteractiveMode::Always && !options.verbose { @@ -396,7 +412,11 @@ fn handle_dir(path: &Path, options: &Options) -> bool { } else if options.dir && (!is_root || !options.preserve_root) { had_err = remove_dir(path, options).bitor(had_err); } else if options.recursive { - show_error!("could not remove directory {}", path.quote()); + show_error!( + "it is dangerous to operate recursively on '{}'", + MAIN_SEPARATOR + ); + show_error!("use --no-preserve-root to override this failsafe"); had_err = true; } else { show_error!( @@ -559,6 +579,20 @@ fn handle_writable_directory(path: &Path, options: &Options, metadata: &Metadata true } } +/// Checks if the path is referring to current or parent directory , if it is referring to current or any parent directory in the file tree e.g '/../..' , '../..' +fn path_is_current_or_parent_directory(path: &Path) -> bool { + let path_str = os_str_as_bytes(path.as_os_str()); + let dir_separator = MAIN_SEPARATOR as u8; + if let Ok(path_bytes) = path_str { + return path_bytes == ([b'.']) + || path_bytes == ([b'.', b'.']) + || path_bytes.ends_with(&[dir_separator, b'.']) + || path_bytes.ends_with(&[dir_separator, b'.', b'.']) + || path_bytes.ends_with(&[dir_separator, b'.', dir_separator]) + || path_bytes.ends_with(&[dir_separator, b'.', b'.', dir_separator]); + } + false +} // For windows we can use windows metadata trait and file attributes to see if a directory is readonly #[cfg(windows)] @@ -586,6 +620,40 @@ fn handle_writable_directory(path: &Path, options: &Options, metadata: &Metadata } } +/// Removes trailing slashes, for example 'd/../////' yield 'd/../' required to fix rm-r4 GNU test +fn clean_trailing_slashes(path: &Path) -> &Path { + let path_str = os_str_as_bytes(path.as_os_str()); + let dir_separator = MAIN_SEPARATOR as u8; + + if let Ok(path_bytes) = path_str { + let mut idx = if path_bytes.len() > 1 { + path_bytes.len() - 1 + } else { + return path; + }; + // Checks if element at the end is a '/' + if path_bytes[idx] == dir_separator { + for i in (1..path_bytes.len()).rev() { + // Will break at the start of the continuous sequence of '/', eg: "abc//////" , will break at + // "abc/", this will clean ////// to the root '/', so we have to be careful to not + // delete the root. + if path_bytes[i - 1] != dir_separator { + idx = i; + break; + } + } + #[cfg(unix)] + return Path::new(OsStr::from_bytes(&path_bytes[0..=idx])); + + #[cfg(not(unix))] + // Unwrapping is fine here as os_str_as_bytes() would return an error on non unix + // systems with non utf-8 characters and thus bypass the if let Ok branch + return Path::new(std::str::from_utf8(&path_bytes[0..=idx]).unwrap()); + } + } + path +} + fn prompt_descend(path: &Path) -> bool { prompt_yes!("descend into directory {}?", path.quote()) } @@ -611,3 +679,17 @@ fn is_symlink_dir(metadata: &Metadata) -> bool { metadata.file_type().is_symlink() && ((metadata.file_attributes() & FILE_ATTRIBUTE_DIRECTORY) != 0) } + +mod tests { + + #[test] + // Testing whether path the `/////` collapses to `/` + fn test_collapsible_slash_path() { + use std::path::Path; + + use crate::clean_trailing_slashes; + let path = Path::new("/////"); + + assert_eq!(Path::new("/"), clean_trailing_slashes(path)); + } +} diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index f997688c8..b220926fe 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -677,6 +677,68 @@ fn test_remove_inaccessible_dir() { assert!(!at.dir_exists(dir_1)); } +#[test] +#[cfg(not(windows))] +fn test_rm_current_or_parent_dir_rm4() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.mkdir("d"); + + let answers = [ + "rm: refusing to remove '.' or '..' directory: skipping 'd/.'", + "rm: refusing to remove '.' or '..' directory: skipping 'd/./'", + "rm: refusing to remove '.' or '..' directory: skipping 'd/./'", + "rm: refusing to remove '.' or '..' directory: skipping 'd/..'", + "rm: refusing to remove '.' or '..' directory: skipping 'd/../'", + ]; + let std_err_str = ts + .ucmd() + .arg("-rf") + .arg("d/.") + .arg("d/./") + .arg("d/.////") + .arg("d/..") + .arg("d/../") + .fails() + .stderr_move_str(); + + for (idx, line) in std_err_str.lines().enumerate() { + assert_eq!(line, answers[idx]); + } +} + +#[test] +#[cfg(windows)] +fn test_rm_current_or_parent_dir_rm4_windows() { + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + at.mkdir("d"); + + let answers = [ + "rm: refusing to remove '.' or '..' directory: skipping 'd\\.'", + "rm: refusing to remove '.' or '..' directory: skipping 'd\\.\\'", + "rm: refusing to remove '.' or '..' directory: skipping 'd\\.\\'", + "rm: refusing to remove '.' or '..' directory: skipping 'd\\..'", + "rm: refusing to remove '.' or '..' directory: skipping 'd\\..\\'", + ]; + let std_err_str = ts + .ucmd() + .arg("-rf") + .arg("d\\.") + .arg("d\\.\\") + .arg("d\\.\\\\\\\\") + .arg("d\\..") + .arg("d\\..\\") + .fails() + .stderr_move_str(); + + for (idx, line) in std_err_str.lines().enumerate() { + assert_eq!(line, answers[idx]); + } +} + #[test] #[cfg(not(windows))] fn test_fifo_removal() {