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 all commits
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
114 changes: 107 additions & 7 deletions src/libstd/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1069,13 +1069,43 @@ 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() {
if cfg!(windows) {
// On Windows symlinks to files and directories removed differently
// we should remove only directories here and have error on file
remove_dir(path)
} else {
// On unix symlinks are always files
remove_file(path)
}
Copy link
Member

Choose a reason for hiding this comment

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

This might be good to extract into a helper function to be shared with below, something like:

fn remove(path: &Path, meta: &fs::Metadata) -> io::Result<()> {
    // ...
}

} else {
remove_dir_all_recursive(path)
}
}
fn remove_dir_all_recursive(path: &Path) -> io::Result<()> {
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 child_type = try!(child.file_type());
if child_type.is_dir() {
try!(remove_dir_all_recursive(&*child_path));
} else {
try!(remove_file(&*child));
if cfg!(windows) {
if child_type.is_symlink() {
let target_type = try!(metadata(&*child_path)).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.

I tested this out a bit, and oddly enough I'm not sure we can use this function to determine this. Using metadata will resolve all intermediate symlinks, showing the file type of the actual destination file. This ends up having what I think are two problems:

  1. If the symlink is broken (e.g. the destination is gone), I don't think this will work.
  2. Removal of the symlink depends on what kind of symlink was created, not the destination itself.

I found that I could create a symlink to a file with symlink_dir, but I could only remove the symlink with remove_dir. I think that we may have enough information in the DirEntry itself to figure this out (via a private API for now).

Copy link
Member

Choose a reason for hiding this comment

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

I found that I could create a symlink to a file with symlink_dir,

That's because Windows doesn't check the target of symbolic links when you create them. You still created a directory symbolic link. It just happens to be pointing at a file.

if target_type.is_dir() {
try!(remove_dir(&*child_path));
} else {
try!(remove_file(&*child_path));
}
} else {
try!(remove_file(&*child_path));
}
} else {
// The FileType::is_dir() is false for symlinks too
try!(remove_file(&*child_path));
}
}
}
remove_dir(path)
Expand Down Expand Up @@ -1704,10 +1734,10 @@ mod tests {
check!(fs::create_dir_all(&Path2::new("/")));
}

// FIXME(#12795) depends on lstat to work on windows
#[cfg(not(windows))]
#[test]
fn recursive_rmdir() {
use os::unix::fs::symlink;
let tmpdir = tmpdir();
let d1 = tmpdir.join("d1");
let dt = d1.join("t");
Expand All @@ -1717,13 +1747,83 @@ mod tests {
check!(fs::create_dir_all(&dtt));
check!(fs::create_dir_all(&d2));
check!(check!(File::create(&canary)).write(b"foo"));
check!(fs::soft_link(&d2, &dt.join("d2")));
check!(symlink(&d2, &dt.join("d2")));
check!(fs::remove_dir_all(&d1));

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

#[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() {
use os::unix::fs::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!(symlink(&d2, &d1));
check!(fs::remove_dir_all(&d1));

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

#[cfg(windows)]
#[test]
fn recursive_rmdir() {
use os::windows::fs::{symlink_file, symlink_dir};
let tmpdir = tmpdir();
let d1 = tmpdir.join("d1");
let dt = d1.join("t");
let dtt = dt.join("t");
let d2 = tmpdir.join("d2");
let canary = d2.join("do_not_delete");
check!(fs::create_dir_all(&dtt));
check!(fs::create_dir_all(&d2));
check!(check!(File::create(&canary)).write(b"foo"));
check!(symlink_dir(&d2, &dt.join("d2")));
check!(symlink_file(&canary, &d1.join("canary")));
check!(fs::remove_dir_all(&d1));

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

#[cfg(windows)]
#[test]
fn recursive_rmdir_of_symlink() {
use os::windows::fs::symlink_dir;
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!(symlink_dir(&d2, &d1));
check!(fs::remove_dir_all(&d1));

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

#[cfg(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 think it's fine to include these tests, they'll need to be guarded though to see if the current user has the privilege to create symbolic links. I don't think we want to require the test suite to run as an administrator by default, unfortunately, but I don't mind going into the bots and giving the rustbuild user privileges to make symlinks!

Copy link
Member

Choose a reason for hiding this comment

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

And also while you're at it, could you add test cases like:

  • remove_dir_all with a broken symlink inside
  • remove a directory with a directory symlink to a file
  • remove a directory with a file symlink to a directory

Copy link
Author

Choose a reason for hiding this comment

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

they'll need to be guarded though to see if the current user has the privilege to create symbolic links

Any hints on how to do that?

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 the PrivilegeCheck API looks like it may do the trick? (I haven't tested this out though).

#[test]
fn recursive_rmdir_of_file_symlink() {
use os::windows::fs::symlink_file;
let tmpdir = tmpdir();
let f1 = tmpdir.join("f1");
let f2 = tmpdir.join("f2");
check!(check!(File::create(&f1)).write(b"foo"));
check!(symlink_file(&f1, &f2));
match fs::remove_dir_all(&f2) {
Ok(..) => panic!("wanted a failure"),
Err(..) => {}
}
}

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