Fix Windows bootstrap panic on invalid symlink removal (issue #143045) by hasip-timurtas · Pull Request #143052 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fix Windows bootstrap panic on invalid symlink removal (issue #143045)
On Windows, use fs::remove_file for symlinks instead of fs::remove_dir to avoid error 267 when the symlink target is invalid. fs::remove_file can handle both file and directory symlinks/junctions, even when their targets don't exist.
r? @ghost
rustbot has assigned @albertlarsan68.
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
@hasip-timurtas Please make your PR description concise so your reviewer does not waste time reading it. It should only need to be about two sentences for a change of this size.
Oh, your original commit message contains r? @ghost
That's something for guiding rustbot. You need to put it in your PR description for it to work. I can help, though, by unassigning your reviewer, since r? @ghost is for a PR that you do not intend to have reviewed.
Failed to set assignee to ghost: invalid assignee
Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.
@hasip-timurtas Yes, that looks good to me. You may want to remove the r? @ghost from your commit message but a final decision there is up to the reviewer. Feel free to use r? again for your previous reviewer and to @rustbot ready.
- Corrected a typo in the error message from "Couldn’t" to "Couldn't".
- Enhanced the logic for removing symlinks on Windows to handle invalid targets more gracefully by attempting to remove them as files before falling back to directory removal.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is incorrect and I also don't think the code is the correct fix. Both remove_file and remove_dir can remove symlinks. However, remove_dir can only remove directory symlinks.
- Simplified the handling of symlinks on Windows by removing the fallback to directory removal. The
fs::remove_filefunction now directly handles both file and directory symlinks, even when the target is invalid, improving code clarity and reliability.
@ChrisDenton Thank you for the feedback
After digging into the Windows implementation, I found that fs::remove_file actually has special handling for this case. When it fails with ERROR_ACCESS_DENIED, it retries with FILE_FLAG_OPEN_REPARSE_POINT to handle the symlink directly (see library/std/src/sys/fs/windows.rs:1228-1235).
This is why it works on invalid symlinks - it doesn't try to follow them. fs::remove_dir doesn't have this fallback, which causes error 267.
Since fs::remove_file handles both file and directory symlinks on Windows, it's the right choice here. Happy to add a comment explaining this if you think it'd be helpful.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Since
fs::remove_filehandles both file and directory symlinks on Windows, it's the right choice here.
Sorry, I think you've misunderstood what that code is doing. Both DeleteFile and the fallback will open the symlink itself not the target. There is no issue if the target does not exist. The fallback is only there for readonly files that DeleteFile can't handle. The problem here is different.
On Windows there are two types of symlink: directory and non-directory. remove_dir can delete directory symlinks and remove_file can delete non-directory symlinks. Mixing them up will not work. Whether or not a symlink is a directory or non-directory symlink is a property of the symlink itself and nothing to do with the target.
So I would prefer this be implemented something like: if remove_dir fails with ErrorKind::NotADirectory then retry with remove_file.
- Updated the symlink removal process on Windows to first attempt removing directory symlinks with
fs::remove_dir, and fallback tofs::remove_filefor file symlinks. This change improves error handling and clarity in the code.
@ChrisDenton Thank you for the clarification. I've implemented your suggested approach, but I'd like to confirm a few details to ensure we're handling all scenarios correctly:
- Will
fs::remove_diron a directory symlink with an invalid target still succeed, or could it also fail with error 267? - Are there any other error codes besides
NotADirectorythat should trigger the fallback toremove_file? - Should we handle the case where both
remove_dirandremove_filefail differently, or is panicking the appropriate behavior?
The current implementation:
if let Err(e) = fs::remove_dir(&host) { if e.kind() == io::ErrorKind::NotADirectory { t!(fs::remove_file(&host)); } else { panic!("failed to remove symlink: {}", e); } }
I want to make sure we're covering all the edge cases for this Windows-specific issue.
This comment has been minimized.
- Changed the error message in the symlink removal logic to use the more concise formatting syntax
{e}instead of the longerformat!style. This improves readability and maintains consistency in error handling.
Can this just use build_helper::fs::recursive_remove? I want to say that doesn't traverse symlinks.
@jieyouxu Seems build_helper::fs::recursive_remove is a better solution here. Looking at its implementation, it properly handles both Windows symlink types without traversing them.It also includes retry logic for transient filesystem errors, and has test coverage for this exact scenario.
Apart from that it also addresses the concerns I raised earlier about error handling edge cases, as recursive remove handles all those scenarios internally with error handling. @ChrisDenton what do you think?
Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-experimental
Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
labels
Can you use recursive_remove as jieyouxu suggested?
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Labels
Operating system: Windows
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)