From 90464544a6a6be5cdccecb8804a4782387005936 Mon Sep 17 00:00:00 2001 From: Paul Colomiets Date: Tue, 27 Oct 2015 23:13:15 +0200 Subject: [PATCH 1/3] Made `remove_dir_all` work as documented when directory is a symlink Previously the function worked correctly only when symlink is inside of a directory, not when path is a symlink itself. This patch also eliminates the `lstat()` call on most systems because `DirEntry::file_type` is enough both to detect if entry is a dir and if entry is a symlink. --- src/libstd/fs.rs | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 6178f1bbb8e12..ceedd5b32eb50 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1069,13 +1069,23 @@ pub fn remove_dir_all>(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) + } else { + _remove_dir_all_unchecked(path) + } +} +fn _remove_dir_all_unchecked(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 mut child_type = try!(child.file_type()); + 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) @@ -1724,6 +1734,23 @@ mod tests { assert!(canary.exists()); } + // FIXME(#12795) depends on lstat to work on windows + #[cfg(not(windows))] + #[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()); From d957c94b77ea7baaab287e13b27bed4e4fdc0a20 Mon Sep 17 00:00:00 2001 From: Paul Colomiets Date: Wed, 28 Oct 2015 21:59:48 +0200 Subject: [PATCH 2/3] The `remove_dir_all` fixes recommended in pull request * Renamed internal method `*_unchecked` -> `*_recursive` * Better windows support (untested) --- src/libstd/fs.rs | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index ceedd5b32eb50..319af66b69120 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1071,21 +1071,41 @@ pub fn remove_dir_all>(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) + 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) + } } else { - _remove_dir_all_unchecked(path) + remove_dir_all_recursive(path) } } -fn _remove_dir_all_unchecked(path: &Path) -> io::Result<()> { +fn remove_dir_all_recursive(path: &Path) -> io::Result<()> { for child in try!(read_dir(path)) { let child = try!(child); let child_path = child.path(); - let mut child_type = try!(child.file_type()); + let child_type = try!(child.file_type()); if child_type.is_dir() { - try!(_remove_dir_all_unchecked(&*child_path)); + try!(remove_dir_all_recursive(&*child_path)); } else { - // The FileType::is_dir() is false for symlinks too - try!(remove_file(&*child_path)); + if cfg!(windows) { + if child_type.is_symlink() { + let target_type = try!(metadata(&*child_path)).file_type(); + 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) From 6f467c729d53fbbb1a2c7a4b8c071214804a3e2a Mon Sep 17 00:00:00 2001 From: Paul Colomiets Date: Wed, 28 Oct 2015 22:27:54 +0200 Subject: [PATCH 3/3] Add unit tests for remove_dir_all on windows --- src/libstd/fs.rs | 61 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 319af66b69120..45a261befe0aa 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1734,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"); @@ -1747,30 +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()); } - // FIXME(#12795) depends on lstat to work on windows #[cfg(not(windows))] #[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!(fs::soft_link(&d2, &d1)); + 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)] + #[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());