mv: Make mv command fallback to copy only if the src and dst are on different device

hamflx · hamflx · commit 616e4223bb86 · 2025-02-06T15:43:54.000+08:00
diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt
@@ -136,6 +136,7 @@ vmsplice
 
 # * vars/libc
 COMFOLLOW
+EXDEV
 FILENO
 FTSENT
 HOSTSIZE
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/src/uu/mv/Cargo.toml b/src/uu/mv/Cargo.toml
@@ -28,6 +28,16 @@ uucore = { workspace = true, features = [
 ] }
 thiserror = { workspace = true }
 
+[target.'cfg(windows)'.dependencies]
+windows-sys = { workspace = true, features = [
+  "Win32_Foundation",
+  "Win32_Security",
+  "Win32_Storage_FileSystem",
+] }
+
+[target.'cfg(unix)'.dependencies]
+libc = { workspace = true }
+
 [[bin]]
 name = "mv"
 path = "src/main.rs"
diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs
@@ -657,7 +657,22 @@ fn rename_with_fallback(
     to: &Path,
     multi_progress: Option<&MultiProgress>,
 ) -> io::Result<()> {
-    if fs::rename(from, to).is_err() {
+    if let Err(err) = fs::rename(from, to) {
+        #[cfg(windows)]
+        const EXDEV: i32 = windows_sys::Win32::Foundation::ERROR_NOT_SAME_DEVICE as _;
+        #[cfg(unix)]
+        const EXDEV: i32 = libc::EXDEV as _;
+
+        // We will only copy if:
+        // 1. Files are on different devices (EXDEV error)
+        // 2. On Windows, if the target file exists and source file is opened by another process
+        //    (MoveFileExW fails with "Access Denied" even if the source file has FILE_SHARE_DELETE permission)
+        let should_fallback = matches!(err.raw_os_error(), Some(EXDEV))
+            || (from.is_file() && can_delete_file(from).unwrap_or(false));
+        if !should_fallback {
+            return Err(err);
+        }
+
         // Get metadata without following symlinks
         let metadata = from.symlink_metadata()?;
         let file_type = metadata.file_type();
@@ -792,3 +807,55 @@ fn is_empty_dir(path: &Path) -> bool {
         Err(_e) => false,
     }
 }
+
+/// Checks if a file can be deleted by attempting to open it with delete permissions.
+#[cfg(windows)]
+fn can_delete_file(path: &Path) -> Result<bool, io::Error> {
+    use std::{
+        os::windows::ffi::OsStrExt as _,
+        ptr::{null, null_mut},
+    };
+
+    use windows_sys::Win32::{
+        Foundation::{CloseHandle, INVALID_HANDLE_VALUE},
+        Storage::FileSystem::{
+            CreateFileW, DELETE, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_DELETE, FILE_SHARE_READ,
+            FILE_SHARE_WRITE, OPEN_EXISTING,
+        },
+    };
+
+    let wide_path = path
+        .as_os_str()
+        .encode_wide()
+        .chain([0])
+        .collect::<Vec<u16>>();
+
+    let handle = unsafe {
+        CreateFileW(
+            wide_path.as_ptr(),
+            DELETE,
+            FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,
+            null(),
+            OPEN_EXISTING,
+            FILE_ATTRIBUTE_NORMAL,
+            null_mut(),
+        )
+    };
+
+    if handle == INVALID_HANDLE_VALUE {
+        return Err(io::Error::last_os_error());
+    }
+
+    unsafe { CloseHandle(handle) };
+
+    Ok(true)
+}
+
+#[cfg(not(windows))]
+fn can_delete_file(_: &Path) -> Result<bool, io::Error> {
+    // On non-Windows platforms, always return false to indicate that we don't need
+    // to try the copy+delete fallback. This is because on Unix-like systems,
+    // rename() failing with errors other than EXDEV means the operation cannot
+    // succeed even with a copy+delete approach (e.g. permission errors).
+    Ok(false)
+}
diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs
@@ -49,6 +49,22 @@ fn test_mv_rename_file() {
     assert!(at.file_exists(file2));
 }
 
+#[test]
+fn test_mv_with_source_file_opened_and_target_file_exists() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    let src = "source_file_opened";
+    let dst = "target_file_exists";
+
+    let f = at.make_file(src);
+
+    at.touch(dst);
+
+    ucmd.arg(src).arg(dst).succeeds().no_stderr().no_stdout();
+
+    drop(f);
+}
+
 #[test]
 fn test_mv_move_file_into_dir() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -1670,6 +1686,51 @@ fn test_acl() {
     assert!(compare_xattrs(&file, &file_target));
 }
 
+#[test]
+#[cfg(windows)]
+fn test_move_should_not_fallback_to_copy() {
+    use std::os::windows::fs::OpenOptionsExt;
+
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    let locked_file = "a_file_is_locked";
+    let locked_file_path = at.plus(locked_file);
+    let file = std::fs::OpenOptions::new()
+        .create(true)
+        .write(true)
+        .share_mode(
+            uucore::windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ
+                | uucore::windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE,
+        )
+        .open(locked_file_path);
+
+    let target_file = "target_file";
+    ucmd.arg(locked_file).arg(target_file).fails();
+
+    assert!(at.file_exists(locked_file));
+    assert!(!at.file_exists(target_file));
+
+    drop(file);
+}
+
+#[test]
+#[cfg(unix)]
+fn test_move_should_not_fallback_to_copy() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    let readonly_dir = "readonly_dir";
+    let locked_file = "readonly_dir/a_file_is_locked";
+    at.mkdir(readonly_dir);
+    at.touch(locked_file);
+    at.set_mode(readonly_dir, 0o555);
+
+    let target_file = "target_file";
+    ucmd.arg(locked_file).arg(target_file).fails();
+
+    assert!(at.file_exists(locked_file));
+    assert!(!at.file_exists(target_file));
+}
+
 // Todo:
 
 // $ at.touch a b
