mv,cp: fix xattr and symlink handling in cross-device operations

Author: sylvestre

src/uu/cp/src/cp.rs

@@ -16,7 +16,7 @@ use std::os::unix::net::UnixListener;
 use std::path::{Path, PathBuf, StripPrefixError};
 use std::{fmt, io};
 #[cfg(all(unix, not(target_os = "android")))]
-use uucore::fsxattr::copy_xattrs_fd;
+use uucore::fsxattr::copy_xattrs;
 use uucore::translate;
 
 use clap::{Arg, ArgAction, ArgMatches, Command, builder::ValueParser, value_parser};
@@ -1675,13 +1675,8 @@ fn handle_preserve<F: Fn() -> CopyResult<()>>(p: &Preserve, f: F) -> CopyResult<
 /// user-writable if needed and restoring its original permissions afterward. This avoids "Operation
 /// not permitted" errors on read-only files. Returns an error if permission or metadata operations fail,
 /// or if xattr copying fails.
-///
-/// Uses file descriptor-based operations to avoid TOCTOU races during xattr copying.
 #[cfg(all(unix, not(target_os = "android")))]
 fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> {
-    use std::fs::File;
-    use uucore::fsxattr::copy_xattrs;
-
     let metadata = fs::symlink_metadata(dest)?;
 
     // Check if the destination file is currently read-only for the user.
@@ -1695,18 +1690,9 @@ fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> {
         fs::set_permissions(dest, perms)?;
     }
 
-    // Use file descriptor-based operations for regular files to avoid TOCTOU races.
-    // For directories and symlinks, we must use path-based operations since:
-    // - Directories cannot be opened with write mode for xattr operations
-    // - Symlinks (especially dangling ones) cannot be opened via File::open
-    let copy_xattrs_result = if metadata.is_file() {
-        // Open both files to pin their inodes, avoiding TOCTOU races during xattr operations
-        let source_file = File::open(source)?;
-        let dest_file = OpenOptions::new().write(true).open(dest)?;
-        copy_xattrs_fd(&source_file, &dest_file)
-    } else {
-        copy_xattrs(source, dest)
-    };
+    // Perform the xattr copy and capture any potential error,
+    // so we can restore permissions before returning.
+    let copy_xattrs_result = copy_xattrs(source, dest);
 
     // Restore read-only if we changed it.
     if was_readonly {

src/uu/mv/src/mv.rs

@@ -975,14 +975,10 @@ fn rename_dir_fallback(
         (_, _) => None,
     };
 
-    // Retrieve xattrs using file descriptor to avoid TOCTOU races
+    // Retrieve xattrs before copying (directories use path-based operations
+    // since they cannot be opened in write mode for xattr operations)
     #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
-    let xattrs = {
-        use std::fs::File;
-        File::open(from)
-            .and_then(|f| fsxattr::retrieve_xattrs_fd(&f))
-            .unwrap_or_else(|_| HashMap::new())
-    };
+    let xattrs = fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| HashMap::new());
 
     // Use directory copying (with or without hardlink support)
     let result = copy_dir_contents(
@@ -997,14 +993,9 @@ fn rename_dir_fallback(
         display_manager,
     );
 
-    // Apply xattrs using file descriptor to avoid TOCTOU races
+    // Apply xattrs after directory contents are copied
     #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
-    {
-        use std::fs::OpenOptions;
-        if let Ok(f) = OpenOptions::new().write(true).open(to) {
-            fsxattr::apply_xattrs_fd(&f, xattrs)?;
-        }
-    }
+    fsxattr::apply_xattrs(to, xattrs)?;
 
     result?;
 
@@ -1071,8 +1062,35 @@ fn copy_dir_contents_recursive(
             pb.set_message(from_path.to_string_lossy().to_string());
         }
 
-        if from_path.is_dir() {
-            // Recursively copy subdirectory
+        if from_path.is_symlink() {
+            // Handle symlinks first, before checking is_dir() which follows symlinks.
+            // This prevents symlinks to directories from being expanded into full copies.
+            #[cfg(unix)]
+            {
+                copy_file_with_hardlinks_helper(
+                    &from_path,
+                    &to_path,
+                    hardlink_tracker,
+                    hardlink_scanner,
+                )?;
+            }
+            #[cfg(not(unix))]
+            {
+                rename_symlink_fallback(&from_path, &to_path)?;
+            }
+
+            // Print verbose message for symlink
+            if verbose {
+                let message = translate!("mv-verbose-renamed", "from" => from_path.quote(), "to" => to_path.quote());
+                match display_manager {
+                    Some(pb) => pb.suspend(|| {
+                        println!("{message}");
+                    }),
+                    None => println!("{message}"),
+                }
+            }
+        } else if from_path.is_dir() {
+            // Recursively copy subdirectory (only real directories, not symlinks)
             fs::create_dir_all(&to_path)?;
 
             // Print verbose message for directory

tests/by-util/test_mv.rs

@@ -2851,3 +2851,54 @@ fn test_mv_xattr_enotsup_silent() {
         std::fs::remove_file("/dev/shm/mv_test").ok();
     }
 }
+
+/// Test that symlinks inside directories are preserved during cross-device moves
+/// (not expanded into full copies of their targets)
+#[test]
+#[cfg(target_os = "linux")]
+fn test_mv_cross_device_symlink_preserved() {
+    use std::fs;
+    use std::os::unix::fs::symlink;
+    use tempfile::TempDir;
+
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // Create a directory with a symlink to /etc inside
+    at.mkdir("src_dir");
+    at.write("src_dir/local.txt", "local content");
+    symlink("/etc", at.plus("src_dir/etc_link")).expect("Failed to create symlink");
+
+    // Verify initial state
+    assert!(at.is_symlink("src_dir/etc_link"));
+
+    // Force cross-filesystem move using /dev/shm (tmpfs)
+    let target_dir =
+        TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm");
+    let target_path = target_dir.path().join("dst_dir");
+
+    scene
+        .ucmd()
+        .arg("src_dir")
+        .arg(target_path.to_str().unwrap())
+        .succeeds()
+        .no_stderr();
+
+    // Verify source was removed
+    assert!(!at.dir_exists("src_dir"));
+
+    // Verify the symlink was preserved (not expanded)
+    let moved_symlink = target_path.join("etc_link");
+    assert!(
+        moved_symlink.is_symlink(),
+        "etc_link should still be a symlink after cross-device move"
+    );
+    assert_eq!(
+        fs::read_link(&moved_symlink).expect("Failed to read symlink"),
+        std::path::Path::new("/etc"),
+        "symlink should still point to /etc"
+    );
+
+    // Verify the regular file was also moved
+    assert!(target_path.join("local.txt").exists());
+}