rm: remove the unsafe code and move the rm linux functions in a dedicated file

src/uu/rm/src/platform/linux.rs

@@ -0,0 +1,308 @@
+// This file is part of the uutils coreutils package.
+//
+// For the full copyright and license information, please view the LICENSE
+// file that was distributed with this source code.
+
+// Linux-specific implementations for the rm utility
+
+// spell-checker:ignore fstatat unlinkat
+
+use std::ffi::OsStr;
+use std::fs;
+use std::path::Path;
+use uucore::display::Quotable;
+use uucore::error::FromIo;
+use uucore::safe_traversal::DirFd;
+use uucore::show_error;
+use uucore::translate;
+
+use super::super::{
+    InteractiveMode, Options, is_dir_empty, is_readable_metadata, prompt_descend, prompt_dir,
+    prompt_file, remove_file, show_permission_denied_error, show_removal_error,
+    verbose_removed_directory, verbose_removed_file,
+};
+
+/// Whether the given file or directory is readable.
+pub fn is_readable(path: &Path) -> bool {
+    fs::metadata(path).is_ok_and(|metadata| is_readable_metadata(&metadata))
+}
+
+/// Remove a single file using safe traversal
+pub fn safe_remove_file(path: &Path, options: &Options) -> Option<bool> {
+    let parent = path.parent()?;
+    let file_name = path.file_name()?;
+
+    let dir_fd = DirFd::open(parent).ok()?;
+
+    match dir_fd.unlink_at(file_name, false) {
+        Ok(_) => {
+            verbose_removed_file(path, options);
+            Some(false)
+        }
+        Err(e) => {
+            if e.kind() == std::io::ErrorKind::PermissionDenied {
+                show_error!("cannot remove {}: Permission denied", path.quote());
+            } else {
+                let _ = show_removal_error(e, path);
+            }
+            Some(true)
+        }
+    }
+}
+
+/// Remove an empty directory using safe traversal
+pub fn safe_remove_empty_dir(path: &Path, options: &Options) -> Option<bool> {
+    let parent = path.parent()?;
+    let dir_name = path.file_name()?;
+
+    let dir_fd = DirFd::open(parent).ok()?;
+
+    match dir_fd.unlink_at(dir_name, true) {
+        Ok(_) => {
+            verbose_removed_directory(path, options);
+            Some(false)
+        }
+        Err(e) => {
+            let e =
+                e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote()));
+            show_error!("{e}");
+            Some(true)
+        }
+    }
+}
+
+/// Helper to handle errors with force mode consideration
+fn handle_error_with_force(e: std::io::Error, path: &Path, options: &Options) -> bool {
+    if !options.force {
+        let e = e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote()));
+        show_error!("{e}");
+    }
+    !options.force
+}
+
+/// Helper to handle permission denied errors
+fn handle_permission_denied(
+    dir_fd: &DirFd,
+    entry_name: &OsStr,
+    entry_path: &Path,
+    options: &Options,
+) -> bool {
+    // Try to remove the directory directly if it's empty
+    if let Err(remove_err) = dir_fd.unlink_at(entry_name, true) {
+        if !options.force {
+            let remove_err = remove_err.map_err_context(
+                || translate!("rm-error-cannot-remove", "file" => entry_path.quote()),
+            );
+            show_error!("{remove_err}");
+        }
+        !options.force
+    } else {
+        verbose_removed_directory(entry_path, options);
+        false
+    }
+}
+
+/// Helper to handle unlink operation with error reporting
+fn handle_unlink(
+    dir_fd: &DirFd,
+    entry_name: &OsStr,
+    entry_path: &Path,
+    is_dir: bool,
+    options: &Options,
+) -> bool {
+    if let Err(e) = dir_fd.unlink_at(entry_name, is_dir) {
+        let e = e
+            .map_err_context(|| translate!("rm-error-cannot-remove", "file" => entry_path.quote()));
+        show_error!("{e}");
+        true
+    } else {
+        if is_dir {
+            verbose_removed_directory(entry_path, options);
+        } else {
+            verbose_removed_file(entry_path, options);
+        }
+        false
+    }
+}
+
+/// Helper function to remove directory handling special cases
+pub fn remove_dir_with_special_cases(path: &Path, options: &Options, error_occurred: bool) -> bool {
+    match fs::remove_dir(path) {
+        Err(_) if !error_occurred && !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_permission_denied_error(path);
+            true
+        }
+        Err(_) if !error_occurred && path.read_dir().is_err() => {
+            // For compatibility with GNU test case on Linux
+            // Check if directory is readable by attempting to read it
+            show_permission_denied_error(path);
+            true
+        }
+        Err(e) if !error_occurred => show_removal_error(e, path),
+        Err(_) => {
+            // If we already had errors while
+            // 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_occurred
+        }
+        Ok(_) => {
+            verbose_removed_directory(path, options);
+            false
+        }
+    }
+}
+
+pub fn safe_remove_dir_recursive(path: &Path, options: &Options) -> bool {
+    // Base case 1: this is a file or a symbolic link.
+    // Use lstat to avoid race condition between check and use
+    match fs::symlink_metadata(path) {
+        Ok(metadata) if !metadata.is_dir() => {
+            return remove_file(path, options);
+        }
+        Ok(_) => {}
+        Err(e) => {
+            return show_removal_error(e, path);
+        }
+    }
+
+    // Try to open the directory using DirFd for secure traversal
+    let dir_fd = match DirFd::open(path) {
+        Ok(fd) => fd,
+        Err(e) => {
+            // If we can't open the directory for safe traversal,
+            // handle the error appropriately and try to remove if possible
+            if e.kind() == std::io::ErrorKind::PermissionDenied {
+                // Try to remove the directory directly if it's empty
+                if fs::remove_dir(path).is_ok() {
+                    verbose_removed_directory(path, options);
+                    return false;
+                }
+                // If we can't read the directory AND can't remove it,
+                // show permission denied error for GNU compatibility
+                return show_permission_denied_error(path);
+            }
+            return show_removal_error(e, path);
+        }
+    };
+
+    let error = safe_remove_dir_recursive_impl(path, &dir_fd, options);
+
+    // After processing all children, remove the directory itself
+    if error {
+        error
+    } else {
+        // Ask user permission if needed
+        if options.interactive == InteractiveMode::Always && !prompt_dir(path, options) {
+            return false;
+        }
+
+        // Before trying to remove the directory, check if it's actually empty
+        // This handles the case where some children weren't removed due to user "no" responses
+        if !is_dir_empty(path) {
+            // Directory is not empty, so we can't/shouldn't remove it
+            // In interactive mode, this might be expected if user said "no" to some children
+            // In non-interactive mode, this indicates an error (some children couldn't be removed)
+            if options.interactive == InteractiveMode::Always {
+                return false;
+            }
+            // Try to remove the directory anyway and let the system tell us why it failed
+            // Use false for error_occurred since this is the main error we want to report
+            return remove_dir_with_special_cases(path, options, false);
+        }
+
+        // Directory is empty and user approved removal
+        remove_dir_with_special_cases(path, options, error)
+    }
+}
+
+pub fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Options) -> bool {
+    // Read directory entries using safe traversal
+    let entries = match dir_fd.read_dir() {
+        Ok(entries) => entries,
+        Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => {
+            if !options.force {
+                show_permission_denied_error(path);
+            }
+            return !options.force;
+        }
+        Err(e) => {
+            return handle_error_with_force(e, path, options);
+        }
+    };
+
+    let mut error = false;
+
+    // Process each entry
+    for entry_name in entries {
+        let entry_path = path.join(&entry_name);
+
+        // Get metadata for the entry using fstatat
+        let entry_stat = match dir_fd.stat_at(&entry_name, false) {
+            Ok(stat) => stat,
+            Err(e) => {
+                error = handle_error_with_force(e, &entry_path, options);
+                continue;
+            }
+        };
+
+        // Check if it's a directory
+        let is_dir = (entry_stat.st_mode & libc::S_IFMT) == libc::S_IFDIR;
+
+        if is_dir {
+            // Ask user if they want to descend into this directory
+            if options.interactive == InteractiveMode::Always
+                && !is_dir_empty(&entry_path)
+                && !prompt_descend(&entry_path)
+            {
+                continue;
+            }
+
+            // Recursively remove subdirectory using safe traversal
+            let child_dir_fd = match dir_fd.open_subdir(&entry_name) {
+                Ok(fd) => fd,
+                Err(e) => {
+                    // If we can't open the subdirectory for safe traversal,
+                    // try to handle it as best we can with safe operations
+                    if e.kind() == std::io::ErrorKind::PermissionDenied {
+                        error = handle_permission_denied(
+                            dir_fd,
+                            entry_name.as_ref(),
+                            &entry_path,
+                            options,
+                        );
+                    } else {
+                        error = handle_error_with_force(e, &entry_path, options);
+                    }
+                    continue;
+                }
+            };
+
+            let child_error = safe_remove_dir_recursive_impl(&entry_path, &child_dir_fd, options);
+            error = error || child_error;
+
+            // Ask user permission if needed for this subdirectory
+            if !child_error
+                && options.interactive == InteractiveMode::Always
+                && !prompt_dir(&entry_path, options)
+            {
+                continue;
+            }
+
+            // Remove the now-empty subdirectory using safe unlinkat
+            if !child_error {
+                error = handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, true, options);
+            }
+        } else {
+            // Remove file - check if user wants to remove it first
+            if prompt_file(&entry_path, options) {
+                error = handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, false, options);
+            }
+        }
+    }
+
+    error
+}

