From 1e414f1ffff517902979ebde83c8a3be97cc3822 Mon Sep 17 00:00:00 2001 From: George Tokmaji Date: Mon, 30 Sep 2024 19:12:05 +0200 Subject: [PATCH 1/4] Win: Use `FILE_RENAME_FLAG_POSIX_SEMANTICS` for `std::fs::rename` if available Windows 10 1601 introduced `FileRenameInfoEx` as well as `FILE_RENAME_FLAG_POSIX_SEMANTICS`, allowing for atomic renaming. If it isn't supported, we fall back to `FileRenameInfo`. This commit also replicates `MoveFileExW`'s behavior of checking whether the source file is a mount point and moving the mount point instead of resolving the target path. --- library/std/src/fs.rs | 8 +- .../std/src/sys/pal/windows/c/bindings.txt | 3 + .../std/src/sys/pal/windows/c/windows_sys.rs | 16 ++ library/std/src/sys/pal/windows/fs.rs | 145 +++++++++++++++++- 4 files changed, 168 insertions(+), 4 deletions(-) diff --git a/library/std/src/fs.rs b/library/std/src/fs.rs index db7867337dd59..7850682f57b91 100644 --- a/library/std/src/fs.rs +++ b/library/std/src/fs.rs @@ -2171,12 +2171,14 @@ pub fn symlink_metadata>(path: P) -> io::Result { /// # Platform-specific behavior /// /// This function currently corresponds to the `rename` function on Unix -/// and the `MoveFileEx` function with the `MOVEFILE_REPLACE_EXISTING` flag on Windows. +/// and the `SetFileInformationByHandle` function on Windows. /// /// Because of this, the behavior when both `from` and `to` exist differs. On /// Unix, if `from` is a directory, `to` must also be an (empty) directory. If -/// `from` is not a directory, `to` must also be not a directory. In contrast, -/// on Windows, `from` can be anything, but `to` must *not* be a directory. +/// `from` is not a directory, `to` must also be not a directory. The behavior +/// on Windows is the same on Windows 10 1607 and higher if `FileRenameInfoEx` +/// is supported by the filesystem; otherwise, `from` can be anything, but +/// `to` must *not* be a directory. /// /// Note that, this [may change in the future][changes]. /// diff --git a/library/std/src/sys/pal/windows/c/bindings.txt b/library/std/src/sys/pal/windows/c/bindings.txt index 9c2e4500da068..964b6ba550db7 100644 --- a/library/std/src/sys/pal/windows/c/bindings.txt +++ b/library/std/src/sys/pal/windows/c/bindings.txt @@ -2295,6 +2295,7 @@ Windows.Win32.Storage.FileSystem.FILE_NAME_OPENED Windows.Win32.Storage.FileSystem.FILE_READ_ATTRIBUTES Windows.Win32.Storage.FileSystem.FILE_READ_DATA Windows.Win32.Storage.FileSystem.FILE_READ_EA +Windows.Win32.Storage.FileSystem.FILE_RENAME_INFO Windows.Win32.Storage.FileSystem.FILE_SHARE_DELETE Windows.Win32.Storage.FileSystem.FILE_SHARE_MODE Windows.Win32.Storage.FileSystem.FILE_SHARE_NONE @@ -2597,5 +2598,7 @@ Windows.Win32.System.Threading.WaitForMultipleObjects Windows.Win32.System.Threading.WaitForSingleObject Windows.Win32.System.Threading.WakeAllConditionVariable Windows.Win32.System.Threading.WakeConditionVariable +Windows.Win32.System.WindowsProgramming.FILE_RENAME_FLAG_POSIX_SEMANTICS +Windows.Win32.System.WindowsProgramming.FILE_RENAME_FLAG_REPLACE_IF_EXISTS Windows.Win32.System.WindowsProgramming.PROGRESS_CONTINUE Windows.Win32.UI.Shell.GetUserProfileDirectoryW diff --git a/library/std/src/sys/pal/windows/c/windows_sys.rs b/library/std/src/sys/pal/windows/c/windows_sys.rs index ab5f8919d7af6..d845145b4a4a7 100644 --- a/library/std/src/sys/pal/windows/c/windows_sys.rs +++ b/library/std/src/sys/pal/windows/c/windows_sys.rs @@ -2470,6 +2470,22 @@ pub const FILE_RANDOM_ACCESS: NTCREATEFILE_CREATE_OPTIONS = 2048u32; pub const FILE_READ_ATTRIBUTES: FILE_ACCESS_RIGHTS = 128u32; pub const FILE_READ_DATA: FILE_ACCESS_RIGHTS = 1u32; pub const FILE_READ_EA: FILE_ACCESS_RIGHTS = 8u32; +pub const FILE_RENAME_FLAG_POSIX_SEMANTICS: u32 = 2u32; +pub const FILE_RENAME_FLAG_REPLACE_IF_EXISTS: u32 = 1u32; +#[repr(C)] +#[derive(Clone, Copy)] +pub struct FILE_RENAME_INFO { + pub Anonymous: FILE_RENAME_INFO_0, + pub RootDirectory: HANDLE, + pub FileNameLength: u32, + pub FileName: [u16; 1], +} +#[repr(C)] +#[derive(Clone, Copy)] +pub union FILE_RENAME_INFO_0 { + pub ReplaceIfExists: BOOLEAN, + pub Flags: u32, +} pub const FILE_RESERVE_OPFILTER: NTCREATEFILE_CREATE_OPTIONS = 1048576u32; pub const FILE_SEQUENTIAL_ONLY: NTCREATEFILE_CREATE_OPTIONS = 4u32; pub const FILE_SESSION_AWARE: NTCREATEFILE_CREATE_OPTIONS = 262144u32; diff --git a/library/std/src/sys/pal/windows/fs.rs b/library/std/src/sys/pal/windows/fs.rs index aab471e28eaeb..2811625b1b508 100644 --- a/library/std/src/sys/pal/windows/fs.rs +++ b/library/std/src/sys/pal/windows/fs.rs @@ -1,5 +1,6 @@ use super::api::{self, WinError}; use super::{IoResult, to_u16s}; +use crate::alloc::{alloc, handle_alloc_error}; use crate::borrow::Cow; use crate::ffi::{OsStr, OsString, c_void}; use crate::io::{self, BorrowedCursor, Error, IoSlice, IoSliceMut, SeekFrom}; @@ -1095,7 +1096,149 @@ pub fn unlink(p: &Path) -> io::Result<()> { pub fn rename(old: &Path, new: &Path) -> io::Result<()> { let old = maybe_verbatim(old)?; let new = maybe_verbatim(new)?; - cvt(unsafe { c::MoveFileExW(old.as_ptr(), new.as_ptr(), c::MOVEFILE_REPLACE_EXISTING) })?; + + let new_len_without_nul_in_bytes = (new.len() - 1).try_into().unwrap(); + + let struct_size = mem::size_of::() - mem::size_of::() + + new.len() * mem::size_of::(); + + let struct_size: u32 = struct_size.try_into().unwrap(); + + let create_file = |extra_access, extra_flags| { + let handle = unsafe { + HandleOrInvalid::from_raw_handle(c::CreateFileW( + old.as_ptr(), + c::SYNCHRONIZE | c::DELETE | extra_access, + c::FILE_SHARE_READ | c::FILE_SHARE_WRITE | c::FILE_SHARE_DELETE, + ptr::null(), + c::OPEN_EXISTING, + c::FILE_ATTRIBUTE_NORMAL | c::FILE_FLAG_BACKUP_SEMANTICS | extra_flags, + ptr::null_mut(), + )) + }; + + OwnedHandle::try_from(handle).map_err(|_| io::Error::last_os_error()) + }; + + // The following code replicates `MoveFileEx`'s behavior as reverse-engineered from its disassembly. + // If `old` refers to a mount point, we move it instead of the target. + let handle = match create_file(c::FILE_READ_ATTRIBUTES, c::FILE_FLAG_OPEN_REPARSE_POINT) { + Ok(handle) => { + let mut file_attribute_tag_info: MaybeUninit = + MaybeUninit::uninit(); + + let result = unsafe { + cvt(c::GetFileInformationByHandleEx( + handle.as_raw_handle(), + c::FileAttributeTagInfo, + file_attribute_tag_info.as_mut_ptr().cast(), + mem::size_of::().try_into().unwrap(), + )) + }; + + if let Err(err) = result { + if err.raw_os_error() == Some(c::ERROR_INVALID_PARAMETER as _) + || err.raw_os_error() == Some(c::ERROR_INVALID_FUNCTION as _) + { + // `GetFileInformationByHandleEx` documents that not all underlying drivers support all file information classes. + // Since we know we passed the correct arguments, this means the underlying driver didn't understand our request; + // `MoveFileEx` proceeds by reopening the file without inhibiting reparse point behavior. + None + } else { + Some(Err(err)) + } + } else { + // SAFETY: The struct has been initialized by GetFileInformationByHandleEx + let file_attribute_tag_info = unsafe { file_attribute_tag_info.assume_init() }; + + if file_attribute_tag_info.FileAttributes & c::FILE_ATTRIBUTE_REPARSE_POINT != 0 + && file_attribute_tag_info.ReparseTag != c::IO_REPARSE_TAG_MOUNT_POINT + { + // The file is not a mount point: Reopen the file without inhibiting reparse point behavior. + None + } else { + // The file is a mount point: Don't reopen the file so that the mount point gets renamed. + Some(Ok(handle)) + } + } + } + // The underlying driver may not support `FILE_FLAG_OPEN_REPARSE_POINT`: Retry without it. + Err(err) if err.raw_os_error() == Some(c::ERROR_INVALID_PARAMETER as _) => None, + Err(err) => Some(Err(err)), + } + .unwrap_or_else(|| create_file(0, 0))?; + + // The last field of FILE_RENAME_INFO, the file name, is unsized. + // Therefore we need to subtract the size of one wide char. + let layout = core::alloc::Layout::from_size_align( + struct_size as _, + mem::align_of::(), + ) + .unwrap(); + + let file_rename_info = unsafe { alloc(layout) } as *mut c::FILE_RENAME_INFO; + + if file_rename_info.is_null() { + handle_alloc_error(layout); + } + + // SAFETY: file_rename_info is a non-null pointer pointing to memory allocated by the global allocator. + let mut file_rename_info = unsafe { Box::from_raw(file_rename_info) }; + + // SAFETY: We have allocated enough memory for a full FILE_RENAME_INFO struct and a filename. + unsafe { + (&raw mut (*file_rename_info).Anonymous).write(c::FILE_RENAME_INFO_0 { + // Don't bother with FileRenameInfo on Windows 7 since it doesn't exist. + #[cfg(not(target_vendor = "win7"))] + Flags: c::FILE_RENAME_FLAG_REPLACE_IF_EXISTS | c::FILE_RENAME_FLAG_POSIX_SEMANTICS, + #[cfg(target_vendor = "win7")] + ReplaceIfExists: 1, + }); + + (&raw mut (*file_rename_info).RootDirectory).write(ptr::null_mut()); + (&raw mut (*file_rename_info).FileNameLength).write(new_len_without_nul_in_bytes); + + new.as_ptr() + .copy_to_nonoverlapping((&raw mut (*file_rename_info).FileName) as *mut u16, new.len()); + } + + #[cfg(not(target_vendor = "win7"))] + const FileInformationClass: c::FILE_INFO_BY_HANDLE_CLASS = c::FileRenameInfoEx; + #[cfg(target_vendor = "win7")] + const FileInformationClass: c::FILE_INFO_BY_HANDLE_CLASS = c::FileRenameInfo; + + // We don't use `set_file_information_by_handle` here as `FILE_RENAME_INFO` is used for both `FileRenameInfo` and `FileRenameInfoEx`. + let result = unsafe { + cvt(c::SetFileInformationByHandle( + handle.as_raw_handle(), + FileInformationClass, + (&raw const *file_rename_info).cast::(), + struct_size, + )) + }; + + #[cfg(not(target_vendor = "win7"))] + if let Err(err) = result { + if err.raw_os_error() == Some(c::ERROR_INVALID_PARAMETER as _) { + // FileRenameInfoEx and FILE_RENAME_FLAG_POSIX_SEMANTICS were added in Windows 10 1607; retry with FileRenameInfo. + file_rename_info.Anonymous.ReplaceIfExists = 1; + + cvt(unsafe { + c::SetFileInformationByHandle( + handle.as_raw_handle(), + c::FileRenameInfo, + (&raw const *file_rename_info).cast::(), + struct_size, + ) + })?; + } else { + return Err(err); + } + } + + #[cfg(target_vendor = "win7")] + result?; + Ok(()) } From e2bb09467d28b5ff72f0c29f311d8a3b9059c1f7 Mon Sep 17 00:00:00 2001 From: George Tokmaji Date: Mon, 30 Sep 2024 19:49:18 +0200 Subject: [PATCH 2/4] Win: Add test cases for renaming a directory while the target file is opened and for renaming over a non-empty directory --- library/std/src/fs/tests.rs | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/library/std/src/fs/tests.rs b/library/std/src/fs/tests.rs index 0672fe6f7718a..e95f4b88c62ac 100644 --- a/library/std/src/fs/tests.rs +++ b/library/std/src/fs/tests.rs @@ -1766,3 +1766,44 @@ fn test_hidden_file_truncation() { let metadata = file.metadata().unwrap(); assert_eq!(metadata.len(), 0); } + +#[cfg(windows)] +#[test] +fn test_rename_file_over_open_file() { + // Make sure that std::fs::rename works if the target file is already opened with FILE_SHARE_DELETE. See #123985. + let tmpdir = tmpdir(); + + // Create source with test data to read. + let source_path = tmpdir.join("source_file.txt"); + fs::write(&source_path, b"source hello world").unwrap(); + + // Create target file with test data to read; + let target_path = tmpdir.join("target_file.txt"); + fs::write(&target_path, b"target hello world").unwrap(); + + // Open target file + let target_file = fs::File::open(&target_path).unwrap(); + + // Rename source + fs::rename(source_path, &target_path).unwrap(); + + core::mem::drop(target_file); + assert_eq!(fs::read(target_path).unwrap(), b"source hello world"); +} + +#[test] +#[cfg(windows)] +fn test_rename_directory_to_non_empty_directory() { + // Renaming a directory over a non-empty existing directory should fail on Windows. + let tmpdir: TempDir = tmpdir(); + + let source_path = tmpdir.join("source_directory"); + let target_path = tmpdir.join("target_directory"); + + fs::create_dir(&source_path).unwrap(); + fs::create_dir(&target_path).unwrap(); + + fs::write(target_path.join("target_file.txt"), b"target hello world").unwrap(); + + error!(fs::rename(source_path, target_path), 145); // ERROR_DIR_NOT_EMPTY +} From 8975a6dcf06a9348e4095c6766915326ae0ff17c Mon Sep 17 00:00:00 2001 From: George Tokmaji Date: Thu, 17 Oct 2024 00:27:46 +0200 Subject: [PATCH 3/4] Win: Remove special casing of the win7 target for `std::fs::rename` --- library/std/src/sys/pal/windows/fs.rs | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/library/std/src/sys/pal/windows/fs.rs b/library/std/src/sys/pal/windows/fs.rs index 2811625b1b508..272d1091d9463 100644 --- a/library/std/src/sys/pal/windows/fs.rs +++ b/library/std/src/sys/pal/windows/fs.rs @@ -1188,11 +1188,7 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> { // SAFETY: We have allocated enough memory for a full FILE_RENAME_INFO struct and a filename. unsafe { (&raw mut (*file_rename_info).Anonymous).write(c::FILE_RENAME_INFO_0 { - // Don't bother with FileRenameInfo on Windows 7 since it doesn't exist. - #[cfg(not(target_vendor = "win7"))] Flags: c::FILE_RENAME_FLAG_REPLACE_IF_EXISTS | c::FILE_RENAME_FLAG_POSIX_SEMANTICS, - #[cfg(target_vendor = "win7")] - ReplaceIfExists: 1, }); (&raw mut (*file_rename_info).RootDirectory).write(ptr::null_mut()); @@ -1202,22 +1198,16 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> { .copy_to_nonoverlapping((&raw mut (*file_rename_info).FileName) as *mut u16, new.len()); } - #[cfg(not(target_vendor = "win7"))] - const FileInformationClass: c::FILE_INFO_BY_HANDLE_CLASS = c::FileRenameInfoEx; - #[cfg(target_vendor = "win7")] - const FileInformationClass: c::FILE_INFO_BY_HANDLE_CLASS = c::FileRenameInfo; - // We don't use `set_file_information_by_handle` here as `FILE_RENAME_INFO` is used for both `FileRenameInfo` and `FileRenameInfoEx`. let result = unsafe { cvt(c::SetFileInformationByHandle( handle.as_raw_handle(), - FileInformationClass, + c::FileRenameInfoEx, (&raw const *file_rename_info).cast::(), struct_size, )) }; - #[cfg(not(target_vendor = "win7"))] if let Err(err) = result { if err.raw_os_error() == Some(c::ERROR_INVALID_PARAMETER as _) { // FileRenameInfoEx and FILE_RENAME_FLAG_POSIX_SEMANTICS were added in Windows 10 1607; retry with FileRenameInfo. @@ -1236,9 +1226,6 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> { } } - #[cfg(target_vendor = "win7")] - result?; - Ok(()) } From bfadeeb45cf25b996a50e3393967dac460d5cd53 Mon Sep 17 00:00:00 2001 From: George Tokmaji Date: Tue, 29 Oct 2024 14:14:06 +0100 Subject: [PATCH 4/4] Win: rename: Use offset_of! in struct size calculation --- library/std/src/sys/pal/windows/fs.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/pal/windows/fs.rs b/library/std/src/sys/pal/windows/fs.rs index 272d1091d9463..62f784f1e8e72 100644 --- a/library/std/src/sys/pal/windows/fs.rs +++ b/library/std/src/sys/pal/windows/fs.rs @@ -1099,8 +1099,13 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> { let new_len_without_nul_in_bytes = (new.len() - 1).try_into().unwrap(); - let struct_size = mem::size_of::() - mem::size_of::() - + new.len() * mem::size_of::(); + // The last field of FILE_RENAME_INFO, the file name, is unsized, + // and FILE_RENAME_INFO has two padding bytes. + // Therefore we need to make sure to not allocate less than + // size_of::() bytes, which would be the case with + // 0 or 1 character paths + a null byte. + let struct_size = mem::size_of::() + .max(mem::offset_of!(c::FILE_RENAME_INFO, FileName) + new.len() * mem::size_of::()); let struct_size: u32 = struct_size.try_into().unwrap(); @@ -1168,8 +1173,6 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> { } .unwrap_or_else(|| create_file(0, 0))?; - // The last field of FILE_RENAME_INFO, the file name, is unsized. - // Therefore we need to subtract the size of one wide char. let layout = core::alloc::Layout::from_size_align( struct_size as _, mem::align_of::(),