From 9dc00987a235e32ec1c29927bc25737a91ade29a Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Thu, 7 Apr 2022 11:33:02 +0200 Subject: [PATCH 1/9] Make UNIX remove_dir_all() looping, use I/O safety move remove_dir_all implementation to new dir_fd module --- library/std/src/sys/unix/fs.rs | 252 ++--------------- library/std/src/sys/unix/fs/dir_fd.rs | 384 ++++++++++++++++++++++++++ library/std/src/sys/unix/mod.rs | 7 + 3 files changed, 421 insertions(+), 222 deletions(-) create mode 100644 library/std/src/sys/unix/fs/dir_fd.rs diff --git a/library/std/src/sys/unix/fs.rs b/library/std/src/sys/unix/fs.rs index 8b0bbd6a55c6b..e6d6d01ebb552 100644 --- a/library/std/src/sys/unix/fs.rs +++ b/library/std/src/sys/unix/fs.rs @@ -75,6 +75,17 @@ use libc::{ #[cfg(any(target_os = "linux", target_os = "emscripten", target_os = "l4re"))] use libc::{dirent64, fstat64, ftruncate64, lseek64, lstat64, off64_t, open64, stat64}; +#[cfg(not(any(target_os = "redox", target_os = "espidf")))] +mod dir_fd; + +// Modern implementation using openat(), unlinkat() and fdopendir() +#[cfg(not(any(target_os = "redox", target_os = "espidf", target_os = "horizon", miri)))] +pub use dir_fd::remove_dir_all; + +// Fallback for REDOX, ESP-IDF, Horizon and Miri +#[cfg(any(target_os = "redox", target_os = "espidf", target_os = "horizon", miri))] +pub use crate::sys_common::fs::remove_dir_all; + pub use crate::sys_common::fs::try_exists; pub struct File(FileDesc); @@ -525,6 +536,24 @@ impl FromInner for FilePermissions { } } +impl ReadDir { + fn new(dirp: Dir, root: PathBuf) -> Self { + let inner = InnerReadDir { dirp, root }; + ReadDir { + inner: Arc::new(inner), + #[cfg(not(any( + target_os = "android", + target_os = "linux", + target_os = "solaris", + target_os = "illumos", + target_os = "fuchsia", + target_os = "redox", + )))] + end_of_stream: false, + } + } +} + impl fmt::Debug for ReadDir { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { // This will only be called from std::fs::ReadDir, which will add a "ReadDir()" frame. @@ -1179,23 +1208,7 @@ pub fn readdir(p: &Path) -> io::Result { let p = cstr(p)?; unsafe { let ptr = libc::opendir(p.as_ptr()); - if ptr.is_null() { - Err(Error::last_os_error()) - } else { - let inner = InnerReadDir { dirp: Dir(ptr), root }; - Ok(ReadDir { - inner: Arc::new(inner), - #[cfg(not(any( - target_os = "android", - target_os = "linux", - target_os = "solaris", - target_os = "illumos", - target_os = "fuchsia", - target_os = "redox", - )))] - end_of_stream: false, - }) - } + if ptr.is_null() { Err(Error::last_os_error()) } else { Ok(ReadDir::new(Dir(ptr), root)) } } } @@ -1557,208 +1570,3 @@ pub fn chroot(dir: &Path) -> io::Result<()> { cvt(unsafe { libc::chroot(dir.as_ptr()) })?; Ok(()) } - -pub use remove_dir_impl::remove_dir_all; - -// Fallback for REDOX, ESP-ID, Horizon, and Miri -#[cfg(any(target_os = "redox", target_os = "espidf", target_os = "horizon", miri))] -mod remove_dir_impl { - pub use crate::sys_common::fs::remove_dir_all; -} - -// Modern implementation using openat(), unlinkat() and fdopendir() -#[cfg(not(any(target_os = "redox", target_os = "espidf", target_os = "horizon", miri)))] -mod remove_dir_impl { - use super::{cstr, lstat, Dir, DirEntry, InnerReadDir, ReadDir}; - use crate::ffi::CStr; - use crate::io; - use crate::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd}; - use crate::os::unix::prelude::{OwnedFd, RawFd}; - use crate::path::{Path, PathBuf}; - use crate::sync::Arc; - use crate::sys::{cvt, cvt_r}; - - #[cfg(not(all(target_os = "macos", not(target_arch = "aarch64")),))] - use libc::{fdopendir, openat, unlinkat}; - #[cfg(all(target_os = "macos", not(target_arch = "aarch64")))] - use macos_weak::{fdopendir, openat, unlinkat}; - - #[cfg(all(target_os = "macos", not(target_arch = "aarch64")))] - mod macos_weak { - use crate::sys::weak::weak; - use libc::{c_char, c_int, DIR}; - - fn get_openat_fn() -> Option c_int> { - weak!(fn openat(c_int, *const c_char, c_int) -> c_int); - openat.get() - } - - pub fn has_openat() -> bool { - get_openat_fn().is_some() - } - - pub unsafe fn openat(dirfd: c_int, pathname: *const c_char, flags: c_int) -> c_int { - get_openat_fn().map(|openat| openat(dirfd, pathname, flags)).unwrap_or_else(|| { - crate::sys::unix::os::set_errno(libc::ENOSYS); - -1 - }) - } - - pub unsafe fn fdopendir(fd: c_int) -> *mut DIR { - #[cfg(all(target_os = "macos", target_arch = "x86"))] - weak!(fn fdopendir(c_int) -> *mut DIR, "fdopendir$INODE64$UNIX2003"); - #[cfg(all(target_os = "macos", target_arch = "x86_64"))] - weak!(fn fdopendir(c_int) -> *mut DIR, "fdopendir$INODE64"); - fdopendir.get().map(|fdopendir| fdopendir(fd)).unwrap_or_else(|| { - crate::sys::unix::os::set_errno(libc::ENOSYS); - crate::ptr::null_mut() - }) - } - - pub unsafe fn unlinkat(dirfd: c_int, pathname: *const c_char, flags: c_int) -> c_int { - weak!(fn unlinkat(c_int, *const c_char, c_int) -> c_int); - unlinkat.get().map(|unlinkat| unlinkat(dirfd, pathname, flags)).unwrap_or_else(|| { - crate::sys::unix::os::set_errno(libc::ENOSYS); - -1 - }) - } - } - - pub fn openat_nofollow_dironly(parent_fd: Option, p: &CStr) -> io::Result { - let fd = cvt_r(|| unsafe { - openat( - parent_fd.unwrap_or(libc::AT_FDCWD), - p.as_ptr(), - libc::O_CLOEXEC | libc::O_RDONLY | libc::O_NOFOLLOW | libc::O_DIRECTORY, - ) - })?; - Ok(unsafe { OwnedFd::from_raw_fd(fd) }) - } - - fn fdreaddir(dir_fd: OwnedFd) -> io::Result<(ReadDir, RawFd)> { - let ptr = unsafe { fdopendir(dir_fd.as_raw_fd()) }; - if ptr.is_null() { - return Err(io::Error::last_os_error()); - } - let dirp = Dir(ptr); - // file descriptor is automatically closed by libc::closedir() now, so give up ownership - let new_parent_fd = dir_fd.into_raw_fd(); - // a valid root is not needed because we do not call any functions involving the full path - // of the DirEntrys. - let dummy_root = PathBuf::new(); - Ok(( - ReadDir { - inner: Arc::new(InnerReadDir { dirp, root: dummy_root }), - #[cfg(not(any( - target_os = "android", - target_os = "linux", - target_os = "solaris", - target_os = "illumos", - target_os = "fuchsia", - target_os = "redox", - )))] - end_of_stream: false, - }, - new_parent_fd, - )) - } - - #[cfg(any( - target_os = "solaris", - target_os = "illumos", - target_os = "haiku", - target_os = "vxworks", - ))] - fn is_dir(_ent: &DirEntry) -> Option { - None - } - - #[cfg(not(any( - target_os = "solaris", - target_os = "illumos", - target_os = "haiku", - target_os = "vxworks", - )))] - fn is_dir(ent: &DirEntry) -> Option { - match ent.entry.d_type { - libc::DT_UNKNOWN => None, - libc::DT_DIR => Some(true), - _ => Some(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) { - Err(err) if matches!(err.raw_os_error(), Some(libc::ENOTDIR | libc::ELOOP)) => { - // not a directory - don't traverse further - // (for symlinks, older Linux kernels may return ELOOP instead of ENOTDIR) - return match parent_fd { - // unlink... - Some(parent_fd) => { - cvt(unsafe { unlinkat(parent_fd, path.as_ptr(), 0) }).map(drop) - } - // ...unless this was supposed to be the deletion root directory - None => Err(err), - }; - } - result => result?, - }; - - // open the directory passing ownership of the fd - let (dir, fd) = fdreaddir(fd)?; - 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)?; - } - } - } - - // unlink the directory after removing its contents - cvt(unsafe { - unlinkat(parent_fd.unwrap_or(libc::AT_FDCWD), path.as_ptr(), libc::AT_REMOVEDIR) - })?; - Ok(()) - } - - fn remove_dir_all_modern(p: &Path) -> io::Result<()> { - // We cannot just call remove_dir_all_recursive() here because that would not delete a passed - // symlink. No need to worry about races, because remove_dir_all_recursive() does not recurse - // into symlinks. - let attr = lstat(p)?; - if attr.file_type().is_symlink() { - crate::fs::remove_file(p) - } else { - remove_dir_all_recursive(None, &cstr(p)?) - } - } - - #[cfg(not(all(target_os = "macos", not(target_arch = "aarch64"))))] - pub fn remove_dir_all(p: &Path) -> io::Result<()> { - remove_dir_all_modern(p) - } - - #[cfg(all(target_os = "macos", not(target_arch = "aarch64")))] - pub fn remove_dir_all(p: &Path) -> io::Result<()> { - if macos_weak::has_openat() { - // openat() is available with macOS 10.10+, just like unlinkat() and fdopendir() - remove_dir_all_modern(p) - } else { - // fall back to classic implementation - crate::sys_common::fs::remove_dir_all(p) - } - } -} diff --git a/library/std/src/sys/unix/fs/dir_fd.rs b/library/std/src/sys/unix/fs/dir_fd.rs new file mode 100644 index 0000000000000..bb877240cb01e --- /dev/null +++ b/library/std/src/sys/unix/fs/dir_fd.rs @@ -0,0 +1,384 @@ +use crate::io; +use crate::path::Path; + +#[cfg(any( + target_os = "linux", + target_os = "emscripten", + target_os = "l4re", + target_os = "android", +))] +use libc::{fdopendir, fstat64 as fstat, openat64 as openat, unlinkat}; + +#[cfg(not(any( + target_os = "linux", + target_os = "emscripten", + target_os = "l4re", + target_os = "android", + all(target_os = "macos", not(target_arch = "aarch64")) +)))] +use libc::{fdopendir, fstat, openat, unlinkat}; + +#[cfg(all(target_os = "macos", not(target_arch = "aarch64")))] +use { + libc::fstat, + macos_compat::{fdopendir, openat, unlinkat}, +}; + +// Dynamically resolve openat, fdopendir and unlinkat on macOS as those functions are only +// available starting with macOS 10.10. This is not necessarry for aarch64 as the first +// version supporting it was macOS 11. +#[cfg(all(target_os = "macos", not(target_arch = "aarch64")))] +mod macos_compat { + use crate::sys::weak::weak; + use libc::{c_char, c_int, DIR}; + + fn get_openat_fn() -> Option c_int> { + weak!(fn openat(c_int, *const c_char, c_int) -> c_int); + openat.get() + } + + pub unsafe fn openat(dirfd: c_int, pathname: *const c_char, flags: c_int) -> c_int { + get_openat_fn().map(|openat| openat(dirfd, pathname, flags)).unwrap_or_else(|| { + crate::sys::unix::os::set_errno(libc::ENOSYS); + -1 + }) + } + + pub unsafe fn fdopendir(fd: c_int) -> *mut DIR { + #[cfg(target_arch = "x86")] + weak!(fn fdopendir(c_int) -> *mut DIR, "fdopendir$INODE64$UNIX2003"); + #[cfg(target_arch = "x86_64")] + weak!(fn fdopendir(c_int) -> *mut DIR, "fdopendir$INODE64"); + fdopendir.get().map(|fdopendir| fdopendir(fd)).unwrap_or_else(|| { + crate::sys::unix::os::set_errno(libc::ENOSYS); + crate::ptr::null_mut() + }) + } + + pub unsafe fn unlinkat(dirfd: c_int, pathname: *const c_char, flags: c_int) -> c_int { + weak!(fn unlinkat(c_int, *const c_char, c_int) -> c_int); + unlinkat.get().map(|unlinkat| unlinkat(dirfd, pathname, flags)).unwrap_or_else(|| { + crate::sys::unix::os::set_errno(libc::ENOSYS); + -1 + }) + } + + pub fn has_openat() -> bool { + get_openat_fn().is_some() + } +} + +// TOCTOU-resistant implementation using openat(), unlinkat() and fdopendir() +mod remove_dir_all_xat { + use super::{fdopendir, fstat, openat, unlinkat}; + use crate::ffi::{CStr, CString}; + use crate::io; + use crate::mem; + use crate::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd}; + use crate::path::{Path, PathBuf}; + use crate::sys::unix::fs::{cstr, lstat, Dir, DirEntry, ReadDir}; + use crate::sys::{cvt, cvt_p, cvt_r}; + use alloc::collections::VecDeque; + use libc::dev_t; + + #[cfg(not(any( + target_os = "linux", + target_os = "emscripten", + target_os = "l4re", + target_os = "android", + )))] + use libc::ino_t; + + #[cfg(any( + target_os = "linux", + target_os = "emscripten", + target_os = "l4re", + target_os = "android" + ))] + use libc::ino64_t as ino_t; + + fn openat_nofollow_dironly(parent_fd: Option>, p: &CStr) -> io::Result { + let fd = cvt_r(|| unsafe { + openat( + parent_fd.map(|fd| fd.as_raw_fd()).unwrap_or(libc::AT_FDCWD), + p.as_ptr(), + libc::O_CLOEXEC | libc::O_RDONLY | libc::O_NOFOLLOW | libc::O_DIRECTORY, + ) + })?; + // SAFETY: file descriptor was opened in this fn + Ok(unsafe { OwnedFd::from_raw_fd(fd) }) + } + + #[cfg(any( + target_os = "solaris", + target_os = "illumos", + target_os = "haiku", + target_os = "vxworks", + ))] + fn is_dir(_ent: &DirEntry) -> Option { + None + } + + #[cfg(not(any( + target_os = "solaris", + target_os = "illumos", + target_os = "haiku", + target_os = "vxworks", + )))] + fn is_dir(ent: &DirEntry) -> Option { + match ent.entry.d_type { + libc::DT_UNKNOWN => None, + libc::DT_DIR => Some(true), + _ => Some(false), + } + } + + enum LazyReadDir<'a> { + Fd(Option), // only Fd(None) temporarily in ensure_open() + OpenReadDir(ReadDir, BorrowedFd<'a>), // also store the fd to avoid having to call dirfd(3) + } + + impl LazyReadDir<'_> { + fn open(path: &CStr) -> io::Result { + let fd = openat_nofollow_dironly(None, path)?; + Ok(LazyReadDir::Fd(Some(fd))) + } + + fn open_subdir_or_unlink_non_dir(&self, child_name: &CStr) -> io::Result> { + let fd = match openat_nofollow_dironly(Some(self.as_fd()), child_name) { + Ok(fd) => fd, + Err(err) if err.raw_os_error() == Some(libc::ENOTDIR) => { + // not a directory - unlink and return + cvt(unsafe { unlinkat(self.as_fd().as_raw_fd(), child_name.as_ptr(), 0) })?; + return Ok(None); + } + Err(err) => return Err(err), + }; + Ok(Some(LazyReadDir::Fd(Some(fd)))) + } + + fn get_parent(&self) -> io::Result { + let fd = openat_nofollow_dironly(Some(self.as_fd()), unsafe { + CStr::from_bytes_with_nul_unchecked(b"..\0") + })?; + Ok(LazyReadDir::Fd(Some(fd))) + } + + fn ensure_open(&mut self) -> io::Result<()> { + if let LazyReadDir::Fd(fd_opt) = self { + let fd = fd_opt.take().unwrap(); + let ptr = cvt_p(unsafe { fdopendir(fd.as_raw_fd()) }); + if let Err(err) = ptr { + *fd_opt = Some(fd); // put the fd back + return Err(err); + } + let dirp = Dir(ptr?); + // fd is now owned by dirp which closes it on drop, so give up ownership + let fd = fd.into_raw_fd(); + // SAFETY: the dirp fd stays valid until self is dropped + let fd = unsafe { BorrowedFd::borrow_raw(fd) }; + // a valid root path is not needed because we do not call any functions + // involving the full path of the DirEntrys. + let dummy_root = PathBuf::new(); + *self = LazyReadDir::OpenReadDir(ReadDir::new(dirp, dummy_root), fd); + } + Ok(()) + } + + fn is_open(&self) -> bool { + match self { + LazyReadDir::OpenReadDir(_, _) => true, + _ => false, + } + } + } + + impl AsFd for LazyReadDir<'_> { + fn as_fd(&self) -> BorrowedFd<'_> { + match self { + LazyReadDir::Fd(Some(fd)) => fd.as_fd(), + LazyReadDir::Fd(None) => { + panic!("LazyReadDir::as_fd() called, but no fd present") + } + LazyReadDir::OpenReadDir(_, fd) => *fd, + } + } + } + + impl Iterator for LazyReadDir<'_> { + type Item = io::Result; + + fn next(&mut self) -> Option> { + if let Err(err) = self.ensure_open() { + return Some(Err(err)); + } + match self { + LazyReadDir::OpenReadDir(rd, _) => rd.next(), + _ => { + unreachable!(); + } + } + } + } + + struct PathComponent { + name: CString, + dev: dev_t, + ino: ino_t, + } + + impl PathComponent { + fn from_name_and_fd(name: &CStr, fd: BorrowedFd<'_>) -> io::Result { + let mut stat = unsafe { mem::zeroed() }; + cvt(unsafe { fstat(fd.as_raw_fd(), &mut stat) })?; + Ok(PathComponent { name: name.to_owned(), dev: stat.st_dev, ino: stat.st_ino }) + } + + fn verify_dev_ino(&self, fd: BorrowedFd<'_>) -> io::Result<()> { + let mut stat = unsafe { mem::zeroed() }; + cvt(unsafe { fstat(fd.as_raw_fd(), &mut stat) })?; + // Make sure that the reopened directory has the same inode as when we visited it descending + // the directory tree. More detailed risk analysis TBD. + if self.dev != stat.st_dev || self.ino != stat.st_ino { + return Err(io::Error::new( + io::ErrorKind::Uncategorized, + "directory with unexpected dev/inode pair", + )); + } + Ok(()) + } + } + + fn remove_dir_all_loop(root: &Path) -> io::Result<()> { + // VecDeque allocates space for 2^n elements if the capacity is 2^n-1. + const MAX_OPEN_FDS: usize = 15; + + // all ancestor names and (dev, inode) pairs from the deletion root directory to the + // parent of the directory currently being processed + let mut path_components = Vec::new(); + // cache of the last up to MAX_OPEN_FDS ancestor ReadDirs and associated file descriptors + let mut readdir_cache = VecDeque::with_capacity(MAX_OPEN_FDS); + // the readdir, currently processed + let mut current_readdir = LazyReadDir::open(&cstr(root)?)?; + // the directory name, inode pair currently being processed + let mut current_path_component = PathComponent::from_name_and_fd( + unsafe { CStr::from_bytes_with_nul_unchecked(b"\0") }, + current_readdir.as_fd(), + )?; + let root_parent_component = PathComponent::from_name_and_fd( + unsafe { CStr::from_bytes_with_nul_unchecked(b"\0") }, + current_readdir.get_parent()?.as_fd(), + )?; + loop { + while let Some(child) = current_readdir.next() { + let child = child?; + let child_name = child.name_cstr(); + if let Some(false) = is_dir(&child) { + // just unlink files + cvt(unsafe { + unlinkat(current_readdir.as_fd().as_raw_fd(), child_name.as_ptr(), 0) + })?; + } else { + if let Some(child_readdir) = + current_readdir.open_subdir_or_unlink_non_dir(child_name)? + { + // descend into this child directory + + let child_path_compoment = + PathComponent::from_name_and_fd(child_name, child_readdir.as_fd())?; + path_components.push(current_path_component); + current_path_component = child_path_compoment; + + // avoid growing the cache over capacity + if readdir_cache.len() == readdir_cache.capacity() { + readdir_cache.pop_front(); + } + readdir_cache.push_back(current_readdir); + current_readdir = child_readdir; + } + } + } + + match path_components.pop() { + Some(parent_component) => { + // going back up... + + // get parent directory readdir + let parent_readdir = match readdir_cache.pop_back() { + Some(readdir) => readdir, + None => { + // cache is empty - reopen parent + let parent_readdir = current_readdir.get_parent()?; + parent_component.verify_dev_ino(parent_readdir.as_fd())?; + parent_readdir + } + }; + + // remove now empty directory + cvt(unsafe { + unlinkat( + parent_readdir.as_fd().as_raw_fd(), + current_path_component.name.as_ptr(), + libc::AT_REMOVEDIR, + ) + })?; + + current_path_component = parent_component; + current_readdir = parent_readdir; + + // Let "child directory" be the directory that was just deleted and "parent directory" + // the parent of "child directory" which is now referred to in current_*. + // If we don't have readdir open for the parent directory that means we got the file + // descriptor via openat(dirfd, ".."). To make sure the that the child directory + // was not moved somewhere else and the parent just happens to have the same reused + // (dev, inode) pair, that we found descending, we check the parent directory + // (dev, inode) as well. + if !current_readdir.is_open() && readdir_cache.is_empty() { + if let Some(parent_component) = path_components.last() { + let parent_readdir = current_readdir.get_parent()?; + parent_component.verify_dev_ino(parent_readdir.as_fd())?; + readdir_cache.push_back(parent_readdir); + } else { + // verify parent of the deletion root directory + let parent_readdir = current_readdir.get_parent()?; + root_parent_component.verify_dev_ino(parent_readdir.as_fd())?; + } + } + } + None => break, + } + } + + // unlink deletion root directory + cvt(unsafe { unlinkat(libc::AT_FDCWD, cstr(root)?.as_ptr(), libc::AT_REMOVEDIR) })?; + Ok(()) + } + + pub fn remove_dir_all(p: &Path) -> io::Result<()> { + // We cannot just call remove_dir_all_loop() here because that would not delete a passed + // symlink. remove_dir_all_loop() does not descend into symlinks and does not delete p + // if it is a file. + let attr = lstat(p)?; + if attr.file_type().is_symlink() { + crate::fs::remove_file(p) + } else { + remove_dir_all_loop(p) + } + } +} + +#[cfg(not(all(target_os = "macos", target_arch = "x86_64")))] +pub fn remove_dir_all(p: &Path) -> io::Result<()> { + remove_dir_all_xat::remove_dir_all(p) +} + +#[cfg(all(target_os = "macos", target_arch = "x86_64"))] +pub fn remove_dir_all(p: &Path) -> io::Result<()> { + if macos_compat::has_openat() { + // openat(), unlinkat() and fdopendir() all appeared in macOS x86-64 10.10+ + remove_dir_all_xat::remove_dir_all(p) + } else { + // fall back to classic implementation + crate::sys_common::fs::remove_dir_all(p) + } +} diff --git a/library/std/src/sys/unix/mod.rs b/library/std/src/sys/unix/mod.rs index 34a023b02c4fe..26428f895fda8 100644 --- a/library/std/src/sys/unix/mod.rs +++ b/library/std/src/sys/unix/mod.rs @@ -253,6 +253,13 @@ pub fn cvt_nz(error: libc::c_int) -> crate::io::Result<()> { if error == 0 { Ok(()) } else { Err(crate::io::Error::from_raw_os_error(error)) } } +pub fn cvt_p(ptr: *mut T) -> crate::io::Result<*mut T> { + if ptr.is_null() { + return Err(crate::io::Error::last_os_error()); + } + Ok(ptr) +} + // libc::abort() will run the SIGABRT handler. That's fine because anyone who // installs a SIGABRT handler already has to expect it to run in Very Bad // situations (eg, malloc crashing). From e9f2466692730311fcd75191c961065905c31859 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Sun, 6 Mar 2022 07:58:40 +0100 Subject: [PATCH 2/9] add unit test --- .../stdlib-unit-tests/remove-dir-all-deep.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 src/test/ui/stdlib-unit-tests/remove-dir-all-deep.rs diff --git a/src/test/ui/stdlib-unit-tests/remove-dir-all-deep.rs b/src/test/ui/stdlib-unit-tests/remove-dir-all-deep.rs new file mode 100644 index 0000000000000..47d9194504667 --- /dev/null +++ b/src/test/ui/stdlib-unit-tests/remove-dir-all-deep.rs @@ -0,0 +1,36 @@ +// run-pass + +use std::env::{current_dir, set_current_dir}; +use std::fs::{create_dir, remove_dir_all, File}; +use std::path::Path; + +pub fn main() { + let saved_cwd = current_dir().unwrap(); + if !Path::exists(Path::new("tmpdir")) { + create_dir("tmpdir").unwrap(); + } + set_current_dir("tmpdir").unwrap(); + let depth = if cfg!(target_os = "linux") { + // Should work on all Linux filesystems. + 4096 + } else if cfg!(target_os = "macos") { + // On Macos increasing depth leads to a superlinear slowdown. + 1024 + } else if cfg!(unix) { + // Should be no problem on other UNIXes either. + 1024 + } else { + // "Safe" fallback for other platforms. + 64 + }; + for _ in 0..depth { + if !Path::exists(Path::new("a")) { + create_dir("empty_dir").unwrap(); + File::create("empty_file").unwrap(); + create_dir("a").unwrap(); + } + set_current_dir("a").unwrap(); + } + set_current_dir(saved_cwd).unwrap(); + remove_dir_all("tmpdir").unwrap(); +} From 40ecc41d59e23a4ab9f71cf7c6f831ef000506a8 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Thu, 21 Apr 2022 08:02:01 +0200 Subject: [PATCH 3/9] apply #96234: remove_dir_all_recursive: treat ELOOP the same as ENOTDIR #96234 --- library/std/src/sys/unix/fs/dir_fd.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/fs/dir_fd.rs b/library/std/src/sys/unix/fs/dir_fd.rs index bb877240cb01e..e114c792f3cdc 100644 --- a/library/std/src/sys/unix/fs/dir_fd.rs +++ b/library/std/src/sys/unix/fs/dir_fd.rs @@ -147,8 +147,9 @@ mod remove_dir_all_xat { fn open_subdir_or_unlink_non_dir(&self, child_name: &CStr) -> io::Result> { let fd = match openat_nofollow_dironly(Some(self.as_fd()), child_name) { Ok(fd) => fd, - Err(err) if err.raw_os_error() == Some(libc::ENOTDIR) => { + Err(err) if matches!(err.raw_os_error(), Some(libc::ENOTDIR | libc::ELOOP)) => { // not a directory - unlink and return + // (for symlinks, older Linux kernels may return ELOOP instead of ENOTDIR) cvt(unsafe { unlinkat(self.as_fd().as_raw_fd(), child_name.as_ptr(), 0) })?; return Ok(None); } From f68ee84d385266c4bf62e69e9359a540af1152df Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Thu, 19 May 2022 09:09:12 +0200 Subject: [PATCH 4/9] fix nits --- library/std/src/sys/unix/fs.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/unix/fs.rs b/library/std/src/sys/unix/fs.rs index e6d6d01ebb552..2fb7712bd316e 100644 --- a/library/std/src/sys/unix/fs.rs +++ b/library/std/src/sys/unix/fs.rs @@ -10,7 +10,7 @@ use crate::ptr; use crate::sync::Arc; use crate::sys::fd::FileDesc; use crate::sys::time::SystemTime; -use crate::sys::{cvt, cvt_r}; +use crate::sys::{cvt, cvt_p, cvt_r}; use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner}; #[cfg(any( @@ -75,7 +75,7 @@ use libc::{ #[cfg(any(target_os = "linux", target_os = "emscripten", target_os = "l4re"))] use libc::{dirent64, fstat64, ftruncate64, lseek64, lstat64, off64_t, open64, stat64}; -#[cfg(not(any(target_os = "redox", target_os = "espidf")))] +#[cfg(not(any(target_os = "redox", target_os = "espidf", miri)))] mod dir_fd; // Modern implementation using openat(), unlinkat() and fdopendir() @@ -1207,8 +1207,8 @@ pub fn readdir(p: &Path) -> io::Result { let root = p.to_path_buf(); let p = cstr(p)?; unsafe { - let ptr = libc::opendir(p.as_ptr()); - if ptr.is_null() { Err(Error::last_os_error()) } else { Ok(ReadDir::new(Dir(ptr), root)) } + let ptr = cvt_p(libc::opendir(p.as_ptr()))?; + Ok(ReadDir::new(Dir(ptr), root)) } } From 3647c3443065281acc9400c7d4f73736e0c54b9d Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Thu, 19 May 2022 09:09:42 +0200 Subject: [PATCH 5/9] doc comment for LazyReadDir variants --- library/std/src/sys/unix/fs/dir_fd.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/library/std/src/sys/unix/fs/dir_fd.rs b/library/std/src/sys/unix/fs/dir_fd.rs index e114c792f3cdc..f0dc18bb29741 100644 --- a/library/std/src/sys/unix/fs/dir_fd.rs +++ b/library/std/src/sys/unix/fs/dir_fd.rs @@ -134,8 +134,14 @@ mod remove_dir_all_xat { } enum LazyReadDir<'a> { - Fd(Option), // only Fd(None) temporarily in ensure_open() - OpenReadDir(ReadDir, BorrowedFd<'a>), // also store the fd to avoid having to call dirfd(3) + /// Contains the file descriptor of the directory, while it has not been opened for reading. + /// It is only `Fd(None)` temporarily in `ensure_open()`. + Fd(Option), + + // Contains the `ReadDir` for the directory while it is being read. The ReadDir does not contain + // a valid `root` path, because it is not needed. It also contains the file descriptor of the + // directory to avoid calls to dirfd(3). + OpenReadDir(ReadDir, BorrowedFd<'a>), } impl LazyReadDir<'_> { From 83fb4c477a26b98357389d6bc5bb9d35589396c2 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Thu, 19 May 2022 09:10:04 +0200 Subject: [PATCH 6/9] doc comment cleanup --- library/std/src/sys/unix/fs/dir_fd.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/fs/dir_fd.rs b/library/std/src/sys/unix/fs/dir_fd.rs index f0dc18bb29741..977a6f541742a 100644 --- a/library/std/src/sys/unix/fs/dir_fd.rs +++ b/library/std/src/sys/unix/fs/dir_fd.rs @@ -245,7 +245,7 @@ mod remove_dir_all_xat { let mut stat = unsafe { mem::zeroed() }; cvt(unsafe { fstat(fd.as_raw_fd(), &mut stat) })?; // Make sure that the reopened directory has the same inode as when we visited it descending - // the directory tree. More detailed risk analysis TBD. + // the directory tree. if self.dev != stat.st_dev || self.ino != stat.st_ino { return Err(io::Error::new( io::ErrorKind::Uncategorized, From 3a4fcc2a24a04f454aed33f0795905fc9aea8b1c Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Thu, 19 May 2022 09:10:28 +0200 Subject: [PATCH 7/9] move grandparent check before the deletion --- library/std/src/sys/unix/fs/dir_fd.rs | 46 +++++++++++---------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/library/std/src/sys/unix/fs/dir_fd.rs b/library/std/src/sys/unix/fs/dir_fd.rs index 977a6f541742a..d56604d560c04 100644 --- a/library/std/src/sys/unix/fs/dir_fd.rs +++ b/library/std/src/sys/unix/fs/dir_fd.rs @@ -191,13 +191,6 @@ mod remove_dir_all_xat { } Ok(()) } - - fn is_open(&self) -> bool { - match self { - LazyReadDir::OpenReadDir(_, _) => true, - _ => false, - } - } } impl AsFd for LazyReadDir<'_> { @@ -314,9 +307,27 @@ mod remove_dir_all_xat { let parent_readdir = match readdir_cache.pop_back() { Some(readdir) => readdir, None => { - // cache is empty - reopen parent + // cache is empty - reopen parent and grandparent fd + let parent_readdir = current_readdir.get_parent()?; parent_component.verify_dev_ino(parent_readdir.as_fd())?; + + // We are about to delete the now empty "child directory". + // To make sure the that the child directory was not moved somewhere + // else and that the parent just happens to have the same reused + // (dev, inode) pair, that we found descending, we verify the + // grandparent directory (dev, inode) as well. + let grandparent_readdir = parent_readdir.get_parent()?; + if let Some(grandparent_component) = path_components.last() { + grandparent_component + .verify_dev_ino(grandparent_readdir.as_fd())?; + readdir_cache.push_back(grandparent_readdir); + } else { + // verify parent of the deletion root directory + root_parent_component + .verify_dev_ino(grandparent_readdir.as_fd())?; + } + parent_readdir } }; @@ -332,25 +343,6 @@ mod remove_dir_all_xat { current_path_component = parent_component; current_readdir = parent_readdir; - - // Let "child directory" be the directory that was just deleted and "parent directory" - // the parent of "child directory" which is now referred to in current_*. - // If we don't have readdir open for the parent directory that means we got the file - // descriptor via openat(dirfd, ".."). To make sure the that the child directory - // was not moved somewhere else and the parent just happens to have the same reused - // (dev, inode) pair, that we found descending, we check the parent directory - // (dev, inode) as well. - if !current_readdir.is_open() && readdir_cache.is_empty() { - if let Some(parent_component) = path_components.last() { - let parent_readdir = current_readdir.get_parent()?; - parent_component.verify_dev_ino(parent_readdir.as_fd())?; - readdir_cache.push_back(parent_readdir); - } else { - // verify parent of the deletion root directory - let parent_readdir = current_readdir.get_parent()?; - root_parent_component.verify_dev_ino(parent_readdir.as_fd())?; - } - } } None => break, } From 714aa3c4215284aa040918bb288df9429981e9c9 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Thu, 19 May 2022 09:24:44 +0200 Subject: [PATCH 8/9] fix --- library/std/src/sys/unix/fs/dir_fd.rs | 43 +++++++++++++++------------ 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/library/std/src/sys/unix/fs/dir_fd.rs b/library/std/src/sys/unix/fs/dir_fd.rs index d56604d560c04..5419c6eb0b771 100644 --- a/library/std/src/sys/unix/fs/dir_fd.rs +++ b/library/std/src/sys/unix/fs/dir_fd.rs @@ -191,6 +191,13 @@ mod remove_dir_all_xat { } Ok(()) } + + fn is_open(&self) -> bool { + match self { + LazyReadDir::OpenReadDir(_, _) => true, + _ => false, + } + } } impl AsFd for LazyReadDir<'_> { @@ -307,31 +314,29 @@ mod remove_dir_all_xat { let parent_readdir = match readdir_cache.pop_back() { Some(readdir) => readdir, None => { - // cache is empty - reopen parent and grandparent fd - + // cache is empty - reopen parent let parent_readdir = current_readdir.get_parent()?; parent_component.verify_dev_ino(parent_readdir.as_fd())?; - - // We are about to delete the now empty "child directory". - // To make sure the that the child directory was not moved somewhere - // else and that the parent just happens to have the same reused - // (dev, inode) pair, that we found descending, we verify the - // grandparent directory (dev, inode) as well. - let grandparent_readdir = parent_readdir.get_parent()?; - if let Some(grandparent_component) = path_components.last() { - grandparent_component - .verify_dev_ino(grandparent_readdir.as_fd())?; - readdir_cache.push_back(grandparent_readdir); - } else { - // verify parent of the deletion root directory - root_parent_component - .verify_dev_ino(grandparent_readdir.as_fd())?; - } - parent_readdir } }; + // If `parent_readdir` was now or previously opened via `get_parent()` + // we need to verify the grandparent (dev, inode) pair as well to protect + // against the situation that the child directory was moved somewhere + // else and that the parent just happens to have the same reused + // (dev, inode) pair, that we found descending. + if !parent_readdir.is_open() && readdir_cache.is_empty() { + let grandparent_readdir = parent_readdir.get_parent()?; + if let Some(grandparent_component) = path_components.last() { + grandparent_component.verify_dev_ino(grandparent_readdir.as_fd())?; + readdir_cache.push_back(grandparent_readdir); + } else { + // verify parent of the deletion root directory + root_parent_component.verify_dev_ino(grandparent_readdir.as_fd())?; + } + } + // remove now empty directory cvt(unsafe { unlinkat( From 85a673f2612f0170aa5e156109dbee670c0de0c9 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Wed, 1 Jun 2022 09:08:23 +0200 Subject: [PATCH 9/9] nits: doc comment, const_io_error --- library/std/src/sys/unix/fs/dir_fd.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/unix/fs/dir_fd.rs b/library/std/src/sys/unix/fs/dir_fd.rs index 5419c6eb0b771..e23ec91368af0 100644 --- a/library/std/src/sys/unix/fs/dir_fd.rs +++ b/library/std/src/sys/unix/fs/dir_fd.rs @@ -138,9 +138,9 @@ mod remove_dir_all_xat { /// It is only `Fd(None)` temporarily in `ensure_open()`. Fd(Option), - // Contains the `ReadDir` for the directory while it is being read. The ReadDir does not contain - // a valid `root` path, because it is not needed. It also contains the file descriptor of the - // directory to avoid calls to dirfd(3). + /// Contains the `ReadDir` for the directory while it is being read. The ReadDir does not contain + /// a valid `root` path, because it is not needed. It also contains the file descriptor of the + /// directory to avoid calls to dirfd(3). OpenReadDir(ReadDir, BorrowedFd<'a>), } @@ -247,7 +247,7 @@ mod remove_dir_all_xat { // Make sure that the reopened directory has the same inode as when we visited it descending // the directory tree. if self.dev != stat.st_dev || self.ino != stat.st_ino { - return Err(io::Error::new( + return Err(io::const_io_error!( io::ErrorKind::Uncategorized, "directory with unexpected dev/inode pair", ));