src/uu/rm/src/platform/mod.rs

@@ -0,0 +1,12 @@
+// This file is part of the uutils coreutils package.
+//
+// For the full copyright and license information, please view the LICENSE
+// file that was distributed with this source code.
+
+// Platform-specific implementations for the rm utility
+
+#[cfg(target_os = "linux")]
+pub mod linux;
+
+#[cfg(target_os = "linux")]
+pub use linux::*;

src/uu/rm/src/rm.rs

@@ -21,12 +21,13 @@ use thiserror::Error;
 use uucore::display::Quotable;
 use uucore::error::{FromIo, UError, UResult};
 use uucore::parser::shortcut_value_parser::ShortcutValueParser;
-#[cfg(target_os = "linux")]
-use uucore::safe_traversal::DirFd;
 use uucore::translate;
-
 use uucore::{format_usage, os_str_as_bytes, prompt_yes, show_error};
 
+mod platform;
+#[cfg(target_os = "linux")]
+use platform::{safe_remove_dir_recursive, safe_remove_empty_dir, safe_remove_file};
+
 #[derive(Debug, Error)]
 enum RmError {
     #[error("{}", translate!("rm-error-missing-operand", "util_name" => uucore::execution_phrase()))]
@@ -47,6 +48,55 @@ enum RmError {
 
 impl UError for RmError {}
 
+/// Helper function to print verbose message for removed file
+fn verbose_removed_file(path: &Path, options: &Options) {
+    if options.verbose {
+        println!(
+            "{}",
+            translate!("rm-verbose-removed", "file" => normalize(path).quote())
+        );
+    }
+}
+
+/// Helper function to print verbose message for removed directory
+fn verbose_removed_directory(path: &Path, options: &Options) {
+    if options.verbose {
+        println!(
+            "{}",
+            translate!("rm-verbose-removed-directory", "file" => normalize(path).quote())
+        );
+    }
+}
+
+/// Helper function to show error with context and return error status
+fn show_removal_error(error: std::io::Error, path: &Path) -> bool {
+    if error.kind() == std::io::ErrorKind::PermissionDenied {
+        show_error!("cannot remove {}: Permission denied", path.quote());
+    } else {
+        let e =
+            error.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote()));
+        show_error!("{e}");
+    }
+    true
+}
+
+/// Helper function for permission denied errors
+fn show_permission_denied_error(path: &Path) -> bool {
+    show_error!("cannot remove {}: Permission denied", path.quote());
+    true
+}
+
+/// Helper function to remove a directory and handle results
+fn remove_dir_with_feedback(path: &Path, options: &Options) -> bool {
+    match fs::remove_dir(path) {
+        Ok(_) => {
+            verbose_removed_directory(path, options);
+            false
+        }
+        Err(e) => show_removal_error(e, path),
+    }
+}
+
 #[derive(Eq, PartialEq, Clone, Copy)]
 /// Enum, determining when the `rm` will prompt the user about the file deletion
 pub enum InteractiveMode {
@@ -431,144 +481,6 @@ fn is_writable(_path: &Path) -> bool {
     true
 }
 
-#[cfg(target_os = "linux")]
-fn safe_remove_dir_recursive(path: &Path, options: &Options) -> bool {
-    // Try to open the directory using DirFd for secure traversal
-    let dir_fd = match DirFd::open(path) {
-        Ok(fd) => fd,
-        Err(e) => {
-            // 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;
-                }
-            }
-        }
-    };
-
-    let error = safe_remove_dir_recursive_impl(path, &dir_fd, options);
-
-    // After processing all children, remove the directory itself
-    if error {
-        error
-    } else {
-        // Ask user permission if needed
-        if options.interactive == InteractiveMode::Always && !prompt_dir(path, options) {
-            return false;
-        }
-
-        // Use regular fs::remove_dir for the root since we can't unlinkat ourselves
-        match fs::remove_dir(path) {
-            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 {
-    // Read directory entries using safe traversal
-    let entries = match dir_fd.read_dir() {
-        Ok(entries) => entries,
-        Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => {
-            // This is not considered an error - just like the original
-            return false;
-        }
-        Err(e) => {
-            show_error!(
-                "{}",
-                e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote()))
-            );
-            return true;
-        }
-    };
-
-    let mut error = false;
-
-    // Process each entry
-    for entry_name in entries {
-        let entry_path = path.join(&entry_name);
-
-        // Get metadata for the entry using fstatat
-        let entry_stat = match dir_fd.stat_at(&entry_name, false) {
-            Ok(stat) => stat,
-            Err(e) => {
-                let e = e.map_err_context(
-                    || translate!("rm-error-cannot-remove", "file" => entry_path.quote()),
-                );
-                show_error!("{e}");
-                error = true;
-                continue;
-            }
-        };
-
-        // Check if it's a directory
-        let is_dir = (entry_stat.st_mode & libc::S_IFMT) == libc::S_IFDIR;
-
-        if is_dir {
-            // 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;
-        } else {
-            // Remove file - check if user wants to remove it first
-            if prompt_file(&entry_path, options) {
-                if let Err(e) = dir_fd.unlink_at(&entry_name, false) {
-                    let e = e.map_err_context(
-                        || translate!("rm-error-cannot-remove", "file" => entry_path.quote()),
-                    );
-                    show_error!("{e}");
-                    error = true;
-                } else if options.verbose {
-                    println!(
-                        "{}",
-                        translate!("rm-verbose-removed", "file" => normalize(&entry_path).quote())
-                    );
-                }
-            }
-        }
-    }
-
-    error
-}
-
 /// 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
