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

matthiaskrgr · web-flow · commit 2c06146be107 · 2024-08-23T06:26:51.000+02:00
fix: fs::remove_dir_all: treat internal ENOENT as success fixes rust-lang#127576 try-job: test-various
diff --git a/std/src/fs.rs b/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
 ///
diff --git a/std/src/sys/pal/solid/fs.rs b/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> {
diff --git a/std/src/sys/pal/unix/fs.rs b/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(())
     }
 
diff --git a/std/src/sys/pal/wasi/fs.rs b/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())?))
 }
diff --git a/std/src/sys/pal/windows/fs.rs b/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 {
diff --git a/std/src/sys_common/fs.rs b/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> {
diff --git a/std/src/sys_common/mod.rs b/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),
+    }
+}
