FIX: supporting close needs RwLocks, atomics are racy

Author: cehteh

Cargo.toml

@@ -32,9 +32,9 @@ statx = []
 
 [dependencies]
 libc = "0.2"
+parking_lot = "0.12"
 
 [build-dependencies]
-libc = "0.2"
 conf_test = "0.2"
 
 [dev-dependencies]

src/builder.rs

@@ -140,7 +140,7 @@ impl<'a> DirMethodFlags<'a> {
     /// Creates a new 'Normal' independently owned handle to the underlying directory.
     pub fn clone_upgrade(&self) -> io::Result<Dir> {
         Ok(Dir::new(clone_dirfd_upgrade(
-            self.object.rawfd()?,
+            *self.object.rawfd()?,
             self.flags,
         )?))
     }

src/dir.rs

@@ -5,7 +5,8 @@ use std::mem;
 use std::os::unix::ffi::OsStringExt;
 use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
 use std::path::{Path, PathBuf};
-use std::sync::atomic::{AtomicI16, AtomicI32, AtomicI64, Ordering};
+
+use parking_lot::{RwLock, RwLockReadGuard};
 
 use crate::list::{open_dirfd, DirIter};
 use crate::metadata::{self, Metadata};
@@ -36,18 +37,18 @@ pub const O_SEARCH: libc::c_int = 0;
 ///
 /// Construct it either with ``Dir::cwd()`` or ``Dir::open(path)``
 #[derive(Debug)]
-pub struct Dir(AtomicRawFd);
+pub struct Dir(RwLock<RawFd>);
 
 impl Dir {
     /// low level ctor, only used by builder and tests
     pub(crate) fn new(fd: RawFd) -> Self {
-        Dir(AtomicRawFd::new(fd))
+        Dir(RwLock::new(fd))
     }
 
     /// Gets the underlying RawFd from a Dir handle.
-    pub(crate) fn rawfd(&self) -> io::Result<RawFd> {
-        let rawfd = self.0.load(Ordering::Acquire);
-        if rawfd != -1 {
+    pub(crate) fn rawfd(&self) -> io::Result<RwLockReadGuard<RawFd>> {
+        let rawfd = self.0.read();
+        if *rawfd != -1 {
             Ok(rawfd)
         } else {
             Err(io::Error::from_raw_os_error(libc::EBADF))
@@ -92,7 +93,7 @@ impl Dir {
     /// explicitly. Thats what 'is_dir()' is for. Returns 'true' when the underlying handles
     /// represents a directory and false otherwise.
     pub fn is_dir(&self) -> bool {
-        match fd_type(self.0.load(Ordering::Acquire)).unwrap_or(FdType::Other) {
+        match fd_type(*self.0.read()).unwrap_or(FdType::Other) {
             FdType::NormalDir | FdType::OPathDir => true,
             FdType::Other => false,
         }
@@ -113,7 +114,7 @@ impl Dir {
     /// Create a DirIter from a Dir
     /// Dir must not be a handle opened with O_PATH.
     pub fn list(self) -> io::Result<DirIter> {
-        let fd = self.rawfd()?;
+        let fd = *self.rawfd()?;
         std::mem::forget(self);
         open_dirfd(fd)
     }
@@ -159,7 +160,7 @@ impl Dir {
     pub(crate) fn _sub_dir(&self, path: &CStr, flags: libc::c_int) -> io::Result<Dir> {
         Ok(Dir::new(unsafe {
             libc_ok(libc::openat(
-                self.rawfd()?,
+                *self.rawfd()?,
                 path.as_ptr(),
                 flags | O_DIRECTORY,
             ))?
@@ -175,7 +176,7 @@ impl Dir {
         let mut buf = vec![0u8; 4096];
         let res = unsafe {
             libc::readlinkat(
-                self.rawfd()?,
+                *self.rawfd()?,
                 path.as_ptr(),
                 buf.as_mut_ptr() as *mut libc::c_char,
                 buf.len(),
@@ -371,7 +372,7 @@ impl Dir {
             // promoted as they are in C this would break on Freebsd where
             // *mode_t* is an alias for `uint16_t`.
             let res = libc_ok(libc::openat(
-                self.rawfd()?,
+                *self.rawfd()?,
                 path.as_ptr(),
                 flags | libc::O_CLOEXEC | libc::O_NOFOLLOW,
                 mode as libc::c_uint,
@@ -389,7 +390,7 @@ impl Dir {
 
     fn _symlink(&self, path: &CStr, link: &CStr) -> io::Result<()> {
         unsafe {
-            let res = libc::symlinkat(link.as_ptr(), self.rawfd()?, path.as_ptr());
+            let res = libc::symlinkat(link.as_ptr(), *self.rawfd()?, path.as_ptr());
             if res < 0 {
                 Err(io::Error::last_os_error())
             } else {
@@ -405,7 +406,7 @@ impl Dir {
 
     fn _create_dir(&self, path: &CStr, mode: libc::mode_t) -> io::Result<()> {
         unsafe {
-            libc_ok(libc::mkdirat(self.rawfd()?, path.as_ptr(), mode))?;
+            libc_ok(libc::mkdirat(*self.rawfd()?, path.as_ptr(), mode))?;
         }
         Ok(())
     }
@@ -448,7 +449,7 @@ impl Dir {
 
     fn _unlink(&self, path: &CStr, flags: libc::c_int) -> io::Result<()> {
         unsafe {
-            libc_ok(libc::unlinkat(self.rawfd()?, path.as_ptr(), flags))?;
+            libc_ok(libc::unlinkat(*self.rawfd()?, path.as_ptr(), flags))?;
         }
         Ok(())
     }
@@ -507,7 +508,7 @@ impl Dir {
     #[cfg(feature = "proc_self_fd")]
     pub fn recover_path(&self) -> io::Result<PathBuf> {
         let fd = self.rawfd()?;
-        if fd != libc::AT_FDCWD {
+        if *fd != libc::AT_FDCWD {
             read_link(format!("/proc/self/fd/{}", fd))
         } else {
             read_link("/proc/self/cwd")
@@ -529,7 +530,7 @@ impl Dir {
         unsafe {
             let mut stat = mem::zeroed(); // TODO(cehteh): uninit
             libc_ok(libc::fstatat(
-                self.rawfd()?,
+                *self.rawfd()?,
                 path.as_ptr(),
                 &mut stat,
                 flags,
@@ -542,7 +543,7 @@ impl Dir {
     pub fn self_metadata(&self) -> io::Result<Metadata> {
         unsafe {
             let mut stat = mem::zeroed(); // TODO(cehteh): uninit
-            libc_ok(libc::fstat(self.rawfd()?, &mut stat))?;
+            libc_ok(libc::fstat(*self.rawfd()?, &mut stat))?;
             Ok(metadata::new(stat))
         }
     }
@@ -563,25 +564,26 @@ impl Dir {
     /// Creates a new independently owned handle to the underlying directory.
     /// The new handle has the same (Normal/O_PATH) semantics as the original handle.
     pub fn try_clone(&self) -> io::Result<Self> {
-        Ok(Dir::new(clone_dirfd(self.rawfd()?)?))
+        Ok(Dir::new(clone_dirfd(*self.rawfd()?)?))
     }
 
     /// Creates a new 'Normal' independently owned handle to the underlying directory.
     pub fn clone_upgrade(&self) -> io::Result<Self> {
-        Ok(Dir::new(clone_dirfd_upgrade(self.rawfd()?, 0)?))
+        Ok(Dir::new(clone_dirfd_upgrade(*self.rawfd()?, 0)?))
     }
 
     /// Creates a new 'O_PATH' restricted independently owned handle to the underlying directory.
     pub fn clone_downgrade(&self) -> io::Result<Self> {
-        Ok(Dir::new(clone_dirfd_downgrade(self.rawfd()?)?))
+        Ok(Dir::new(clone_dirfd_downgrade(*self.rawfd()?)?))
     }
 
     /// Explicit closing of a file handle. The object stays alive but all operations on it will fail.
     pub fn close(&self) {
-        let fd = self.0.swap(-1, Ordering::AcqRel);
-        if fd != libc::AT_FDCWD && fd != -1 {
+        let mut fd = self.0.write();
+        if *fd != libc::AT_FDCWD && *fd != -1 {
             unsafe {
-                libc::close(fd);
+                libc::close(*fd);
+                *fd = -1;
             }
         }
     }
@@ -726,9 +728,9 @@ where
 fn _rename(old_dir: &Dir, old: &CStr, new_dir: &Dir, new: &CStr) -> io::Result<()> {
     unsafe {
         libc_ok(libc::renameat(
-            old_dir.rawfd()?,
+            *old_dir.rawfd()?,
             old.as_ptr(),
-            new_dir.rawfd()?,
+            *new_dir.rawfd()?,
             new.as_ptr(),
         ))?;
     }
@@ -766,9 +768,9 @@ fn _hardlink(
 ) -> io::Result<()> {
     unsafe {
         libc_ok(libc::linkat(
-            old_dir.rawfd()?,
+            *old_dir.rawfd()?,
             old.as_ptr(),
-            new_dir.rawfd()?,
+            *new_dir.rawfd()?,
             new.as_ptr(),
             flags,
         ))?;
@@ -831,7 +833,7 @@ fn _rename_flags(
 impl AsRawFd for Dir {
     #[inline]
     fn as_raw_fd(&self) -> RawFd {
-        self.0.load(Ordering::Acquire)
+        *self.0.read()
     }
 }
 
@@ -847,7 +849,7 @@ impl FromRawFd for Dir {
 impl IntoRawFd for Dir {
     #[inline]
     fn into_raw_fd(self) -> RawFd {
-        let result = self.0.load(Ordering::Acquire);
+        let result = *self.0.read();
         mem::forget(self);
         result
     }
@@ -864,26 +866,6 @@ pub(crate) fn to_cstr<P: AsPath>(path: P) -> io::Result<P::Buffer> {
         .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "nul byte in file name"))
 }
 
-// For the esoteric platforms where c_int is not i32,
-// make the RawFd/c_int atomic in a portable way
-trait Atomic {
-    type T;
-}
-
-impl Atomic for i16 {
-    type T = AtomicI16;
-}
-
-impl Atomic for i32 {
-    type T = AtomicI32;
-}
-
-impl Atomic for i64 {
-    type T = AtomicI64;
-}
-
-type AtomicRawFd = <RawFd as Atomic>::T;
-
 #[cfg(test)]
 mod test {
     use std::io::Read;

src/list.rs

@@ -1,10 +1,12 @@
 use std::ffi::{CStr, CString, OsStr};
 use std::io;
+use std::fmt;
 use std::mem;
 use std::os::unix::ffi::OsStrExt;
-use std::sync::atomic::{AtomicPtr, Ordering};
 use std::sync::Arc;
 
+use parking_lot::{RwLock, RwLockReadGuard};
+
 use crate::{dir::libc_ok, metadata, Metadata, SimpleType};
 
 // We have such weird constants because C types are ugly
@@ -14,7 +16,6 @@ const DOTDOT: [libc::c_char; 3] = [b'.' as libc::c_char, b'.' as libc::c_char, 0
 /// Iterator over directory entries
 ///
 /// Created using `Dir::list_dir()`
-#[derive(Debug)]
 pub struct DirIter {
     // Needs Arc here to be shared with Entries, for metdata()
     dir: Arc<DirHandle>,
@@ -40,7 +41,6 @@ pub struct DirPosition {
 }
 
 /// Entry returned by iterating over `DirIter` iterator
-#[derive(Debug)]
 pub struct Entry {
     dir:             Arc<DirHandle>,
     pub(crate) name: CString,
@@ -69,7 +69,7 @@ impl Entry {
         unsafe {
             let mut stat = mem::zeroed(); // TODO(cehteh): uninit
             libc_ok(libc::fstatat(
-                libc::dirfd(self.dir.raw()?),
+                libc::dirfd(*self.dir.raw()?),
                 self.name.as_ptr(),
                 &mut stat,
                 libc::AT_SYMLINK_NOFOLLOW,
@@ -84,6 +84,17 @@ impl Entry {
     }
 }
 
+impl fmt::Debug for Entry {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("Entry")
+            .field("dir", &Arc::as_ptr(&self.dir))
+            .field("name", &self.name)
+            .field("file_type", &self.file_type)
+            .field("ino", &self.ino)
+            .finish()
+    }
+}
+
 #[cfg(any(target_os = "linux", target_os = "fuchsia"))]
 unsafe fn errno_location() -> *mut libc::c_int {
     libc::__errno_location()
@@ -110,7 +121,7 @@ impl DirIter {
         // Reset errno to detect if error occurred
         *errno_location() = 0;
 
-        let entry = libc::readdir(self.dir.raw()?);
+        let entry = libc::readdir(*self.dir.raw()?);
         if entry.is_null() {
             if *errno_location() == 0 {
                 return Ok(None);
@@ -123,7 +134,7 @@ impl DirIter {
 
     /// Returns the current directory iterator position. The result should be handled as opaque value
     pub fn current_position(&self) -> io::Result<DirPosition> {
-        let pos = unsafe { libc::telldir(self.dir.raw()?) };
+        let pos = unsafe { libc::telldir(*self.dir.raw()?) };
 
         if pos == -1 {
             Err(io::Error::last_os_error())
@@ -136,14 +147,14 @@ impl DirIter {
     /// Sets the current directory iterator position to some location queried by 'current_position()'
     pub fn seek(&self, position: DirPosition) {
         if let Ok(dir) = self.dir.raw() {
-            unsafe { libc::seekdir(dir, position.pos) };
+            unsafe { libc::seekdir(*dir, position.pos) };
         }
     }
 
     /// Resets the current directory iterator position to the beginning
     pub fn rewind(&self) {
         if let Ok(dir) = self.dir.raw() {
-            unsafe { libc::rewinddir(dir) };
+            unsafe { libc::rewinddir(*dir) };
         }
     }
 
@@ -196,16 +207,16 @@ impl Iterator for DirIter {
     }
 }
 
-#[derive(Debug)]
-struct DirHandle(AtomicPtr<libc::DIR>);
+//#[derive(Debug)]
+struct DirHandle(RwLock<*mut libc::DIR>);
 
 impl DirHandle {
     fn new(dir: *mut libc::DIR) -> Self {
-        DirHandle(AtomicPtr::new(dir))
+        DirHandle(RwLock::new(dir))
     }
 
-    fn raw(&self) -> io::Result<*mut libc::DIR> {
-        let dir = self.0.load(Ordering::Acquire);
+    fn raw(&self) -> io::Result<RwLockReadGuard<*mut libc::DIR>> {
+        let dir = self.0.read();
         if !dir.is_null() {
             Ok(dir)
         } else {
@@ -214,10 +225,11 @@ impl DirHandle {
     }
 
     fn close(&self) {
-        let dir = self.0.swap(std::ptr::null_mut(), Ordering::AcqRel);
+        let mut dir = self.0.write();
         if !dir.is_null() {
             unsafe {
-                libc::closedir(dir);
+                libc::closedir(*dir);
+                *dir = std::ptr::null_mut();
             }
         }
     }