Rollup merge of #127623 - lolbinarycat:fix_remove_dir_all, r=Amanieu

fix: fs::remove_dir_all: treat internal ENOENT as success

fixes #127576

try-job: test-various

Author: matthiaskrgr

library/std/src/fs.rs

@@ -2491,6 +2491,8 @@ pub fn remove_dir<P: AsRef<Path>>(path: P) -> io::Result<()> {
 ///
 /// Consider ignoring the error if validating the removal is not required for your use case.
 ///
+/// [`io::ErrorKind::NotFound`] is only returned if no removal occurs.
+///
 /// [`fs::remove_file`]: remove_file
 /// [`fs::remove_dir`]: remove_dir
 ///

library/std/src/sys/pal/solid/fs.rs

@@ -10,6 +10,7 @@ use crate::sync::Arc;
 use crate::sys::time::SystemTime;
 use crate::sys::unsupported;
 pub use crate::sys_common::fs::exists;
+use crate::sys_common::ignore_notfound;
 
 /// A file descriptor.
 #[derive(Clone, Copy)]
@@ -527,15 +528,23 @@ pub fn rmdir(p: &Path) -> io::Result<()> {
 
 pub fn remove_dir_all(path: &Path) -> io::Result<()> {
     for child in readdir(path)? {
-        let child = child?;
-        let child_type = child.file_type()?;
-        if child_type.is_dir() {
-            remove_dir_all(&child.path())?;
-        } else {
-            unlink(&child.path())?;
+        let result: io::Result<()> = try {
+            let child = child?;
+            let child_type = child.file_type()?;
+            if child_type.is_dir() {
+                remove_dir_all(&child.path())?;
+            } else {
+                unlink(&child.path())?;
+            }
+        };
+        // ignore internal NotFound errors
+        if let Err(err) = result
+            && err.kind() != io::ErrorKind::NotFound
+        {
+            return result;
         }
     }
-    rmdir(path)
+    ignore_notfound(rmdir(path))
 }
 
 pub fn readlink(p: &Path) -> io::Result<PathBuf> {

library/std/src/sys/pal/unix/fs.rs

@@ -2002,6 +2002,7 @@ mod remove_dir_impl {
     use crate::path::{Path, PathBuf};
     use crate::sys::common::small_c_string::run_path_with_cstr;
     use crate::sys::{cvt, cvt_r};
+    use crate::sys_common::ignore_notfound;
 
     pub fn openat_nofollow_dironly(parent_fd: Option<RawFd>, p: &CStr) -> io::Result<OwnedFd> {
         let fd = cvt_r(|| unsafe {
@@ -2055,6 +2056,16 @@ mod remove_dir_impl {
         }
     }
 
+    fn is_enoent(result: &io::Result<()>) -> bool {
+        if let Err(err) = result
+            && matches!(err.raw_os_error(), Some(libc::ENOENT))
+        {
+            true
+        } else {
+            false
+        }
+    }
+
     fn remove_dir_all_recursive(parent_fd: Option<RawFd>, path: &CStr) -> io::Result<()> {
         // try opening as directory
         let fd = match openat_nofollow_dironly(parent_fd, &path) {
@@ -2078,27 +2089,35 @@ mod remove_dir_impl {
         for child in dir {
             let child = child?;
             let child_name = child.name_cstr();
-            match is_dir(&child) {
-                Some(true) => {
-                    remove_dir_all_recursive(Some(fd), child_name)?;
-                }
-                Some(false) => {
-                    cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;
-                }
-                None => {
-                    // POSIX specifies that calling unlink()/unlinkat(..., 0) on a directory can succeed
-                    // if the process has the appropriate privileges. This however can causing orphaned
-                    // directories requiring an fsck e.g. on Solaris and Illumos. So we try recursing
-                    // into it first instead of trying to unlink() it.
-                    remove_dir_all_recursive(Some(fd), child_name)?;
+            // we need an inner try block, because if one of these
+            // directories has already been deleted, then we need to
+            // continue the loop, not return ok.
+            let result: io::Result<()> = try {
+                match is_dir(&child) {
+                    Some(true) => {
+                        remove_dir_all_recursive(Some(fd), child_name)?;
+                    }
+                    Some(false) => {
+                        cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;
+                    }
+                    None => {
+                        // POSIX specifies that calling unlink()/unlinkat(..., 0) on a directory can succeed
+                        // if the process has the appropriate privileges. This however can causing orphaned
+                        // directories requiring an fsck e.g. on Solaris and Illumos. So we try recursing
+                        // into it first instead of trying to unlink() it.
+                        remove_dir_all_recursive(Some(fd), child_name)?;
+                    }
                 }
+            };
+            if result.is_err() && !is_enoent(&result) {
+                return result;
             }
         }
 
         // unlink the directory after removing its contents
-        cvt(unsafe {
+        ignore_notfound(cvt(unsafe {
             unlinkat(parent_fd.unwrap_or(libc::AT_FDCWD), path.as_ptr(), libc::AT_REMOVEDIR)
-        })?;
+        }))?;
         Ok(())
     }
 

library/std/src/sys/pal/wasi/fs.rs

@@ -13,7 +13,7 @@ use crate::sys::common::small_c_string::run_path_with_cstr;
 use crate::sys::time::SystemTime;
 use crate::sys::unsupported;
 pub use crate::sys_common::fs::exists;
-use crate::sys_common::{AsInner, FromInner, IntoInner};
+use crate::sys_common::{ignore_notfound, AsInner, FromInner, IntoInner};
 use crate::{fmt, iter, ptr};
 
 pub struct File {
@@ -794,14 +794,22 @@ fn remove_dir_all_recursive(parent: &WasiFd, path: &Path) -> io::Result<()> {
             io::const_io_error!(io::ErrorKind::Uncategorized, "invalid utf-8 file name found")
         })?;
 
-        if entry.file_type()?.is_dir() {
-            remove_dir_all_recursive(&entry.inner.dir.fd, path.as_ref())?;
-        } else {
-            entry.inner.dir.fd.unlink_file(path)?;
+        let result: io::Result<()> = try {
+            if entry.file_type()?.is_dir() {
+                remove_dir_all_recursive(&entry.inner.dir.fd, path.as_ref())?;
+            } else {
+                entry.inner.dir.fd.unlink_file(path)?;
+            }
+        };
+        // ignore internal NotFound errors
+        if let Err(err) = &result
+            && err.kind() != io::ErrorKind::NotFound
+        {
+            return result;
         }
     }
 
     // Once all this directory's contents are deleted it should be safe to
     // delete the directory tiself.
-    parent.remove_directory(osstr2str(path.as_ref())?)
+    ignore_notfound(parent.remove_directory(osstr2str(path.as_ref())?))
 }

library/std/src/sys/pal/windows/fs.rs

@@ -14,7 +14,7 @@ use crate::sys::handle::Handle;
 use crate::sys::path::maybe_verbatim;
 use crate::sys::time::SystemTime;
 use crate::sys::{c, cvt, Align8};
-use crate::sys_common::{AsInner, FromInner, IntoInner};
+use crate::sys_common::{ignore_notfound, AsInner, FromInner, IntoInner};
 use crate::{fmt, ptr, slice, thread};
 
 pub struct File {
@@ -1160,7 +1160,7 @@ pub fn remove_dir_all(path: &Path) -> io::Result<()> {
         return Err(io::Error::from_raw_os_error(c::ERROR_DIRECTORY as _));
     }
 
-    match remove_dir_all_iterative(&file, File::posix_delete) {
+    match ignore_notfound(remove_dir_all_iterative(&file, File::posix_delete)) {
         Err(e) => {
             if let Some(code) = e.raw_os_error() {
                 match code as u32 {

library/std/src/sys_common/fs.rs

@@ -3,6 +3,7 @@
 use crate::fs;
 use crate::io::{self, Error, ErrorKind};
 use crate::path::Path;
+use crate::sys_common::ignore_notfound;
 
 pub(crate) const NOT_FILE_ERROR: Error = io::const_io_error!(
     ErrorKind::InvalidInput,
@@ -32,14 +33,22 @@ pub fn remove_dir_all(path: &Path) -> io::Result<()> {
 
 fn remove_dir_all_recursive(path: &Path) -> io::Result<()> {
     for child in fs::read_dir(path)? {
-        let child = child?;
-        if child.file_type()?.is_dir() {
-            remove_dir_all_recursive(&child.path())?;
-        } else {
-            fs::remove_file(&child.path())?;
+        let result: io::Result<()> = try {
+            let child = child?;
+            if child.file_type()?.is_dir() {
+                remove_dir_all_recursive(&child.path())?;
+            } else {
+                fs::remove_file(&child.path())?;
+            }
+        };
+        // ignore internal NotFound errors to prevent race conditions
+        if let Err(err) = &result
+            && err.kind() != io::ErrorKind::NotFound
+        {
+            return result;
         }
     }
-    fs::remove_dir(path)
+    ignore_notfound(fs::remove_dir(path))
 }
 
 pub fn exists(path: &Path) -> io::Result<bool> {

library/std/src/sys_common/mod.rs

@@ -80,3 +80,11 @@ pub fn mul_div_u64(value: u64, numer: u64, denom: u64) -> u64 {
     // r < denom, so (denom*numer) is the upper bound of (r*numer)
     q * numer + r * numer / denom
 }
+
+pub fn ignore_notfound<T>(result: crate::io::Result<T>) -> crate::io::Result<()> {
+    match result {
+        Err(err) if err.kind() == crate::io::ErrorKind::NotFound => Ok(()),
+        Ok(_) => Ok(()),
+        Err(err) => Err(err),
+    }
+}

tests/run-make/remove-dir-all-race/rmake.rs

@@ -0,0 +1,62 @@
+//@ ignore-windows
+
+// This test attempts to make sure that running `remove_dir_all`
+// doesn't result in a NotFound error one of the files it
+// is deleting is deleted concurrently.
+//
+// The windows implementation for `remove_dir_all` is significantly
+// more complicated, and has not yet been brought up to par with
+// the implementation on other platforms, so this test is marked as
+// `ignore-windows` until someone more expirenced with windows can
+// sort that out.
+
+use std::fs::remove_dir_all;
+use std::path::Path;
+use std::thread;
+use std::time::Duration;
+
+use run_make_support::rfs::{create_dir, write};
+use run_make_support::run_in_tmpdir;
+
+fn main() {
+    let mut race_happened = false;
+    run_in_tmpdir(|| {
+        for i in 0..150 {
+            create_dir("outer");
+            create_dir("outer/inner");
+            write("outer/inner.txt", b"sometext");
+
+            thread::scope(|scope| {
+                let t1 = scope.spawn(|| {
+                    thread::sleep(Duration::from_nanos(i));
+                    remove_dir_all("outer").unwrap();
+                });
+
+                let race_happened_ref = &race_happened;
+                let t2 = scope.spawn(|| {
+                    let r1 = remove_dir_all("outer/inner");
+                    let r2 = remove_dir_all("outer/inner.txt");
+                    if r1.is_ok() && r2.is_err() {
+                        race_happened = true;
+                    }
+                });
+            });
+
+            assert!(!Path::new("outer").exists());
+
+            // trying to remove a nonexistant top-level directory should
+            // still result in an error.
+            let Err(err) = remove_dir_all("outer") else {
+                panic!("removing nonexistant dir did not result in an error");
+            };
+            assert_eq!(err.kind(), std::io::ErrorKind::NotFound);
+        }
+    });
+    if !race_happened {
+        eprintln!(
+            "WARNING: multithreaded deletion never raced, \
+                   try increasing the number of attempts or \
+                   adjusting the sleep timing"
+        );
+    }
+}