From 20a3d57669614b6c03c496c8dedb6825e513e6d9 Mon Sep 17 00:00:00 2001 From: Eric Cornelissen Date: Tue, 11 Jul 2023 21:36:51 +0200 Subject: [PATCH] Fix removing symbolic links to directories on Windows Update the implementation of `rm::remove` to, conditionally on the target platform, remove symbolic files either: 1. On non-Windows systems always as files (unchanged), or 2. On Windows systems as either a directory or file depending on what the link is pointing to. The latter is necessary for removal of symbolic links on Windows because otherwise users will get a `PermissionDenied` error. All tests that were disabled on Windows for this reason previously now run conditionally on the `test-symlink` test feature, just like all other symlink-related tests. Except for the unit test named `symlink_to_ dir_at_ location_of_a_dir_toctou`. With the current implementation of the `remove` function this TOCTOU scenario is handled differently on Windows from non-Windows systems, but both outcomes are considered okay (both versions of the test are kept to notice changes/regressions on any platform). --- src/main.rs | 57 +++++++++++++++++++++++++++++++++++------ tests/dir_test.rs | 10 ++++++-- tests/link_test.rs | 10 ++++++-- tests/recursive_test.rs | 10 ++++++-- 4 files changed, 73 insertions(+), 14 deletions(-) diff --git a/src/main.rs b/src/main.rs index 556c324..e956c68 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2594,8 +2594,8 @@ mod rm { #[test] #[cfg_attr( - all(windows, not(feature = "test-symlink")), - ignore = "Only run with the test-symlink feature" + any(not(feature = "test-trash"), all(windows, not(feature = "test-symlink"))), + ignore = "Only run with the test-trash (and test-symlink on Windows) feature" )] fn symlink_to_empty_dir() -> TestResult { with_test_dir(|test_dir| { @@ -2693,8 +2693,8 @@ mod rm { #[test] #[cfg_attr( - all(windows, not(feature = "test-symlink")), - ignore = "Only run with the test-symlink feature" + any(not(feature = "test-trash"), all(windows, not(feature = "test-symlink"))), + ignore = "Only run with the test-trash (and test-symlink on Windows) feature" )] fn symlink_to_dir_at_location_of_a_dir_toctou() -> TestResult { with_test_dir(|test_dir| { @@ -2738,7 +2738,16 @@ mod rm { let path = entry.path(); let result = match entry.kind() { fs::EntryKind::Dir => remove_dir(path), - fs::EntryKind::File | fs::EntryKind::Symlink => remove_file(path), + fs::EntryKind::File => remove_file(path), + #[cfg(not(windows))] + fs::EntryKind::Symlink => remove_file(path), + #[cfg(windows)] + fs::EntryKind::Symlink => match std::fs::metadata(&path) { + Ok(metadata) if metadata.is_dir() => remove_dir(path), + Ok(metadata) if metadata.is_file() => remove_file(path), + Ok(_) => unreachable!(), + Err(err) => Err(err), + }, }; match result { @@ -2881,7 +2890,10 @@ mod rm { } #[test] - #[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] + #[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" + )] fn symlink_to_empty_dir() -> TestResult { with_test_dir(|test_dir| { let dir = test_dir.child("dir"); @@ -2903,7 +2915,10 @@ mod rm { } #[test] - #[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] + #[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" + )] fn symlink_to_filled_dir() -> TestResult { with_test_dir(|test_dir| { let dir = test_dir.child("dir"); @@ -2972,7 +2987,7 @@ mod rm { } #[test] - #[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] + #[cfg(not(windows))] fn symlink_to_dir_at_location_of_a_dir_toctou() -> TestResult { with_test_dir(|test_dir| { let dir = test_dir.child("dir"); @@ -2992,6 +3007,32 @@ mod rm { Ok(()) }) } + + #[test] + #[cfg(windows)] + #[cfg_attr( + not(feature = "test-symlink"), + ignore = "Only run with the test-symlink feature" + )] + fn symlink_to_dir_at_location_of_a_dir_toctou() -> TestResult { + with_test_dir(|test_dir| { + let dir = test_dir.child("dir"); + dir.create_dir_all()?; + let link = test_dir.child("link"); + link.symlink_to_dir(&dir)?; + + let path = link.path(); + let entry = fs::test_helpers::new_dir(path); + + let out = remove(entry); + assert_eq!(out, Ok(format!("Removed {}", path.display().bold()))); + + dir.assert(predicate::path::exists()); + link.assert(predicate::path::missing()); + + Ok(()) + }) + } } /// Pretend to dispose of the [`fs::Entry`]. diff --git a/tests/dir_test.rs b/tests/dir_test.rs index cd093e9..cb51a3a 100644 --- a/tests/dir_test.rs +++ b/tests/dir_test.rs @@ -147,7 +147,10 @@ fn symlink_to_file() -> TestResult { } #[test] -#[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] +#[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" +)] fn symlink_to_empty_dir() -> TestResult { let linkname = "link"; @@ -186,7 +189,10 @@ fn symlink_to_empty_dir() -> TestResult { } #[test] -#[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] +#[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" +)] fn symlink_to_filled_dir() -> TestResult { let linkname = "link"; diff --git a/tests/link_test.rs b/tests/link_test.rs index 2072b58..cda55ce 100644 --- a/tests/link_test.rs +++ b/tests/link_test.rs @@ -96,7 +96,10 @@ fn symlink_to_a_file_remove_file() -> TestResult { } #[test] -#[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] +#[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" +)] fn symlink_to_an_empty_dir_remove_link() -> TestResult { let linkname = "link"; @@ -177,7 +180,10 @@ fn symlink_to_an_empty_dir_remove_dir() -> TestResult { } #[test] -#[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] +#[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" +)] fn symlink_to_a_filled_dir_remove_link() -> TestResult { let linkname = "link"; diff --git a/tests/recursive_test.rs b/tests/recursive_test.rs index 528af62..6451d08 100644 --- a/tests/recursive_test.rs +++ b/tests/recursive_test.rs @@ -214,7 +214,10 @@ fn symlink_to_file() -> TestResult { } #[test] -#[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] +#[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" +)] fn symlink_to_empty_dir() -> TestResult { let linkname = "link"; @@ -253,7 +256,10 @@ fn symlink_to_empty_dir() -> TestResult { } #[test] -#[cfg_attr(windows, ignore = "TODO: investigate symlink test errors on Windows")] +#[cfg_attr( + all(windows, not(feature = "test-symlink")), + ignore = "Only run with the test-symlink feature" +)] fn symlink_to_filled_dir() -> TestResult { let linkname = "link";