Add a test for Windows filesystem behavior that we depend on.

Author: sunfishcode

cap-async-std/src/fs/dir.rs

@@ -46,6 +46,9 @@ pub struct Dir {
 
 impl Dir {
     /// Constructs a new instance of `Self` from the given `async_std::fs::File`.
+    ///
+    /// To prevent race conditions on Windows, the file must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     pub fn from_std_file(std_file: fs::File) -> Self {
         Self { std_file }
@@ -624,6 +627,8 @@ impl FromRawFd for Dir {
 
 #[cfg(windows)]
 impl FromRawHandle for Dir {
+    /// To prevent race conditions on Windows, the handle must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     unsafe fn from_raw_handle(handle: RawHandle) -> Self {
         Self::from_std_file(fs::File::from_raw_handle(handle))

cap-async-std/src/fs_utf8/dir.rs

@@ -31,6 +31,9 @@ pub struct Dir {
 
 impl Dir {
     /// Constructs a new instance of `Self` from the given `async_std::fs::File`.
+    ///
+    /// To prevent race conditions on Windows, the file must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     pub fn from_std_file(std_file: fs::File) -> Self {
         Self::from_cap_std(crate::fs::Dir::from_std_file(std_file))
@@ -543,6 +546,8 @@ impl FromRawFd for Dir {
 
 #[cfg(windows)]
 impl FromRawHandle for Dir {
+    /// To prevent race conditions on Windows, the handle must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     unsafe fn from_raw_handle(handle: RawHandle) -> Self {
         Self::from_std_file(fs::File::from_raw_handle(handle))

cap-primitives/src/fs/open_options.rs

@@ -174,6 +174,8 @@ impl std::os::windows::fs::OpenOptionsExt for OpenOptions {
         self
     }
 
+    /// To prevent race conditions on Windows, handles for directories must be opened
+    /// without `FILE_SHARE_DELETE`.
     #[inline]
     fn share_mode(&mut self, val: u32) -> &mut Self {
         self.ext.share_mode(val);

cap-primitives/src/winx/fs/dir_utils.rs

@@ -9,6 +9,7 @@ use std::{
     },
     path::{Path, PathBuf},
 };
+use winapi::um::winnt;
 use winx::file::Flags;
 
 /// Rust's `Path` implicitly strips redundant slashes and `.` components, however
@@ -46,10 +47,14 @@ pub(crate) fn strip_dir_suffix(path: &Path) -> impl Deref<Target = Path> + '_ {
 
 /// Return an `OpenOptions` for opening directories.
 pub(crate) fn dir_options() -> OpenOptions {
+    // Set `FILE_FLAG_BACKUP_SEMANTICS` so that we can open directories. Unset
+    // `FILE_SHARE_DELETE` so that directories can't be renamed or deleted
+    // underneath us, since we use paths to implement many directory operations.
     OpenOptions::new()
         .read(true)
         .dir_required(true)
         .custom_flags(Flags::FILE_FLAG_BACKUP_SEMANTICS.bits())
+        .share_mode(winnt::FILE_SHARE_READ | winnt::FILE_SHARE_WRITE)
         .clone()
 }
 

cap-primitives/src/winx/fs/open_options_ext.rs

@@ -29,6 +29,8 @@ impl std::os::windows::fs::OpenOptionsExt for OpenOptionsExt {
         self
     }
 
+    /// To prevent race conditions on Windows, handles must be opened without
+    /// `FILE_SHARE_DELETE`.
     fn share_mode(&mut self, share: u32) -> &mut Self {
         self.share_mode = share;
         self

cap-primitives/src/winx/fs/remove_open_dir_impl.rs

@@ -3,5 +3,22 @@ use std::{fs, io};
 
 pub(crate) fn remove_open_dir_impl(dir: fs::File) -> io::Result<()> {
     let path = get_path(&dir)?;
+
+    // Drop the directory before removing it, since we open directories without
+    // `FILE_SHARE_DELETE`, and removing it requires accessing via its name
+    // rather than its handle.
+    //
+    // There is a window here in which another process could remove or rename
+    // a directory with this path after the handle is dropped, however it's
+    // unlikely to happen by accident, and unlikely to cause major problems.
+    // It may cause spurious failures, or failures with different error codes,
+    // but this appears to be unaoidable.
+    //
+    // Even if we did have `FILE_SHARE_DELETE` and we kept the handle open
+    // while doing the `remove_dir, `FILE_SHARE_DELETE` would grant other
+    // processes the right to remove or rename the directory. So there
+    // doesn't seem to be a race-free way of removing opened directories.
+    drop(dir);
+
     fs::remove_dir(path)
 }

cap-primitives/src/yanix/fs/remove_open_dir_by_searching.rs

@@ -10,7 +10,6 @@ pub(crate) fn remove_open_dir_by_searching(dir: fs::File) -> io::Result<()> {
     while let Some(child) = iter.next() {
         let child = child?;
         if child.is_same_file(&metadata)? {
-            drop(dir);
             return child.remove_dir();
         }
     }

cap-std/src/fs/dir.rs

@@ -46,6 +46,9 @@ pub struct Dir {
 
 impl Dir {
     /// Constructs a new instance of `Self` from the given `std::fs::File`.
+    ///
+    /// To prevent race conditions on Windows, the file must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     pub fn from_std_file(std_file: fs::File) -> Self {
         Self { std_file }
@@ -602,6 +605,8 @@ impl FromRawFd for Dir {
 
 #[cfg(windows)]
 impl FromRawHandle for Dir {
+    /// To prevent race conditions on Windows, the handle must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     unsafe fn from_raw_handle(handle: RawHandle) -> Self {
         Self::from_std_file(fs::File::from_raw_handle(handle))

cap-std/src/fs_utf8/dir.rs

@@ -30,6 +30,9 @@ pub struct Dir {
 
 impl Dir {
     /// Constructs a new instance of `Self` from the given `std::fs::File`.
+    ///
+    /// To prevent race conditions on Windows, the file must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     pub fn from_std_file(std_file: fs::File) -> Self {
         Self::from_cap_std(crate::fs::Dir::from_std_file(std_file))
@@ -556,6 +559,8 @@ impl FromRawFd for Dir {
 
 #[cfg(windows)]
 impl FromRawHandle for Dir {
+    /// To prevent race conditions on Windows, the handle must be opened without
+    /// `FILE_SHARE_DELETE`.
     #[inline]
     unsafe fn from_raw_handle(handle: RawHandle) -> Self {
         Self::from_std_file(fs::File::from_raw_handle(handle))

cap-tempfile/src/lib.rs

@@ -191,7 +191,7 @@ fn close_outer() {
     #[cfg(windows)]
     assert_eq!(
         t.close().unwrap_err().raw_os_error(),
-        Some(winapi::shared::winerror::ERROR_DIR_NOT_EMPTY as i32)
+        Some(winapi::shared::winerror::ERROR_SHARING_VIOLATION as i32)
     );
     #[cfg(not(windows))]
     t.close().unwrap();

tests/windows-open.rs

@@ -0,0 +1,94 @@
+//! On Windows, cap-std uses the technique of looking up absolute paths
+//! for file and directory handles. This would be racy, except that Windows
+//! doesn't allow open files or directories to be removed or renamed. Test
+//! that this is the case.
+
+#[cfg(windows)]
+#[macro_use]
+mod sys_common;
+
+#[cfg(windows)]
+use sys_common::io::tmpdir;
+
+#[test]
+#[cfg(windows)]
+fn windows_open_one() {
+    let tmpdir = tmpdir();
+    check!(tmpdir.create_dir("aaa"));
+
+    let dir = tmpdir.open_dir("aaa");
+
+    // Attempts to remove or rename the open directory should fail.
+    tmpdir.remove_dir("aaa").unwrap_err();
+    tmpdir.rename("aaa", &tmpdir, "zzz").unwrap_err();
+
+    drop(dir);
+
+    // Now that we've droped the handle, the same operations should succeed.
+    check!(tmpdir.rename("aaa", &tmpdir, "xxx"));
+    check!(tmpdir.remove_dir("xxx"));
+}
+
+#[test]
+#[cfg(windows)]
+fn windows_open_multiple() {
+    let tmpdir = tmpdir();
+    check!(tmpdir.create_dir_all("aaa/bbb"));
+
+    let dir = tmpdir.open_dir("aaa/bbb");
+
+    // Attempts to remove or rename any component of the open directory should fail.
+    tmpdir.remove_dir("aaa/bbb").unwrap_err();
+    tmpdir.remove_dir("aaa").unwrap_err();
+    tmpdir.rename("aaa/bbb", &tmpdir, "aaa/yyy").unwrap_err();
+    tmpdir.rename("aaa", &tmpdir, "zzz").unwrap_err();
+
+    drop(dir);
+
+    // Now that we've droped the handle, the same operations should succeed.
+    check!(tmpdir.rename("aaa/bbb", &tmpdir, "aaa/www"));
+    check!(tmpdir.rename("aaa", &tmpdir, "xxx"));
+    check!(tmpdir.remove_dir("xxx/www"));
+    check!(tmpdir.remove_dir("xxx"));
+}
+
+/// Like `windows_open_multiple`, but does so within a directory that we
+/// can close and then independently mutate.
+#[test]
+#[cfg(windows)]
+fn windows_open_tricky() {
+    let tmpdir = tmpdir();
+    check!(tmpdir.create_dir("qqq"));
+
+    let qqq = check!(tmpdir.open_dir("qqq"));
+    check!(qqq.create_dir_all("aaa/bbb"));
+
+    let dir = check!(qqq.open_dir("aaa/bbb"));
+
+    // Now drop `qqq`.
+    drop(qqq);
+
+    // Attempts to remove or rename any component of the open directory should fail.
+    dir.remove_dir("aaa/bbb").unwrap_err();
+    dir.remove_dir("aaa").unwrap_err();
+    dir.rename("aaa/bbb", &tmpdir, "aaa/yyy").unwrap_err();
+    dir.rename("aaa", &tmpdir, "zzz").unwrap_err();
+    tmpdir.remove_dir("qqq/aaa/bbb").unwrap_err();
+    tmpdir.remove_dir("qqq/aaa").unwrap_err();
+    tmpdir.remove_dir("qqq").unwrap_err();
+    tmpdir
+        .rename("qqq/aaa/bbb", &tmpdir, "qqq/aaa/yyy")
+        .unwrap_err();
+    tmpdir.rename("qqq/aaa", &tmpdir, "qqq/zzz").unwrap_err();
+    tmpdir.rename("qqq", &tmpdir, "vvv").unwrap_err();
+
+    drop(dir);
+
+    // Now that we've droped the handle, the same operations should succeed.
+    check!(tmpdir.rename("qqq/aaa/bbb", &tmpdir, "qqq/aaa/www"));
+    check!(tmpdir.rename("qqq/aaa", &tmpdir, "qqq/xxx"));
+    check!(tmpdir.rename("qqq", &tmpdir, "uuu"));
+    check!(tmpdir.remove_dir("uuu/xxx/www"));
+    check!(tmpdir.remove_dir("uuu/xxx"));
+    check!(tmpdir.remove_dir("uuu"));
+}