mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: fs::remove_dir_all: treat ENOENT as success

Browse Source 
fixes #127576

windows implementation still needs some work
This commit is contained in:
binarycat 2024-07-11 15:44:58 -04:00
parent 5367673014
commit 736f773844
8 changed files with 153 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
library/std/src/fs.rs
View File

@ -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
///

23
library/std/src/sys/pal/solid/fs.rs
View File

@ -10,6 +10,7 @@
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> {

49
library/std/src/sys/pal/unix/fs.rs
View File

@ -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<RawFd>, p: &CStr) -> io::Result<OwnedFd> {
let fd = cvt_r(|| unsafe {
@ -2082,6 +2083,16 @@ fn is_dir(ent: &DirEntry) -> Option<bool> {
}
}
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) {
@ -2105,27 +2116,35 @@ fn remove_dir_all_recursive(parent_fd: Option<RawFd>, path: &CStr) -> io::Result
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(())
}

20
library/std/src/sys/pal/wasi/fs.rs
View File

@ -13,7 +13,7 @@
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())?))
}

4
library/std/src/sys/pal/windows/fs.rs
View File

@ -14,7 +14,7 @@
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 {

21
library/std/src/sys_common/fs.rs
View File

@ -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> {

8
library/std/src/sys_common/mod.rs
View File

@ -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),
}
}

62
tests/run-make/remove-dir-all-race/rmake.rs Normal file
View File

@ -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"
);
}
}