From e773c95c4e62424db17563242c35e488a6d1ae9b Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Fri, 26 Sep 2025 10:53:13 +0200 Subject: [PATCH] rm: on linux use the safe traversal in all cases --- src/uu/rm/src/rm.rs | 196 ++++++++++++++++++++++---------------------- 1 file changed, 99 insertions(+), 97 deletions(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 763590f79..92244de49 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -395,6 +395,7 @@ fn is_readable_metadata(metadata: &Metadata) -> bool { /// Whether the given file or directory is readable. #[cfg(unix)] +#[cfg(not(target_os = "linux"))] fn is_readable(path: &Path) -> bool { match fs::metadata(path) { Err(_) => false, @@ -436,11 +437,29 @@ fn safe_remove_dir_recursive(path: &Path, options: &Options) -> bool { let dir_fd = match DirFd::open(path) { Ok(fd) => fd, Err(e) => { - show_error!( - "{}", - e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote())) - ); - return true; + // If we can't open the directory for safe traversal, try removing it as empty directory + // This handles the case where it's an empty directory with no read permissions + match fs::remove_dir(path) { + Ok(_) => { + if options.verbose { + println!( + "{}", + translate!("rm-verbose-removed-directory", "file" => normalize(path).quote()) + ); + } + return false; + } + Err(_) => { + // If we can't remove it as empty dir either, report the original open error + show_error!( + "{}", + e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => path.quote()) + ) + ); + return true; + } + } } }; @@ -457,28 +476,35 @@ fn safe_remove_dir_recursive(path: &Path, options: &Options) -> bool { // Use regular fs::remove_dir for the root since we can't unlinkat ourselves match fs::remove_dir(path) { - Ok(_) => false, - Err(e) => { + Ok(_) => { + if options.verbose { + println!( + "{}", + translate!("rm-verbose-removed-directory", "file" => normalize(path).quote()) + ); + } + false + } + Err(e) if !error => { let e = e.map_err_context( || translate!("rm-error-cannot-remove", "file" => path.quote()), ); show_error!("{e}"); true } + Err(_) => { + // If there has already been at least one error when + // trying to remove the children, then there is no need to + // show another error message as we return from each level + // of the recursion. + error + } } } } #[cfg(target_os = "linux")] fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Options) -> bool { - // Check if we should descend into this directory - if options.interactive == InteractiveMode::Always - && !is_dir_empty(path) - && !prompt_descend(path) - { - return false; - } - // Read directory entries using safe traversal let entries = match dir_fd.read_dir() { Ok(entries) => entries, @@ -518,35 +544,9 @@ fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Options let is_dir = (entry_stat.st_mode & libc::S_IFMT) == libc::S_IFDIR; if is_dir { - // Recursively remove directory - let subdir_fd = match dir_fd.open_subdir(&entry_name) { - Ok(fd) => fd, - Err(e) => { - let e = e.map_err_context( - || translate!("rm-error-cannot-remove", "file" => entry_path.quote()), - ); - show_error!("{e}"); - error = true; - continue; - } - }; - - let child_error = safe_remove_dir_recursive_impl(&entry_path, &subdir_fd, options); + // Recursively remove subdirectory - handle in the style of the non-Linux version + let child_error = remove_dir_recursive(&entry_path, options); error = error || child_error; - - // Try to remove the directory (even if there were some child errors) - // Ask user permission if needed - if options.interactive == InteractiveMode::Always && !prompt_dir(&entry_path, options) { - continue; - } - - if let Err(e) = dir_fd.unlink_at(&entry_name, true) { - let e = e.map_err_context( - || translate!("rm-error-cannot-remove", "file" => entry_path.quote()), - ); - show_error!("{e}"); - error = true; - } } else { // Remove file - check if user wants to remove it first if prompt_file(&entry_path, options) { @@ -556,6 +556,11 @@ fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Options ); show_error!("{e}"); error = true; + } else if options.verbose { + println!( + "{}", + translate!("rm-verbose-removed", "file" => normalize(&entry_path).quote()) + ); } } } @@ -590,17 +595,13 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { return false; } - // Use secure traversal on Linux for long paths + // Use secure traversal on Linux for all recursive directory removals #[cfg(target_os = "linux")] { - if let Some(s) = path.to_str() { - if s.len() > 1000 { - return safe_remove_dir_recursive(path, options); - } - } + safe_remove_dir_recursive(path, options) } - // Fallback for non-Linux or shorter paths + // Fallback for non-Linux or use fs::remove_dir_all for very long paths #[cfg(not(target_os = "linux"))] { if let Some(s) = path.to_str() { @@ -617,62 +618,63 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool { } } } - } - // Recursive case: this is a directory. - let mut error = false; - match fs::read_dir(path) { - Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { - // This is not considered an error. - } - Err(_) => error = true, - Ok(iter) => { - for entry in iter { - match entry { - Err(_) => error = true, - Ok(entry) => { - let child_error = remove_dir_recursive(&entry.path(), options); - error = error || child_error; + // Recursive case: this is a directory. + let mut error = false; + match fs::read_dir(path) { + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + // This is not considered an error. + } + Err(_) => error = true, + Ok(iter) => { + for entry in iter { + match entry { + Err(_) => error = true, + Ok(entry) => { + let child_error = remove_dir_recursive(&entry.path(), options); + error = error || child_error; + } } } } } - } - // Ask the user whether to remove the current directory. - if options.interactive == InteractiveMode::Always && !prompt_dir(path, options) { - return false; - } + // Ask the user whether to remove the current directory. + if options.interactive == InteractiveMode::Always && !prompt_dir(path, options) { + return false; + } - // Try removing the directory itself. - match fs::remove_dir(path) { - Err(_) if !error && !is_readable(path) => { - // For compatibility with GNU test case - // `tests/rm/unread2.sh`, show "Permission denied" in this - // case instead of "Directory not empty". - show_error!("cannot remove {}: Permission denied", path.quote()); - error = true; + // Try removing the directory itself. + match fs::remove_dir(path) { + Err(_) if !error && !is_readable(path) => { + // For compatibility with GNU test case + // `tests/rm/unread2.sh`, show "Permission denied" in this + // case instead of "Directory not empty". + show_error!("cannot remove {}: Permission denied", path.quote()); + error = true; + } + Err(e) if !error => { + let e = e.map_err_context( + || translate!("rm-error-cannot-remove", "file" => path.quote()), + ); + show_error!("{e}"); + error = true; + } + Err(_) => { + // If there has already been at least one error when + // trying to remove the children, then there is no need to + // show another error message as we return from each level + // of the recursion. + } + Ok(_) if options.verbose => println!( + "{}", + translate!("rm-verbose-removed-directory", "file" => normalize(path).quote()) + ), + Ok(_) => {} } - Err(e) if !error => { - let e = - e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote())); - show_error!("{e}"); - error = true; - } - Err(_) => { - // If there has already been at least one error when - // trying to remove the children, then there is no need to - // show another error message as we return from each level - // of the recursion. - } - Ok(_) if options.verbose => println!( - "{}", - translate!("rm-verbose-removed-directory", "file" => normalize(path).quote()) - ), - Ok(_) => {} - } - error + error + } } fn handle_dir(path: &Path, options: &Options) -> bool {