From b96f825503f6a831dd5af5549e4c8e28a75470a5 Mon Sep 17 00:00:00 2001 From: hamflx Date: Fri, 1 Mar 2024 20:46:21 +0800 Subject: [PATCH] mv: Make mv command fallback to copy only if the src and dst are on different device --- .../workspace.wordlist.txt | 1 + Cargo.lock | 2 + src/uu/mv/Cargo.toml | 10 +++ src/uu/mv/src/mv.rs | 69 ++++++++++++++++++- tests/by-util/test_mv.rs | 61 ++++++++++++++++ 5 files changed, 142 insertions(+), 1 deletion(-) diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index ee34a38110e..45373d95c72 100644 --- 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 index 83fe0dc78e6..765114901ec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3009,8 +3009,10 @@ dependencies = [ "clap", "fs_extra", "indicatif", + "libc", "thiserror 2.0.11", "uucore", + "windows-sys 0.59.0", ] [[package]] diff --git a/src/uu/mv/Cargo.toml b/src/uu/mv/Cargo.toml index 13b40f7fb0c..45d1b194226 100644 --- 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 index 6e533dace85..2334c37df14 100644 --- 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 { + 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::>(); + + 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 { + // 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 index 6441357f12e..2454a0a03cd 100644 --- 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