From 736f773844e7ebf05ccb827c17b7ad9eb28aa295 Mon Sep 17 00:00:00 2001 From: binarycat Date: Thu, 11 Jul 2024 15:44:58 -0400 Subject: [PATCH] fix: fs::remove_dir_all: treat ENOENT as success fixes #127576 windows implementation still needs some work --- library/std/src/fs.rs | 2 + library/std/src/sys/pal/solid/fs.rs | 23 +++++--- library/std/src/sys/pal/unix/fs.rs | 49 +++++++++++----- library/std/src/sys/pal/wasi/fs.rs | 20 +++++-- library/std/src/sys/pal/windows/fs.rs | 4 +- library/std/src/sys_common/fs.rs | 21 +++++-- library/std/src/sys_common/mod.rs | 8 +++ tests/run-make/remove-dir-all-race/rmake.rs | 62 +++++++++++++++++++++ 8 files changed, 153 insertions(+), 36 deletions(-) create mode 100644 tests/run-make/remove-dir-all-race/rmake.rs diff --git a/library/std/src/fs.rs b/library/std/src/fs.rs index c5edb03bb08be..6a0d9f47960ec 100644 --- a/library/std/src/fs.rs +++ b/library/std/src/fs.rs @@ -2491,6 +2491,8 @@ pub fn remove_dir>(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/library/std/src/sys/pal/solid/fs.rs b/library/std/src/sys/pal/solid/fs.rs index 8179ec8821a38..591be66fcb463 100644 --- a/library/std/src/sys/pal/solid/fs.rs +++ b/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 { diff --git a/library/std/src/sys/pal/unix/fs.rs b/library/std/src/sys/pal/unix/fs.rs index bdb83f0785784..acd9c338f05a6 100644 --- a/library/std/src/sys/pal/unix/fs.rs +++ b/library/std/src/sys/pal/unix/fs.rs @@ -2029,6 +2029,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, p: &CStr) -> io::Result { let fd = cvt_r(|| unsafe { @@ -2082,6 +2083,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, path: &CStr) -> io::Result<()> { // try opening as directory let fd = match openat_nofollow_dironly(parent_fd, &path) { @@ -2105,27 +2116,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/library/std/src/sys/pal/wasi/fs.rs b/library/std/src/sys/pal/wasi/fs.rs index 11900886f0b5c..7064473f274e6 100644 --- a/library/std/src/sys/pal/wasi/fs.rs +++ b/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())?)) } diff --git a/library/std/src/sys/pal/windows/fs.rs b/library/std/src/sys/pal/windows/fs.rs index d99d4931de40f..2134152ea93f1 100644 --- a/library/std/src/sys/pal/windows/fs.rs +++ b/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 { diff --git a/library/std/src/sys_common/fs.rs b/library/std/src/sys_common/fs.rs index acb6713cf1b14..a25a7244660bb 100644 --- a/library/std/src/sys_common/fs.rs +++ b/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 { diff --git a/library/std/src/sys_common/mod.rs b/library/std/src/sys_common/mod.rs index 60ee405ecaaa2..1c884f107beeb 100644 --- a/library/std/src/sys_common/mod.rs +++ b/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(result: crate::io::Result) -> crate::io::Result<()> { + match result { + Err(err) if err.kind() == crate::io::ErrorKind::NotFound => Ok(()), + Ok(_) => Ok(()), + Err(err) => Err(err), + } +} diff --git a/tests/run-make/remove-dir-all-race/rmake.rs b/tests/run-make/remove-dir-all-race/rmake.rs new file mode 100644 index 0000000000000..03c94b76127d5 --- /dev/null +++ b/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" + ); + } +}