Skip to content

Windows: fix Dir::remove and Dir::rename following symlinks #164559

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

Merged
rust-bors merged 2 commits into from
Nov 10, 2026
Merged

Windows: fix Dir::remove and Dir::rename following symlinks #164559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eaf2a71
fs::Dir tests: ensure remove and rename do not follow symlinks
RalfJung Oct 19, 2026
811ebf7
windows: fix Dir::{remove_*,rename} symlink behavior
RalfJung Oct 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions library/std/src/fs/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3250,6 +3250,14 @@ fn test_dir_remove_file() {
check!(f.flush());

@ChrisDenton ChrisDenton Nov 10, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why was this removed? Or why was it there before?

View changes since the review

@RalfJung RalfJung Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I removed it because it seemed unnecessary and I was touching this test anyway. There are a couple of flush in this file. The oldest one was added by you in c6f7aa0; #150679 likely just copied that then.

EDIT: I undid this part of the diff. These flush should IMO have comments as they look quite odd, but that is orthogonal to this PR.

@RalfJung RalfJung Nov 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Specifically File::flush is even documented to be a NOP.

Since a File structure doesn’t contain any buffers, this function is currently a no-op on Unix and Windows. Note that this may change in the future.

@ChrisDenton ChrisDenton Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It might have been to ensure the metadata is updated before we reopen the file.

@RalfJung RalfJung Nov 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

flush is not sync_all though.
I made this a separate PR: #165739.

@ChrisDenton ChrisDenton Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I was speculating as to the intent, not the effect.

drop(f);
let dir = check!(Dir::open(tmpdir.path()));

if got_symlink_permission(&tmpdir) {
// Removing a symlink should remove the link, not the destination.
check!(symlink_file("foo.txt", &tmpdir.join("link")));
check!(dir.remove_file("link"));
assert!(check!(dir.metadata("foo.txt")).is_file()); // file still exists
}

check!(dir.remove_file("foo.txt"));
assert!(!matches!(exists(tmpdir.join("foo.txt")), Ok(true)));
}
Expand All @@ -3266,6 +3274,14 @@ fn test_dir_rename_file() {
let mut buf = [0u8; 3];
check!(f.read_exact(&mut buf));
assert_eq!(b"bar", &buf);

// Renaming a symlink should rename the link, not the destination.
if got_symlink_permission(&tmpdir) {
check!(symlink_file("baz.txt", &tmpdir.join("link")));
check!(dir.rename("link", &dir, "link-renamed"));
assert!(check!(dir.symlink_metadata("baz.txt")).is_file()); // file still exists
assert!(check!(dir.symlink_metadata("link-renamed")).is_symlink()); // symlink has new name
}
}

// FIXME: re-enable once QNX fixes TOCTOU bug for fs::remove_dir
Expand All @@ -3277,6 +3293,21 @@ fn test_dir_remove_dir() {
let tmpdir = tmpdir();
check!(fs::create_dir(tmpdir.join("foo")));
let dir = check!(Dir::open(tmpdir.path()));

if got_symlink_permission(&tmpdir) {
// Removing a symlink should remove the link, not the destination.
check!(symlink_dir("foo", &tmpdir.join("link")));
if cfg!(windows) {
// directory symlinks need `remove_dir` on Windows
dir.remove_file("link").unwrap_err();
check!(dir.remove_dir("link"));
} else {
dir.remove_dir("link").unwrap_err();
check!(dir.remove_file("link"));
}
assert!(check!(dir.metadata("foo")).is_dir()); // dir still exists
}

check!(dir.remove_dir("foo"));
assert!(!matches!(exists(tmpdir.join("foo")), Ok(true)));
}
Expand All @@ -3289,6 +3320,14 @@ fn test_dir_rename_dir() {
check!(dir.rename("foo", &dir, "baz"));
let m = check!(tmpdir.join("baz").metadata());
assert!(m.is_dir());

// Renaming a symlink should rename the link, not the destination.
if got_symlink_permission(&tmpdir) {
check!(symlink_dir("baz", &tmpdir.join("link")));
check!(dir.rename("link", &dir, "link-renamed"));
assert!(check!(dir.symlink_metadata("baz")).is_dir()); // dir still exists
assert!(check!(dir.symlink_metadata("link-renamed")).is_symlink()); // symlink has new name
}
}

#[test]
Expand Down
8 changes: 4 additions & 4 deletions library/std/src/sys/fs/windows/dir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,17 +157,17 @@ impl Dir {
fn remove_native(&self, path: &[u16], dir: bool) -> io::Result<()> {
let mut opts = OpenOptions::new();
opts.access_mode(c::DELETE);
let create_opt = if dir { c::FILE_DIRECTORY_FILE } else { c::FILE_NON_DIRECTORY_FILE };
let mut create_opt = if dir { c::FILE_DIRECTORY_FILE } else { c::FILE_NON_DIRECTORY_FILE };
create_opt |= c::FILE_OPEN_REPARSE_POINT;
let handle = self.open_file_native(path, &opts, create_opt)?;
File::from_inner(handle).delete().io_result()
}

fn rename_native(&self, from: &[u16], to_dir: &Self, to: &[u16]) -> io::Result<()> {
let mut opts = OpenOptions::new();
opts.access_mode(c::DELETE);
// FIXME: custom_flags is ignored by `open_file_native`!
opts.custom_flags(c::FILE_FLAG_OPEN_REPARSE_POINT | c::FILE_FLAG_BACKUP_SEMANTICS);
let handle = self.open_file_native(from, &opts, /* create_opt */ 0)?;
let handle =
self.open_file_native(from, &opts, /* create_opt */ c::FILE_OPEN_REPARSE_POINT)?;
// Calculate the layout of the `FILE_RENAME_INFORMATION` we pass to `NtSetInformationFile`
// This is a dynamically sized struct so we need to get the position of the last field to calculate the actual size.
const too_long_err: io::Error =
Expand Down
Loading