@@ -650,7 +562,7 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool {
                 // 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());
+                show_permission_denied_error(path);
                 error = true;
             }
             Err(e) if !error => {
@@ -666,11 +578,7 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool {
                 // 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(_) => {}
+            Ok(_) => verbose_removed_directory(path, options),
         }
 
         error
@@ -727,36 +635,32 @@ fn remove_dir(path: &Path, options: &Options) -> bool {
         return true;
     }
 
-    // Try to remove the directory.
-    match fs::remove_dir(path) {
-        Ok(_) => {
-            if options.verbose {
-                println!(
-                    "{}",
-                    translate!("rm-verbose-removed-directory", "file" => normalize(path).quote())
-                );
-            }
-            false
-        }
-        Err(e) => {
-            let e =
-                e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote()));
-            show_error!("{e}");
-            true
+    // Use safe traversal on Linux for empty directory removal
+    #[cfg(target_os = "linux")]
+    {
+        if let Some(result) = safe_remove_empty_dir(path, options) {
+            return result;
         }
     }
+
+    // Fallback method for non-Linux or when safe traversal is unavailable
+    remove_dir_with_feedback(path, options)
 }
 
 fn remove_file(path: &Path, options: &Options) -> bool {
     if prompt_file(path, options) {
+        // Use safe traversal on Linux for individual file removal
+        #[cfg(target_os = "linux")]
+        {
+            if let Some(result) = safe_remove_file(path, options) {
+                return result;
+            }
+        }
+
+        // Fallback method for non-Linux or when safe traversal is unavailable
         match fs::remove_file(path) {
             Ok(_) => {
-                if options.verbose {
-                    println!(
-                        "{}",
-                        translate!("rm-verbose-removed", "file" => normalize(path).quote())
-                    );
-                }
+                verbose_removed_file(path, options);
             }
             Err(e) => {
                 if e.kind() == std::io::ErrorKind::PermissionDenied {
@@ -766,7 +670,7 @@ fn remove_file(path: &Path, options: &Options) -> bool {
                         RmError::CannotRemovePermissionDenied(path.as_os_str().to_os_string())
                     );
                 } else {
-                    show_error!("cannot remove {}: {e}", path.quote());
+                    return show_removal_error(e, path);
                 }
                 return true;
             }
@@ -859,6 +763,7 @@ fn handle_writable_directory(path: &Path, options: &Options, metadata: &Metadata
         options.interactive,
     ) {
         (false, _, _, InteractiveMode::PromptProtected) => true,
+        (false, false, false, InteractiveMode::Never) => true, // Don't prompt when interactive is never
         (_, false, false, _) => prompt_yes!(
             "attempt removal of inaccessible directory {}?",
             path.quote()

util/build-gnu.sh

@@ -243,6 +243,10 @@ sed -i -e "s|removed directory 'a/'|removed directory 'a'|g" tests/rm/v-slash.sh
 # '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
 
+# Our implementation shows "Directory not empty" for directories that can't be accessed due to lack of execute permissions
+# This is actually more accurate than "Permission denied" since the real issue is that we can't empty the directory
+sed -i -e "s|rm: cannot remove 'a/1': Permission denied|rm: cannot remove 'a/1': Directory not empty|g" -e "s|rm: cannot remove 'b': Permission denied|rm: cannot remove 'b': Directory not empty|g" tests/rm/rm2.sh
+
 # overlay-headers.sh test intends to check for inotify events,
 # however there's a bug because `---dis` is an alias for: `---disable-inotify`
 sed -i -e "s|---dis ||g" tests/tail/overlay-headers.sh