Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Win: Use POSIX rename semantics for std::fs::rename if available #131072

Merged
merged 4 commits into from
Dec 22, 2024

Conversation

Fulgen301
Copy link
Contributor

Windows 10 1601 introduced FileRenameInfoEx as well as FILE_RENAME_FLAG_POSIX_SEMANTICS, allowing for atomic renaming and renaming if the target file is has already been opened with FILE_SHARE_DELETE, in which case the file gets renamed on disk while the open file handle still refers to the old file, just like in POSIX. This resolves #123985, where atomic renaming proved difficult to impossible due to race conditions.

If FileRenameInfoEx isn't available due to missing support from the underlying filesystem or missing OS support, the renaming is retried with FileRenameInfo, which matches the behavior of MoveFileEx.

This PR also manually replicates parts of MoveFileEx's internal logic, as reverse-engineered from the disassembly: If the source file is a reparse point and said reparse point is a mount point, the mount point itself gets renamed; otherwise the reparse point is resolved and the result renamed.

Notes:

  • Currently, the win7 target doesn't bother with FileRenameInfoEx at all; it's probably desirable to remove that special casing and try FileRenameInfoEx anyway if it doesn't exist, in case the binary is run on newer OS versions.

Fixes #123985

…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.
… opened and for renaming over a non-empty directory
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 30, 2024
@ChrisDenton
Copy link
Member

This looks good to me.

Currently, the win7 target doesn't bother with FileRenameInfoEx at all; it's probably desirable to remove that special casing and try FileRenameInfoEx anyway if it doesn't exist, in case the binary is run on newer OS versions.

Yes, I'd prefer if we just did that. Doing so doesn't appear to require anything special and the less the win7 target diverges the better.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is looking good. Just a couple of nits.

library/std/src/sys/pal/windows/fs.rs Outdated Show resolved Hide resolved
library/std/src/sys/pal/windows/fs.rs Outdated Show resolved Hide resolved
@ChrisDenton
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2024

📌 Commit bfadeeb has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2024
DianQK added a commit to DianQK/rust that referenced this pull request Dec 21, 2024
…antics, r=ChrisDenton

Win: Use POSIX rename semantics for `std::fs::rename` if available

Windows 10 1601 introduced `FileRenameInfoEx` as well as `FILE_RENAME_FLAG_POSIX_SEMANTICS`, allowing for atomic renaming and renaming if the target file is has already been opened with `FILE_SHARE_DELETE`, in which case the file gets renamed on disk while the open file handle still refers to the old file, just like in POSIX. This resolves rust-lang#123985, where atomic renaming proved difficult to impossible due to race conditions.

If `FileRenameInfoEx` isn't available due to missing support from the underlying filesystem or missing OS support, the renaming is retried with `FileRenameInfo`, which matches the behavior of `MoveFileEx`.

This PR also manually replicates parts of `MoveFileEx`'s internal logic, as reverse-engineered from the disassembly: If the source file is a reparse point and said reparse point is a mount point, the mount point itself gets renamed; otherwise the reparse point is resolved and the result renamed.

Notes:
- Currently, the `win7` target doesn't bother with `FileRenameInfoEx` at all; it's probably desirable to remove that special casing and try `FileRenameInfoEx` anyway if it doesn't exist, in case the binary is run on newer OS versions.

Fixes rust-lang#123985
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 21, 2024
…antics, r=ChrisDenton

Win: Use POSIX rename semantics for `std::fs::rename` if available

Windows 10 1601 introduced `FileRenameInfoEx` as well as `FILE_RENAME_FLAG_POSIX_SEMANTICS`, allowing for atomic renaming and renaming if the target file is has already been opened with `FILE_SHARE_DELETE`, in which case the file gets renamed on disk while the open file handle still refers to the old file, just like in POSIX. This resolves rust-lang#123985, where atomic renaming proved difficult to impossible due to race conditions.

If `FileRenameInfoEx` isn't available due to missing support from the underlying filesystem or missing OS support, the renaming is retried with `FileRenameInfo`, which matches the behavior of `MoveFileEx`.

This PR also manually replicates parts of `MoveFileEx`'s internal logic, as reverse-engineered from the disassembly: If the source file is a reparse point and said reparse point is a mount point, the mount point itself gets renamed; otherwise the reparse point is resolved and the result renamed.

Notes:
- Currently, the `win7` target doesn't bother with `FileRenameInfoEx` at all; it's probably desirable to remove that special casing and try `FileRenameInfoEx` anyway if it doesn't exist, in case the binary is run on newer OS versions.

Fixes rust-lang#123985
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#131072 (Win: Use POSIX rename semantics for `std::fs::rename` if available)
 - rust-lang#134325 (Correctly document CTFE behavior of is_null and methods that call is_null.)
 - rust-lang#134526 (update `rustc_index_macros` feature handling)
 - rust-lang#134581 (Bump Fuchsia toolchain for testing)
 - rust-lang#134607 (on pair → on par)
 - rust-lang#134608 (Move test rust-lang#93775 to crashes)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#131072 (Win: Use POSIX rename semantics for `std::fs::rename` if available)
 - rust-lang#134325 (Correctly document CTFE behavior of is_null and methods that call is_null.)
 - rust-lang#134526 (update `rustc_index_macros` feature handling)
 - rust-lang#134581 (Bump Fuchsia toolchain for testing)
 - rust-lang#134607 (on pair → on par)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 51df98d into rust-lang:master Dec 22, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2024
Rollup merge of rust-lang#131072 - Fulgen301:windows-rename-posix-semantics, r=ChrisDenton

Win: Use POSIX rename semantics for `std::fs::rename` if available

Windows 10 1601 introduced `FileRenameInfoEx` as well as `FILE_RENAME_FLAG_POSIX_SEMANTICS`, allowing for atomic renaming and renaming if the target file is has already been opened with `FILE_SHARE_DELETE`, in which case the file gets renamed on disk while the open file handle still refers to the old file, just like in POSIX. This resolves rust-lang#123985, where atomic renaming proved difficult to impossible due to race conditions.

If `FileRenameInfoEx` isn't available due to missing support from the underlying filesystem or missing OS support, the renaming is retried with `FileRenameInfo`, which matches the behavior of `MoveFileEx`.

This PR also manually replicates parts of `MoveFileEx`'s internal logic, as reverse-engineered from the disassembly: If the source file is a reparse point and said reparse point is a mount point, the mount point itself gets renamed; otherwise the reparse point is resolved and the result renamed.

Notes:
- Currently, the `win7` target doesn't bother with `FileRenameInfoEx` at all; it's probably desirable to remove that special casing and try `FileRenameInfoEx` anyway if it doesn't exist, in case the binary is run on newer OS versions.

Fixes rust-lang#123985
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std::fs::rename sometimes fails on Windows due to missing FILE_RENAME_POSIX_SEMANTICS
4 participants