Skip to content

Made remove_dir_all work as documented when directory is a symlink #29412

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

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
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
37 changes: 32 additions & 5 deletions src/libstd/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1069,13 +1069,23 @@ pub fn remove_dir_all<P: AsRef<Path>>(path: P) -> io::Result<()> {
}

fn _remove_dir_all(path: &Path) -> io::Result<()> {
let filetype = try!(symlink_metadata(path)).file_type();
if filetype.is_symlink() {
remove_file(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this will work correctly on Windows; a symlink isn't always a file on Windows.

Copy link
Author

Choose a reason for hiding this comment

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

Well, the code that removed a symlink inside the directory used remove_file too, AFAICS. So at least it's not a regression.

Anyway, it would be helpful if someone can test it on windows.

Copy link
Member

Choose a reason for hiding this comment

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

I've confirmed that directory symlinks on windows cannot be removed by remove_file but can be removed via remove_dir.

Copy link
Member

Choose a reason for hiding this comment

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

I think that this may want to also use is_dir to choose whether to recurse or not (like below). Junctions on Windows (e.g. hard links to directories) don't report is_dir, but to behave the same as unix we should delete their contents.

Copy link
Author

Choose a reason for hiding this comment

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

I think that this may want to also use is_dir to choose whether to recurse or not (like below). Junctions on Windows (e.g. hard links to directories) don't report is_dir, but to behave the same as unix we should delete their contents.

Isn't it a contradiction? If they don't report is_dir how should we know to recurse?

The question probably is: is_dir() && is_symlink() (impossible on unix) should recurse or remove_dir() ?

Copy link
Author

Choose a reason for hiding this comment

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

Okay, it looks like impossible combination on windows too:
https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/fs.rs#L429

So probably I must stat symlink on windows to find out whether it's a directory. And then if it's a directory do (non-recursive) remove_dir. Right? According to wikipedia, junctions are removed with rmdir (i.e. non-recursive)

Copy link
Contributor

Choose a reason for hiding this comment

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

remove_dir() seems like the answer which is most consistent with other forms of symbolic links.

Copy link
Member

Choose a reason for hiding this comment

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

Hm yes I think I may have gotten a little confused, I'll have to think on this a bit.

} else {
_remove_dir_all_unchecked(path)
}
}
fn _remove_dir_all_unchecked(path: &Path) -> io::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

It's fine to remove the leading underscore here, the convention above is just because it's paired with a sibling remove_dir_all function.

Copy link
Member

Choose a reason for hiding this comment

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

Also this may be best with a _recursive prefix here instead of _unchecked

for child in try!(read_dir(path)) {
let child = try!(child).path();
let stat = try!(symlink_metadata(&*child));
if stat.is_dir() {
try!(remove_dir_all(&*child));
let child = try!(child);
let child_path = child.path();
let mut child_type = try!(child.file_type());
Copy link
Member

Choose a reason for hiding this comment

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

This should fall back to calling symlink_metadata in the case file_type() failed (as it's not guaranteed to work across all platforms right now)

Copy link
Author

Choose a reason for hiding this comment

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

AFAICS, fallback is already inside DirEntry: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fs.rs#L186 (unix), on windows it looks like it always work.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right! I forgot about that, carry on!

if child_type.is_dir() {
try!(_remove_dir_all_unchecked(&*child_path));
} else {
try!(remove_file(&*child));
// The FileType::is_dir() is false for symlinks too
try!(remove_file(&*child_path));
}
}
remove_dir(path)
Expand Down Expand Up @@ -1724,6 +1734,23 @@ mod tests {
assert!(canary.exists());
}

// FIXME(#12795) depends on lstat to work on windows
#[cfg(not(windows))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to disable this test on Windows? #12795 was fixed a long time ago.

Copy link
Author

Choose a reason for hiding this comment

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

As I outlined in the description of the pull request, I don't know. Just copied it from another test on remove_dir_all. Can you test on windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, nevermind, the test won't work because soft_link doesn't do the right thing on Windows.

Actually, given that this is a new test, please avoid using soft_link because it's deprecated.

Copy link
Author

Choose a reason for hiding this comment

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

So should I remove the comment but leave cfg(not(windows))? (so use symlink from os::unix)

I'm pretty sure tests for windows should be separate testing both symlink_file, and symlink_dir. And I'm unable to write them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's fine for now; once this is merged, you can file a followup issue about adding Windows tests.

#[test]
fn recursive_rmdir_of_symlink() {
let tmpdir = tmpdir();
let d1 = tmpdir.join("d1");
let d2 = tmpdir.join("d2");
let canary = d2.join("do_not_delete");
check!(fs::create_dir_all(&d2));
check!(check!(File::create(&canary)).write(b"foo"));
check!(fs::soft_link(&d2, &d1));
check!(fs::remove_dir_all(&d1));

assert!(!d1.is_dir());
assert!(canary.exists());
}

#[test]
fn unicode_path_is_dir() {
assert!(Path2::new(".").is_dir());
Expand Down