Win: Fix std::fs::rename failing on Windows Server by attempting the non-atomic rename first #138133
Conversation
|
r? @ChrisDenton rustbot has assigned @ChrisDenton. Use |
rustbot
added
O-windows
|
☔ The latest upstream changes (presumably #138208) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ok, I've been testing this on many Windows versions and on different filesystem. I think the better approach is to test for the following error codes:
|
What are the non-atomic semantics on windows? Unix software generally expects renames to be atomic. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
@Fulgen301 |
|
I'll fix it up before the week is over. |
In addition to ERROR_INVALID_PARAMETER for Windows versions older than Windows 10 1607, some Windows versions may also fail with ERROR_INVALID_FUNCTION or ERROR_NOT_SUPPORTED to indicate that atomic rename is not supported. Fallback to FileRenameInfo in those cases.
| // This is a dynamically sized struct so we need to get the position of the last field to calculate the actual size. | ||
| let Ok(new_len_without_nul_in_bytes): Result<u32, _> = ((new.count_bytes() - 1) * 2).try_into() | ||
| else { | ||
| return Err(WinError::INVALID_PARAMETER).io_result(); |
There was a problem hiding this comment.
Probably ERROR_FILENAME_EXCED_RANGE would be better or else a custom error message. Even if it's not strictly what happened before.
| 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))?; |
There was a problem hiding this comment.
Looking at the error codes that are handled I worry this doesn't account for reparse points that cannot be opened, not because they're aren't supported but because they cannot be interpreted by CreateFile. E.g. app execution aliases aren't actually links in the filesystem sense and can only be interpreted by CreateProcess. We should still be able to rename them though.
| }; | ||
|
|
||
| unsafe { | ||
| dealloc(file_rename_info.cast::<u8>(), layout); |
There was a problem hiding this comment.
This is now fairly distant from the alloc. While nothing returns or panics before this atm, I wouldn't be surprised if some refactoring in the future missed this subtlety. So I would be happier with a drop guard or some other custom struct that is guaranteed to free memory wherever the scope ends.
We previously attempted the atomic rename first, and fell back to trying a non-atomic rename if it wasn't supported. However, this does not work reliably on Windows Server - NTFS partitions created in Windows Server 2016 apparently need to be upgraded, and it outright fails with
ERROR_NOT_SUPPORTEDon ReFS on Windows Server 2022.This commit switches the two calls so that FileRenameInfo is tried first, and an atomic rename via FileRenameInfoEx is only attempted if the non-atomic rename fails.
Fix #137499.
Fix #137971.
This PR works similarly to #137528, but is a cleaner fix in my opinion and avoids additional syscalls to reopen the file if
MoveFileExfails.