fix(install): address PR review comments for symlink race condition fix

- Replace std::io::copy() with copy_stream() for consistency and splice optimization
- Consolidate DirFd::open() and open_subdir() to take follow_symlinks parameter
- Extract finalize_installed_file() helper to reduce code duplication
- Add documentation for security behavior (symlink removal) and mode handling
- Clean up verbose comments

Author: abendrothj

src/uu/chmod/src/chmod.rs

@@ -480,7 +480,7 @@ impl Chmoder {
 
         // If the path is a directory (or we should follow symlinks), recurse into it using safe traversal
         if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() {
-            match DirFd::open(file_path) {
+            match DirFd::open(file_path, true) {
                 Ok(dir_fd) => {
                     r = self.safe_traverse_dir(&dir_fd, file_path).and(r);
                 }
@@ -534,7 +534,7 @@ impl Chmoder {
 
                 // Recurse into subdirectories using the existing directory fd
                 if meta.is_dir() {
-                    match dir_fd.open_subdir(&entry_name) {
+                    match dir_fd.open_subdir(&entry_name, true) {
                         Ok(child_dir_fd) => {
                             r = self.safe_traverse_dir(&child_dir_fd, &entry_path).and(r);
                         }

src/uu/du/src/du.rs

@@ -358,7 +358,7 @@ fn safe_du(
             Ok(s) => s,
             Err(_e) => {
                 // Try using our new DirFd method for the root directory
-                match DirFd::open(path) {
+                match DirFd::open(path, true) {
                     Ok(dir_fd) => match Stat::new_from_dirfd(&dir_fd, path) {
                         Ok(s) => s,
                         Err(e) => {
@@ -396,8 +396,8 @@ fn safe_du(
 
     // Open the directory using DirFd
     let open_result = match parent_fd {
-        Some(parent) => parent.open_subdir(path.file_name().unwrap_or(path.as_os_str())),
-        None => DirFd::open(path),
+        Some(parent) => parent.open_subdir(path.file_name().unwrap_or(path.as_os_str()), true),
+        None => DirFd::open(path, true),
     };
 
     let dir_fd = match open_result {

src/uu/install/src/install.rs

@@ -646,7 +646,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
                         && sources.len() == 1
                         && !is_potential_directory_path(&target)
                     {
-                        if let Ok(dir_fd) = DirFd::open_no_follow(to_create) {
+                        if let Ok(dir_fd) = DirFd::open(to_create, false) {
                             if let Some(filename) = target.file_name() {
                                 target_parent_fd = Some(dir_fd);
                                 target_filename = Some(filename.to_os_string());
@@ -672,6 +672,8 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
 
                 #[cfg(unix)]
                 {
+                    // Use DEFAULT_MODE (0o755) for created directories - this matches GNU install
+                    // behavior. The actual mode will be modified by umask at the kernel level.
                     match create_dir_all_safe(to_create, DEFAULT_MODE) {
                         Ok(dir_fd) => {
                             if b.target_dir.is_none()
@@ -768,47 +770,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
 
                 copy_file_safe(source, parent_fd, filename.as_os_str())?;
 
-                #[cfg(not(windows))]
-                if b.strip {
-                    strip_file(&target, b)?;
-                }
-
-                set_ownership_and_permissions(&target, b)?;
-
-                if b.preserve_timestamps {
-                    preserve_timestamps(source, &target)?;
-                }
-
-                #[cfg(all(feature = "selinux", target_os = "linux"))]
-                if !b.unprivileged {
-                    if b.preserve_context {
-                        uucore::selinux::preserve_security_context(source, &target)
-                            .map_err(|e| InstallError::SelinuxContextFailed(e.to_string()))?;
-                    } else if b.default_context {
-                        set_selinux_default_context(&target)
-                            .map_err(|e| InstallError::SelinuxContextFailed(e.to_string()))?;
-                    } else if b.context.is_some() {
-                        let context = get_context_for_selinux(b);
-                        set_selinux_security_context(&target, context)
-                            .map_err(|e| InstallError::SelinuxContextFailed(e.to_string()))?;
-                    }
-                }
-
-                if b.verbose {
-                    print!(
-                        "{}",
-                        translate!("install-verbose-copy", "from" => source.quote(), "to" => target.quote())
-                    );
-                    match backup_path {
-                        Some(path) => println!(
-                            " {}",
-                            translate!("install-verbose-backup", "backup" => path.quote())
-                        ),
-                        None => println!(),
-                    }
-                }
-
-                Ok(())
+                finalize_installed_file(source, &target, b, backup_path)
             } else {
                 copy(source, &target, b)
             }
@@ -964,8 +926,7 @@ fn copy_file_safe(from: &Path, to_parent_fd: &DirFd, to_filename: &std::ffi::OsS
 
     let mut src = File::open(from)?;
     let mut dst = to_parent_fd.open_file_at(to_filename)?;
-    std::io::copy(&mut src, &mut dst)?;
-    dst.sync_all()?;
+    copy_stream(&mut src, &mut dst)?;
 
     Ok(())
 }
@@ -997,9 +958,7 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> {
         .into());
     }
 
-    // Remove existing file at destination to allow overwriting
-    // Note: create_new() below provides TOCTOU protection; if something
-    // appears at this path between the remove and create, it will fail safely
+    // Remove existing file (create_new below provides TOCTOU protection)
     if let Err(e) = fs::remove_file(to) {
         if e.kind() != std::io::ErrorKind::NotFound {
             show_error!(
@@ -1010,7 +969,6 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> {
     }
 
     let mut handle = File::open(from)?;
-    // create_new provides TOCTOU protection
     let mut dest = OpenOptions::new().write(true).create_new(true).open(to)?;
 
     copy_stream(&mut handle, &mut dest).map_err(|err| {
@@ -1113,28 +1071,13 @@ fn preserve_timestamps(from: &Path, to: &Path) -> UResult<()> {
     Ok(())
 }
 
-/// Copy one file to a new location, changing metadata.
-///
-/// Returns a Result type with the Err variant containing the error message.
-///
-/// # Parameters
-///
-/// _from_ must exist as a non-directory.
-/// _to_ must be a non-existent file, whose parent directory exists.
-///
-/// # Errors
-///
-/// If the copy system call fails, we print a verbose error and return an empty error value.
-///
-fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> {
-    if b.compare && !need_copy(from, to, b) {
-        return Ok(());
-    }
-    // Declare the path here as we may need it for the verbose output below.
-    let backup_path = perform_backup(to, b)?;
-
-    copy_file(from, to)?;
-
+/// Apply post-copy operations: strip, ownership, permissions, timestamps, SELinux, and verbose output.
+fn finalize_installed_file(
+    from: &Path,
+    to: &Path,
+    b: &Behavior,
+    backup_path: Option<PathBuf>,
+) -> UResult<()> {
     #[cfg(not(windows))]
     if b.strip {
         strip_file(to, b)?;
@@ -1178,6 +1121,31 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> {
     Ok(())
 }
 
+/// Copy one file to a new location, changing metadata.
+///
+/// Returns a Result type with the Err variant containing the error message.
+///
+/// # Parameters
+///
+/// _from_ must exist as a non-directory.
+/// _to_ must be a non-existent file, whose parent directory exists.
+///
+/// # Errors
+///
+/// If the copy system call fails, we print a verbose error and return an empty error value.
+///
+fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> {
+    if b.compare && !need_copy(from, to, b) {
+        return Ok(());
+    }
+    // Declare the path here as we may need it for the verbose output below.
+    let backup_path = perform_backup(to, b)?;
+
+    copy_file(from, to)?;
+
+    finalize_installed_file(from, to, b, backup_path)
+}
+
 #[cfg(all(feature = "selinux", target_os = "linux"))]
 fn get_context_for_selinux(b: &Behavior) -> Option<&String> {
     if b.default_context {

src/uu/rm/src/platform/unix.rs

@@ -120,7 +120,7 @@ pub fn safe_remove_file(
     let parent = path.parent().unwrap_or(Path::new("."));
     let file_name = path.file_name()?;
 
-    let dir_fd = DirFd::open(parent).ok()?;
+    let dir_fd = DirFd::open(parent, true).ok()?;
 
     match dir_fd.unlink_at(file_name, false) {
         Ok(_) => {
@@ -151,7 +151,7 @@ pub fn safe_remove_empty_dir(
     let parent = path.parent().unwrap_or(Path::new("."));
     let dir_name = path.file_name()?;
 
-    let dir_fd = DirFd::open(parent).ok()?;
+    let dir_fd = DirFd::open(parent, true).ok()?;
 
     match dir_fd.unlink_at(dir_name, true) {
         Ok(_) => {
@@ -290,7 +290,7 @@ pub fn safe_remove_dir_recursive(
     };
 
     // Try to open the directory using DirFd for secure traversal
-    let dir_fd = match DirFd::open(path) {
+    let dir_fd = match DirFd::open(path, true) {
         Ok(fd) => fd,
         Err(e) => {
             // If we can't open the directory for safe traversal,
@@ -389,7 +389,7 @@ pub fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Opt
             }
 
             // Recursively remove subdirectory using safe traversal
-            let child_dir_fd = match dir_fd.open_subdir(&entry_name) {
+            let child_dir_fd = match dir_fd.open_subdir(&entry_name, true) {
                 Ok(fd) => fd,
                 Err(e) => {
                     // If we can't open the subdirectory for safe traversal,

src/uucore/src/lib/features/perms.rs

@@ -311,7 +311,7 @@ impl ChownExecutor {
             #[cfg(target_os = "linux")]
             let chown_result = if path.is_dir() {
                 // For directories on Linux, use safe traversal from the start
-                match DirFd::open(path) {
+                match DirFd::open(path, true) {
                     Ok(dir_fd) => self
                         .safe_chown_dir(&dir_fd, path, &meta)
                         .map(|_| String::new()),
@@ -537,7 +537,7 @@ impl ChownExecutor {
 
             // Recurse into subdirectories
             if meta.is_dir() && (follow || !meta.file_type().is_symlink()) {
-                match dir_fd.open_subdir(&entry_name) {
+                match dir_fd.open_subdir(&entry_name, true) {
                     Ok(subdir_fd) => {
                         self.safe_traverse_dir(&subdir_fd, &entry_path, ret);
                     }
@@ -695,7 +695,7 @@ impl ChownExecutor {
     /// Try to open directory with error reporting
     #[cfg(target_os = "linux")]
     fn try_open_dir(&self, path: &Path) -> Option<DirFd> {
-        DirFd::open(path)
+        DirFd::open(path, true)
             .map_err(|e| {
                 if self.verbosity.level != VerbosityLevel::Silent {
                     show_error!("cannot access {}: {}", path.quote(), strip_errno(&e));