diff --git a/.github/workflows/ignore-intermittent.txt b/.github/workflows/ignore-intermittent.txt index e163202a2ca..4fee2da92b5 100644 --- a/.github/workflows/ignore-intermittent.txt +++ b/.github/workflows/ignore-intermittent.txt @@ -1,3 +1,2 @@ tests/tail/inotify-dir-recreate tests/misc/timeout -tests/rm/rm1 diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 4fc37a1300f..548cd77b1b4 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -9,6 +9,7 @@ use clap::{builder::ValueParser, crate_version, parser::ValueSource, Arg, ArgAct use std::collections::VecDeque; use std::ffi::{OsStr, OsString}; use std::fs::{self, File, Metadata}; +use std::io; use std::io::ErrorKind; use std::ops::BitOr; use std::path::{Path, PathBuf}; @@ -322,6 +323,86 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { had_err } +// Custom function to format io::Error messages +fn format_io_error(err: &io::Error) -> String { + match err.kind() { + std::io::ErrorKind::PermissionDenied => "Permission denied".to_string(), + std::io::ErrorKind::NotFound => "File or directory not found".to_string(), + _ => { + if err.raw_os_error() == Some(39) { + "Directory not empty".to_string() + } else { + format!("{}", err) + } + } + } +} + +/// remove_dir_all implementation in std rust doesn't continue when hitting an error +/// This is a implementation of remove_all_dir that continues +fn remove_dir_all_recursive_continue(path: &Path) -> Result<(), bool> { + let mut had_err = false; + + for child in fs::read_dir(path).map_err(|e| { + if e.kind() == std::io::ErrorKind::PermissionDenied { + // we will be able to manage it later with remove_dir + // so, no need to show an error + false + } else { + show_error!("cannot remove {}: {}", path.quote(), format_io_error(&e)); + true + } + })? { + match child { + Ok(child) => { + let is_dir = child + .file_type() + .map_err(|e| { + show_error!( + "cannot read file type for {}: {}", + child.path().quote(), + format_io_error(&e) + ); + true + })? + .is_dir(); + + if is_dir { + if remove_dir_all_recursive_continue(&child.path()).is_err() { + had_err = true; + } + } else if let Err(e) = fs::remove_file(&child.path()) { + show_error!( + "cannot remove {}: {}", + child.path().quote(), + format_io_error(&e) + ); + had_err = true; + } + } + Err(e) => { + show_error!( + "error processing child of {}: {}", + path.quote(), + format_io_error(&e) + ); + had_err = true; + } + } + } + + if let Err(e) = fs::remove_dir(path) { + show_error!("cannot remove {}: {}", path.quote(), format_io_error(&e)); + had_err = true; + } + + if had_err { + Err(had_err) + } else { + Ok(()) + } +} + #[allow(clippy::cognitive_complexity)] fn handle_dir(path: &Path, options: &Options) -> bool { let mut had_err = false; @@ -329,22 +410,12 @@ fn handle_dir(path: &Path, options: &Options) -> bool { 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 { - if let Err(e) = fs::remove_dir_all(path) { - // GNU compatibility (rm/empty-inacc.sh) - // remove_dir_all failed. maybe it is because of the permissions - // but if the directory is empty, remove_dir might work. - // So, let's try that before failing for real - if let Err(_e) = fs::remove_dir(path) { - had_err = true; - if e.kind() == std::io::ErrorKind::PermissionDenied { - // GNU compatibility (rm/fail-eacces.sh) - // here, GNU doesn't use some kind of remove_dir_all - // It will show directory+file - show_error!("cannot remove {}: {}", path.quote(), "Permission denied"); - } else { - show_error!("cannot remove {}: {}", path.quote(), e); - } - } + // GNU compatibility (rm/empty-inacc.sh) + // remove_dir_all_recursive_continue failed. maybe it is because of the permissions + // but if the directory is empty, remove_dir might work. + // So, let's try that before failing for real + if remove_dir_all_recursive_continue(path).is_err() && fs::remove_dir(path).is_err() { + had_err = true; } } else { let mut dirs: VecDeque = VecDeque::new(); @@ -421,16 +492,7 @@ fn remove_dir(path: &Path, options: &Options) -> bool { } } Err(e) => { - if e.kind() == std::io::ErrorKind::PermissionDenied { - // GNU compatibility (rm/fail-eacces.sh) - show_error!( - "cannot remove {}: {}", - path.quote(), - "Permission denied" - ); - } else { - show_error!("cannot remove {}: {}", path.quote(), e); - } + show_error!("cannot remove {}: {}", path.quote(), format_io_error(&e)); return true; } } @@ -463,12 +525,7 @@ fn remove_file(path: &Path, options: &Options) -> bool { } } Err(e) => { - if e.kind() == std::io::ErrorKind::PermissionDenied { - // GNU compatibility (rm/fail-eacces.sh) - show_error!("cannot remove {}: {}", path.quote(), "Permission denied"); - } else { - show_error!("cannot remove {}: {}", path.quote(), e); - } + show_error!("cannot remove {}: {}", path.quote(), format_io_error(&e)); return true; } } diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 5125c746da7..eec9584da50 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -698,3 +698,28 @@ fn test_non_utf8() { ucmd.arg(file).succeeds(); assert!(!at.file_exists(file)); } + +#[cfg(feature = "chmod")] +#[test] +fn test_rm_recursive_with_permission_denied() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.mkdir_all("b/a/p"); + at.mkdir_all("b/c"); + at.mkdir_all("b/d"); + + // Change permissions to make 'b/a' write-protected + scene.ccmd("chmod").arg("ug-w").arg("b/a").succeeds(); + + let result = scene.ucmd().arg("-rf").arg("b").fails(); + + assert!(result + .stderr_str() + .contains("rm: cannot remove 'b/a/p': Permission denied")); + assert!(result + .stderr_str() + .contains("rm: cannot remove 'b/a': Directory not empty")); + assert!(!at.dir_exists("b/d")); + assert!(at.dir_exists("b/a")); +} diff --git a/util/build-gnu.sh b/util/build-gnu.sh index 18545dd4817..a9cb0398013 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -202,13 +202,28 @@ sed -i -e "s|rm: cannot remove 'e/slink'|rm: cannot remove 'e'|g" tests/rm/fail- sed -i -e "s|rm: cannot remove 'a/b'|rm: cannot remove 'a'|g" tests/rm/fail-2eperm.sh -sed -i -e "s|rm: cannot remove 'a/b/file'|rm: cannot remove 'a'|g" tests/rm/cycle.sh - -sed -i -e "s|rm: cannot remove directory 'b/a/p'|rm: cannot remove 'b'|g" tests/rm/rm1.sh - sed -i -e "s|rm: cannot remove 'a/1'|rm: cannot remove 'a'|g" tests/rm/rm2.sh sed -i -e "s|removed directory 'a/'|removed directory 'a'|g" tests/rm/v-slash.sh +sed -i "s|\$prog: cannot remove '\$d': Permission denied\\\n||" tests/rm/unreadable.pl + +# our error messages are better +if ! grep -q "rm: cannot remove 'b'" tests/rm/rm1.sh; then +# to make sure the modification is done only once +sed -i 's|rm: cannot remove '\''b/a/p'\'': Permission denied|rm: cannot remove '\''b/a/p'\'': Permission denied\nrm: cannot remove '\''b/a'\'': Directory not empty\nrm: cannot remove '\''b'\'': Directory not empty|' tests/rm/rm1.sh +fi + +sed -i 's|rm: cannot remove '\''e'\'': Permission denied|rm: cannot remove '\''e/slink'\'': Permission denied\nrm: cannot remove '\''e'\'': Directory not empty|' tests/rm/fail-eacces.sh + +if ! grep -q "rm: cannot remove 'a'" tests/rm/cycle.sh; then +# to make sure the modification is done only once +sed -i '/rm: cannot remove '\''a\/b\/file'\''$/{ +N +/\nrm: cannot remove '\''a\/b'\''\n rm: cannot remove '\''a'\''/!{ +s|rm: cannot remove '\''a\/b\/file'\''|&\nrm: cannot remove '\''a/b'\''\nrm: cannot remove '\''a'\''|g +} +}' tests/rm/cycle.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