Skip to content

Commit

Permalink
Rollup merge of rust-lang#127623 - lolbinarycat:fix_remove_dir_all, r…
Browse files Browse the repository at this point in the history
…=Amanieu

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

fixes rust-lang#127576

try-job: test-various
  • Loading branch information
jieyouxu authored Aug 20, 2024
2 parents fdf61d4 + 5fc3993 commit 9bc3a7c
Show file tree
Hide file tree
Showing 8 changed files with 141 additions and 36 deletions.
2 changes: 2 additions & 0 deletions library/std/src/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand Down
23 changes: 16 additions & 7 deletions library/std/src/sys/pal/solid/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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> {
Expand Down
49 changes: 34 additions & 15 deletions library/std/src/sys/pal/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand All @@ -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(())
}

Expand Down
20 changes: 14 additions & 6 deletions library/std/src/sys/pal/wasi/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 changes: 2 additions & 2 deletions library/std/src/sys/pal/windows/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
21 changes: 15 additions & 6 deletions library/std/src/sys_common/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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> {
Expand Down
8 changes: 8 additions & 0 deletions library/std/src/sys_common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}
50 changes: 50 additions & 0 deletions tests/run-make/remove-dir-all-race/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
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 the 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"
);
}
}

0 comments on commit 9bc3a7c

Please sign in to comment.