Fix a race condition in remove_dir_all. (#222)

Port the fix from rust-lang/rust#93112 to cap-std.
bytecodealliance · Jan 21, 2022 · 4191da8 · 4191da8
1 parent 0a85201
commit 4191da8
Show file tree

Hide file tree

Showing 17 changed files with 256 additions and 102 deletions.
diff --git a/cap-primitives/src/fs/dir_entry.rs b/cap-primitives/src/fs/dir_entry.rs
@@ -1,4 +1,6 @@
-use crate::fs::{dir_options, DirEntryInner, FileType, Metadata, OpenOptions, ReadDir};
+use crate::fs::{
+    dir_options, DirEntryInner, FileType, FollowSymlinks, Metadata, OpenOptions, ReadDir,
+};
 #[cfg(not(windows))]
 use rustix::fs::DirEntryExt;
 use std::ffi::OsString;
@@ -57,7 +59,7 @@ impl DirEntry {
     /// Returns an iterator over the entries within the subdirectory.
     #[inline]
     pub fn read_dir(&self) -> io::Result<ReadDir> {
-        self.inner.read_dir()
+        self.inner.read_dir(FollowSymlinks::Yes)
     }
 
     /// Returns the metadata for the file that this entry points at.

diff --git a/cap-primitives/src/fs/file_path_by_searching.rs b/cap-primitives/src/fs/file_path_by_searching.rs
@@ -1,4 +1,6 @@
-use crate::fs::{is_root_dir, open_dir_unchecked, read_dir_unchecked, MaybeOwnedFile, Metadata};
+use crate::fs::{
+    is_root_dir, open_dir_unchecked, read_dir_unchecked, FollowSymlinks, MaybeOwnedFile, Metadata,
+};
 use std::fs;
 use std::path::{Component, PathBuf};
 
@@ -13,7 +15,8 @@ pub(crate) fn file_path_by_searching(file: &fs::File) -> Option<PathBuf> {
     // Iterate with `..` until we reach the root directory.
     'next_component: loop {
         // Open `..`.
-        let mut iter = read_dir_unchecked(&base, Component::ParentDir.as_ref()).ok()?;
+        let mut iter =
+            read_dir_unchecked(&base, Component::ParentDir.as_ref(), FollowSymlinks::No).ok()?;
         let metadata = Metadata::from_file(&*base).ok()?;
 
         // Search the children until we find one with matching metadata, and

diff --git a/cap-primitives/src/fs/mod.rs b/cap-primitives/src/fs/mod.rs
@@ -54,7 +54,8 @@ pub(crate) use super::rustix::fs::*;
 #[cfg(windows)]
 pub(crate) use super::windows::fs::*;
 
-pub(crate) use read_dir::read_dir_unchecked;
+#[cfg(not(windows))]
+pub(crate) use read_dir::{read_dir_nofollow, read_dir_unchecked};
 
 pub use canonicalize::canonicalize;
 pub use copy::copy;

diff --git a/cap-primitives/src/fs/open_dir.rs b/cap-primitives/src/fs/open_dir.rs
@@ -18,9 +18,14 @@ pub fn open_dir(start: &fs::File, path: &Path) -> io::Result<fs::File> {
 
 /// Like `open_dir`, but additionally request the ability to read the directory
 /// entries.
+#[cfg(not(windows))]
 #[inline]
-pub fn open_dir_for_reading(start: &fs::File, path: &Path) -> io::Result<fs::File> {
-    open(start, path, &readdir_options())
+pub(crate) fn open_dir_for_reading(
+    start: &fs::File,
+    path: &Path,
+    follow: FollowSymlinks,
+) -> io::Result<fs::File> {
+    open(start, path, readdir_options().follow(follow))
 }
 
 /// Similar to `open_dir`, but fails if the path names a symlink.
@@ -43,8 +48,9 @@ pub(crate) fn open_dir_unchecked(start: &fs::File, path: &Path) -> io::Result<fs
 pub(crate) fn open_dir_for_reading_unchecked(
     start: &fs::File,
     path: &Path,
+    follow: FollowSymlinks,
 ) -> io::Result<fs::File> {
-    open_unchecked(start, path, &readdir_options()).map_err(Into::into)
+    open_unchecked(start, path, readdir_options().follow(follow)).map_err(Into::into)
 }
 
 /// Open a directory named by a bare path, using the host process' ambient

diff --git a/cap-primitives/src/fs/read_dir.rs b/cap-primitives/src/fs/read_dir.rs
@@ -1,4 +1,4 @@
-use crate::fs::{DirEntry, ReadDirInner};
+use crate::fs::{DirEntry, FollowSymlinks, ReadDirInner};
 use std::path::Path;
 use std::{fmt, fs, io};
 
@@ -8,7 +8,16 @@ use std::{fmt, fs, io};
 #[inline]
 pub fn read_dir(start: &fs::File, path: &Path) -> io::Result<ReadDir> {
     Ok(ReadDir {
-        inner: ReadDirInner::new(start, path)?,
+        inner: ReadDirInner::new(start, path, FollowSymlinks::Yes)?,
+    })
+}
+
+/// Like `read_dir`, but fails if `path` names a symlink.
+#[inline]
+#[cfg(not(windows))]
+pub(crate) fn read_dir_nofollow(start: &fs::File, path: &Path) -> io::Result<ReadDir> {
+    Ok(ReadDir {
+        inner: ReadDirInner::new(start, path, FollowSymlinks::No)?,
     })
 }
 
@@ -23,9 +32,14 @@ pub fn read_base_dir(start: &fs::File) -> io::Result<ReadDir> {
 
 /// Like `read_dir`, but doesn't perform sandboxing.
 #[inline]
-pub(crate) fn read_dir_unchecked(start: &fs::File, path: &Path) -> io::Result<ReadDir> {
+#[cfg(not(windows))]
+pub(crate) fn read_dir_unchecked(
+    start: &fs::File,
+    path: &Path,
+    follow: FollowSymlinks,
+) -> io::Result<ReadDir> {
     Ok(ReadDir {
-        inner: ReadDirInner::new_unchecked(start, path)?,
+        inner: ReadDirInner::new_unchecked(start, path, follow)?,
     })
 }
 

diff --git a/cap-primitives/src/rustix/fs/dir_entry_inner.rs b/cap-primitives/src/rustix/fs/dir_entry_inner.rs
@@ -1,4 +1,6 @@
-use crate::fs::{FileType, FileTypeExt, Metadata, OpenOptions, ReadDir, ReadDirInner};
+use crate::fs::{
+    FileType, FileTypeExt, FollowSymlinks, Metadata, OpenOptions, ReadDir, ReadDirInner,
+};
 use rustix::fs::DirEntry;
 use std::ffi::{OsStr, OsString};
 #[cfg(unix)]
@@ -29,8 +31,8 @@ impl DirEntryInner {
     }
 
     #[inline]
-    pub(crate) fn read_dir(&self) -> io::Result<ReadDir> {
-        self.read_dir.read_dir(self.file_name_bytes())
+    pub(crate) fn read_dir(&self, follow: FollowSymlinks) -> io::Result<ReadDir> {
+        self.read_dir.read_dir(self.file_name_bytes(), follow)
     }
 
     #[inline]

diff --git a/cap-primitives/src/rustix/fs/read_dir_inner.rs b/cap-primitives/src/rustix/fs/read_dir_inner.rs
@@ -22,8 +22,8 @@ pub(crate) struct ReadDirInner {
 }
 
 impl ReadDirInner {
-    pub(crate) fn new(start: &fs::File, path: &Path) -> io::Result<Self> {
-        let dir = Dir::from(open_dir_for_reading(start, path)?)?;
+    pub(crate) fn new(start: &fs::File, path: &Path, follow: FollowSymlinks) -> io::Result<Self> {
+        let dir = Dir::from(open_dir_for_reading(start, path, follow)?)?;
         Ok(Self {
             raw_fd: dir.as_fd().as_raw_fd(),
             rustix: Arc::new(Mutex::new(dir)),
@@ -38,15 +38,20 @@ impl ReadDirInner {
         let dir = Dir::from(open_dir_for_reading_unchecked(
             start,
             Component::CurDir.as_ref(),
+            FollowSymlinks::No,
         )?)?;
         Ok(Self {
             raw_fd: dir.as_fd().as_raw_fd(),
             rustix: Arc::new(Mutex::new(dir)),
         })
     }
 
-    pub(crate) fn new_unchecked(start: &fs::File, path: &Path) -> io::Result<Self> {
-        let dir = open_dir_for_reading_unchecked(start, path)?;
+    pub(crate) fn new_unchecked(
+        start: &fs::File,
+        path: &Path,
+        follow: FollowSymlinks,
+    ) -> io::Result<Self> {
+        let dir = open_dir_for_reading_unchecked(start, path, follow)?;
         Ok(Self {
             raw_fd: dir.as_fd().as_raw_fd(),
             rustix: Arc::new(Mutex::new(Dir::from(dir)?)),
@@ -73,8 +78,12 @@ impl ReadDirInner {
         Metadata::from_file(&self.as_file_view())
     }
 
-    pub(super) fn read_dir(&self, file_name: &OsStr) -> io::Result<ReadDir> {
-        read_dir_unchecked(&self.as_file_view(), file_name.as_ref())
+    pub(super) fn read_dir(
+        &self,
+        file_name: &OsStr,
+        follow: FollowSymlinks,
+    ) -> io::Result<ReadDir> {
+        read_dir_unchecked(&self.as_file_view(), file_name.as_ref(), follow)
     }
 
     #[allow(unsafe_code)]

diff --git a/cap-primitives/src/rustix/fs/remove_dir_all_impl.rs b/cap-primitives/src/rustix/fs/remove_dir_all_impl.rs
@@ -1,6 +1,6 @@
 use crate::fs::{
-    read_dir, read_dir_unchecked, remove_dir, remove_file, remove_open_dir, stat, FollowSymlinks,
-    ReadDir,
+    read_dir_nofollow, read_dir_unchecked, remove_dir, remove_file, remove_open_dir, stat,
+    FollowSymlinks, ReadDir,
 };
 use std::path::{Component, Path};
 use std::{fs, io};
@@ -13,21 +13,25 @@ pub(crate) fn remove_dir_all_impl(start: &fs::File, path: &Path) -> io::Result<(
     if filetype.is_symlink() {
         remove_file(start, path)
     } else {
-        remove_dir_all_recursive(read_dir(start, path)?)?;
+        remove_dir_all_recursive(read_dir_nofollow(start, path)?)?;
         remove_dir(start, path)
     }
 }
 
 pub(crate) fn remove_open_dir_all_impl(dir: fs::File) -> io::Result<()> {
-    remove_dir_all_recursive(read_dir_unchecked(&dir, Component::CurDir.as_ref())?)?;
+    remove_dir_all_recursive(read_dir_unchecked(
+        &dir,
+        Component::CurDir.as_ref(),
+        FollowSymlinks::No,
+    )?)?;
     remove_open_dir(dir)
 }
 
 fn remove_dir_all_recursive(children: ReadDir) -> io::Result<()> {
     for child in children {
         let child = child?;
         if child.file_type()?.is_dir() {
-            remove_dir_all_recursive(child.read_dir()?)?;
+            remove_dir_all_recursive(child.inner.read_dir(FollowSymlinks::No)?)?;
             child.remove_dir()?;
         } else {
             child.remove_file()?;

diff --git a/cap-primitives/src/rustix/fs/remove_open_dir_by_searching.rs b/cap-primitives/src/rustix/fs/remove_open_dir_by_searching.rs
@@ -1,4 +1,4 @@
-use crate::fs::{errors, is_root_dir, read_dir_unchecked, Metadata};
+use crate::fs::{errors, is_root_dir, read_dir_unchecked, FollowSymlinks, Metadata};
 use std::path::Component;
 use std::{fs, io};
 
@@ -7,7 +7,7 @@ use std::{fs, io};
 /// available.
 pub(crate) fn remove_open_dir_by_searching(dir: fs::File) -> io::Result<()> {
     let metadata = Metadata::from_file(&dir)?;
-    let mut iter = read_dir_unchecked(&dir, Component::ParentDir.as_ref())?;
+    let mut iter = read_dir_unchecked(&dir, Component::ParentDir.as_ref(), FollowSymlinks::No)?;
     while let Some(child) = iter.next() {
         let child = child?;
         if child.is_same_file(&metadata)? {

diff --git a/cap-primitives/src/windows/fs/dir_entry_inner.rs b/cap-primitives/src/windows/fs/dir_entry_inner.rs
@@ -68,7 +68,12 @@ impl DirEntryInner {
     }
 
     #[inline]
-    pub(crate) fn read_dir(&self) -> io::Result<ReadDir> {
+    pub(crate) fn read_dir(&self, follow: FollowSymlinks) -> io::Result<ReadDir> {
+        assert_eq!(
+            follow,
+            FollowSymlinks::Yes,
+            "`read_dir` without following symlinks is not implemented yet"
+        );
         let std = fs::read_dir(self.std.path())?;
         let inner = ReadDirInner::from_std(std);
         Ok(ReadDir { inner })

diff --git a/cap-primitives/src/windows/fs/read_dir_inner.rs b/cap-primitives/src/windows/fs/read_dir_inner.rs
@@ -1,5 +1,5 @@
 use super::get_path::concatenate_or_return_absolute;
-use crate::fs::{open_dir, DirEntryInner};
+use crate::fs::{open_dir, DirEntryInner, FollowSymlinks};
 use std::path::{Component, Path};
 use std::{fmt, fs, io};
 
@@ -8,7 +8,12 @@ pub(crate) struct ReadDirInner {
 }
 
 impl ReadDirInner {
-    pub(crate) fn new(start: &fs::File, path: &Path) -> io::Result<Self> {
+    pub(crate) fn new(start: &fs::File, path: &Path, follow: FollowSymlinks) -> io::Result<Self> {
+        assert_eq!(
+            follow,
+            FollowSymlinks::Yes,
+            "`read_dir` without following symlinks is not implemented yet"
+        );
         let dir = open_dir(start, path)?;
         Self::new_unchecked(&dir, Component::CurDir.as_ref())
     }

diff --git a/cap-primitives/src/windows/fs/remove_dir_all_impl.rs b/cap-primitives/src/windows/fs/remove_dir_all_impl.rs
@@ -1,74 +1,30 @@
-use crate::fs::{
-    read_dir_unchecked, remove_dir, remove_file, remove_open_dir, stat, FollowSymlinks,
-};
+use super::get_path::get_path;
+use crate::fs::{open_dir, open_dir_nofollow, remove_dir, stat, FollowSymlinks};
 #[cfg(windows_file_type_ext)]
 use std::os::windows::fs::FileTypeExt;
-use std::path::{Component, Path};
+use std::path::Path;
 use std::{fs, io};
 
 pub(crate) fn remove_dir_all_impl(start: &fs::File, path: &Path) -> io::Result<()> {
-    // Code derived from `remove_dir_all` in Rust's
-    // library/std/src/sys/windows/fs.rs at revision
-    // 108e90ca78f052c0c1c49c42a22c85620be19712.
-    let filetype = stat(start, path, FollowSymlinks::No)?.file_type();
-    if filetype.is_symlink() {
-        // On Windows symlinks to files and directories are removed
-        // differently. `remove_dir` only deletes dir symlinks and junctions,
-        // not file symlinks.
+    // Open the directory, following symlinks, to make sure it is a directory.
+    let file = open_dir(start, path)?;
+    // Test whether the path is a symlink.
+    let md = stat(start, path, FollowSymlinks::No)?;
+    drop(file);
+    if md.is_symlink() {
+        // If so, just remove the link.
         remove_dir(start, path)
     } else {
-        remove_dir_all_recursive(start, path)?;
-        remove_dir(start, path)
+        // Otherwise, remove the tree.
+        let dir = open_dir_nofollow(start, path)?;
+        remove_open_dir_all_impl(dir)
     }
 }
 
 pub(crate) fn remove_open_dir_all_impl(dir: fs::File) -> io::Result<()> {
-    remove_dir_all_recursive(&dir, Component::CurDir.as_ref())?;
-    remove_open_dir(dir)
-}
-
-#[cfg(windows_file_type_ext)]
-fn remove_dir_all_recursive(start: &fs::File, path: &Path) -> io::Result<()> {
-    // Code derived from `remove_dir_all_recursive` in Rust's
-    // library/std/src/sys/windows/fs.rs at revision
-    // 108e90ca78f052c0c1c49c42a22c85620be19712.
-    for child in read_dir_unchecked(start, path)? {
-        let child = child?;
-        let child_type = child.file_type()?;
-        if child_type.is_dir() {
-            let path = path.join(child.file_name());
-            remove_dir_all_recursive(start, &path)?;
-            remove_dir(start, &path)?;
-        } else if child_type.is_symlink_dir() {
-            remove_dir(start, &path.join(child.file_name()))?;
-        } else {
-            remove_file(start, &path.join(child.file_name()))?;
-        }
-    }
-    Ok(())
-}
-
-#[cfg(not(windows_file_type_ext))]
-fn remove_dir_all_recursive(start: &fs::File, path: &Path) -> io::Result<()> {
-    for child in read_dir_unchecked(start, path)? {
-        let child = child?;
-        let child_type = child.file_type()?;
-        if child_type.is_dir() {
-            let path = path.join(child.file_name());
-            remove_dir_all_recursive(start, &path)?;
-            remove_dir(start, &path)?;
-        } else {
-            match remove_dir(start, &path.join(child.file_name())) {
-                Ok(()) => (),
-                Err(e) => {
-                    if e.raw_os_error() == Some(winapi::shared::winerror::ERROR_DIRECTORY as i32) {
-                        remove_file(start, &path.join(child.file_name()))?;
-                    } else {
-                        return Err(e);
-                    }
-                }
-            }
-        }
-    }
-    Ok(())
+    // Close the directory so that we can delete it. This is racy; see the
+    // comments in `remove_open_dir_impl` for details.
+    let path = get_path(&dir)?;
+    drop(dir);
+    fs::remove_dir_all(&path)
 }
diff --git a/cap-tempfile/src/lib.rs b/cap-tempfile/src/lib.rs
@@ -218,10 +218,11 @@ fn close_outer() {
     let t = tempdir(ambient_authority()).unwrap();
     let _s = tempdir_in(&t).unwrap();
     #[cfg(windows)]
-    assert_eq!(
-        t.close().unwrap_err().raw_os_error(),
-        Some(winapi::shared::winerror::ERROR_SHARING_VIOLATION as i32)
-    );
+    assert!(matches!(
+        t.close().unwrap_err().raw_os_error().map(|err| err as _),
+        Some(winapi::shared::winerror::ERROR_SHARING_VIOLATION)
+            | Some(winapi::shared::winerror::ERROR_DIR_NOT_EMPTY)
+    ));
     #[cfg(not(windows))]
     t.close().unwrap();
 }

diff --git a/cap-tempfile/src/utf8.rs b/cap-tempfile/src/utf8.rs
@@ -214,10 +214,11 @@ fn close_outer() {
     let t = tempdir(ambient_authority()).unwrap();
     let _s = tempdir_in(&t).unwrap();
     #[cfg(windows)]
-    assert_eq!(
-        t.close().unwrap_err().raw_os_error(),
-        Some(winapi::shared::winerror::ERROR_SHARING_VIOLATION as i32)
-    );
+    assert!(matches!(
+        t.close().unwrap_err().raw_os_error().map(|err| err as _),
+        Some(winapi::shared::winerror::ERROR_SHARING_VIOLATION)
+            | Some(winapi::shared::winerror::ERROR_DIR_NOT_EMPTY)
+    ));
     #[cfg(not(windows))]
     t.close().unwrap();
 }