Skip to content

Windows: remove readonly files #134679

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 9, 2025
Merged

Windows: remove readonly files #134679

merged 3 commits into from
Feb 9, 2025

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Dec 23, 2024 •
edited by matthiaskrgr
Loading

When calling remove_file, we shouldn't fail to delete readonly files. As the test makes clear, this make the Windows behaviour consistent with other platforms. This also makes us internally consistent with remove_dir_all.

try-job: x86_64-msvc-ext1

@rustbot
Copy link
Collaborator

rustbot commented Dec 23, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 Dec 23, 2024
@tbu-
Copy link
Contributor

tbu- commented Dec 26, 2024

Could you add that to the platform-specific behavior section of fs::remove?

@ChrisDenton
Copy link
Member Author

I've added a note about which functions are used.

Mark-Simulacrum
Mark-Simulacrum reviewed Dec 27, 2024
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Do we need libs/libs-api FCP of some kind given the new (at least implicit) guarantee? The impl seems fine but I don't have the Windows knowledge to thoroughly check it.

library/std/src/sys/pal/windows/fs.rs Outdated
let mut opts = OpenOptions::new();
opts.access_mode(c::DELETE);
opts.custom_flags(c::FILE_FLAG_OPEN_REPARSE_POINT);
if File::open_native(&p_u16s, &opts).map(|f| f.posix_delete()).is_ok() {
Copy link
Member

Choose a reason for hiding this comment

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

Could this cause concurrent deletes of the same file to fail due to having an open handle (iirc windows doesn't let you delete files with open handles?)? I guess before it would also fail with the same error, so not sure that matters even if true...

Copy link
Member Author

Choose a reason for hiding this comment

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

DeleteFile does also open the file in a similar way to we're doing so there's actually no change there.

iirc windows doesn't let you delete files with open handles

That's not quite right. When opening files there's a "sharing mode" that lets you allow others to open the same file. One of those modes is FILE_SHARE_DELETE. If someone opens a file without that share mode then it will prevent any deletions no matter what we do.

Deletes can also race with each other because there's a time between marking a file for deletion and closing the handle where the file is in a kind of limbo state. It can't be opened but it still exists on the filesystem.

But either case is the same whether we use DeleteFile or open the file ourselves.

Copy link
Member

Choose a reason for hiding this comment

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

I see, sounds good. Thanks for clarifying! :)

@ChrisDenton
Copy link
Member Author

Do we need libs/libs-api FCP of some kind given the new (at least implicit) guarantee

I'll add a label to see if libs-api want to discuss it. This aligns the behaviour with other platforms so I think it's worthwhile.

@ChrisDenton ChrisDenton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 27, 2024
@ChrisDenton
Copy link
Member Author

Oh, I should write a summary of the question being asked of libs-api

@rust-lang/libs-api on platforms except Windows, read-only files are deleted when calling remove_file. On Windows support for doing this was only added relatively recently and is not done by the normal DeleteFile function.

This PR changes remove_file so that it will delete read-only files if possible. It does this by reusing code we already use in remove_dir_all.

Does this new (for Windows) behaviour require a libs-api FCP?

@dtolnay
Copy link
Member

dtolnay commented Dec 27, 2024

@rustbot ping windows
See summary in the previous comment. Please point out if there is some reason this wouldn't be desirable.

@rustbot
Copy link
Collaborator

rustbot commented Dec 27, 2024

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @albertlarsan68 @arlosi @ChrisDenton @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @sivadeilra @wesleywiser

@dtolnay
Copy link
Member

dtolnay commented Dec 27, 2024

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 27, 2024 •
edited by Amanieu
Loading

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 27, 2024
@Amanieu Amanieu removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 14, 2025
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 14, 2025
@rfcbot
Copy link

rfcbot commented Jan 14, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 14, 2025
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 24, 2025
@rfcbot
Copy link

rfcbot commented Jan 24, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jan 24, 2025
@rustbot rustbot mentioned this pull request Jan 24, 2025
Closed
3 tasks
@ChrisDenton
Copy link
Member Author

The libs-api team discussed this, the RFC is now complete and Mark said the implementation looked fine so...

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jan 24, 2025

📌 Commit 6ffc9ca has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 8, 2025

📌 Commit 962ebf0 has been approved by Mark-Simulacrum

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 Feb 8, 2025
This was referenced Feb 8, 2025
Closed
Merged
@bors bors merged commit 3418247 into rust-lang:master Feb 9, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 9, 2025
@ChrisDenton ChrisDenton deleted the rm-readonly branch February 9, 2025 17:34
@RalfJung RalfJung mentioned this pull request Feb 10, 2025
Merged
@cuviper cuviper mentioned this pull request Mar 21, 2025
Closed
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.