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

Author: sylvestre

src/uu/cp/src/cp.rs

@@ -1711,8 +1711,6 @@ pub(crate) fn set_selinux_context(path: &Path, context: Option<&String>) -> Copy
 /// 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, skip_selinux: bool) -> CopyResult<()> {
     use std::fs::File;

src/uu/mv/src/mv.rs

@@ -15,8 +15,7 @@ use clap::{Arg, ArgAction, ArgMatches, Command};
 use indicatif::{MultiProgress, ProgressBar, ProgressStyle};
 
 #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
-use rustc_hash::FxHashMap;
-use rustc_hash::FxHashSet;
+use rustc_hash::{FxHashMap, FxHashSet};
 use std::env;
 use std::ffi::OsString;
 use std::fs;
@@ -967,7 +966,8 @@ 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;
@@ -989,12 +989,12 @@ fn rename_dir_fallback(
         display_manager,
     );
 
-    // Apply xattrs using file descriptor to avoid TOCTOU races
+    // Apply xattrs after directory contents are copied, ignoring ENOTSUP errors
+    // (filesystem doesn't support xattrs, which is acceptable for cross-device moves)
     #[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)?;
+    if let Err(e) = fsxattr::apply_xattrs(to, xattrs) {
+        if e.raw_os_error() != Some(libc::EOPNOTSUPP) {
+            return Err(e);
         }
     }
 
@@ -1063,8 +1063,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

@@ -2864,3 +2864,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());
+}