fix two problems in the Dir module: (#2674)

readdir_r does not work well on systems where {NAME_MAX} can vary.  The main
reason to use it is for thread-safety.  However, POSIX 1003.1-2024 Issue 8
makes readdir_r obsolete and clarifies that readdir must be thread-safe except
when concurrent calls are made to the same directory stream.  So all
applications should prefer it now.  The original rationale for Nix using
readdir_r[^1], in addition to thread safety, was that the Rust standard library
used it.  But that is no longer the case.  So Nix should switch to plain
readdir.

The second problem is that on some operating systems, libc::dirent is an
open-ended structure where the last field (d_name) has a size of 1 byte,
but additional data follow the end of the structure.  Nix currently
relies on copying the libc::dirent structure, which can't possibly work
on those platforms.  Fix that bug by copying the d_name field to an
owned CString.

[^1]: f9ebcb7 , from 2018

Fixes #2673
Fixes #1783

Author: asomers

changelog/2674.fixed.md

@@ -0,0 +1,3 @@
+Fixed the Dir module on NTO, Solaris, Hurd, and possibly other platforms.  The
+d_name field was not copied correctly on those platforms.  For some other
+platforms, it could be copied incorrectly for files with very long pathnames.

src/dir.rs

@@ -5,16 +5,10 @@ use crate::fcntl::{self, OFlag};
 use crate::sys;
 use crate::{NixPath, Result};
 use cfg_if::cfg_if;
-use std::ffi;
+use std::ffi::{CStr, CString};
 use std::os::unix::io::{AsRawFd, IntoRawFd, RawFd};
 use std::ptr;
 
-#[cfg(target_os = "linux")]
-use libc::{dirent64 as dirent, readdir64_r as readdir_r};
-
-#[cfg(not(target_os = "linux"))]
-use libc::{dirent, readdir_r};
-
 /// An open directory.
 ///
 /// This is a lower-level interface than [`std::fs::ReadDir`]. Notable differences:
@@ -26,7 +20,7 @@ use libc::{dirent, readdir_r};
 ///    * can be iterated through multiple times without closing and reopening the file
 ///      descriptor. Each iteration rewinds when finished.
 ///    * returns entries for `.` (current directory) and `..` (parent directory).
-///    * returns entries' names as a [`CStr`][cstr] (no allocation or conversion beyond whatever libc
+///    * returns entries' names as a [`CStr`] (no allocation or conversion beyond whatever libc
 ///      does).
 ///
 /// [AsFd]: std::os::fd::AsFd
@@ -124,10 +118,7 @@ impl Dir {
     }
 }
 
-// `Dir` is not `Sync`. With the current implementation, it could be, but according to
-// https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html,
-// future versions of POSIX are likely to obsolete `readdir_r` and specify that it's unsafe to
-// call `readdir` simultaneously from multiple threads.
+// `Dir` is not `Sync` because it's unsafe to call `readdir` simultaneously from multiple threads.
 //
 // `Dir` is safe to pass from one thread to another, as it's not reference-counted.
 unsafe impl Send for Dir {}
@@ -160,29 +151,22 @@ impl Drop for Dir {
 }
 
 // The pass by mut is technically needless only because the inner NonNull is
-// Copy.  But philosophically we're mutating the Dir, so we pass by mut.
+// Copy.  But we are actually mutating the Dir, so we pass by mut.
 #[allow(clippy::needless_pass_by_ref_mut)]
-fn next(dir: &mut Dir) -> Option<Result<Entry>> {
+fn readdir(dir: &mut Dir) -> Option<Result<Entry>> {
+    Errno::clear();
     unsafe {
-        // Note: POSIX specifies that portable applications should dynamically allocate a
-        // buffer with room for a `d_name` field of size `pathconf(..., _PC_NAME_MAX)` plus 1
-        // for the NUL byte. It doesn't look like the std library does this; it just uses
-        // fixed-sized buffers (and libc's dirent seems to be sized so this is appropriate).
-        // Probably fine here too then.
-        let mut ent = std::mem::MaybeUninit::<dirent>::uninit();
-        let mut result = ptr::null_mut();
-        if let Err(e) = Errno::result(readdir_r(
-            dir.0.as_ptr(),
-            ent.as_mut_ptr(),
-            &mut result,
-        )) {
-            return Some(Err(e));
-        }
-        if result.is_null() {
-            return None;
+        let de = libc::readdir(dir.0.as_ptr());
+        if de.is_null() {
+            if Errno::last_raw() == 0 {
+                // EOF
+                None
+            } else {
+                Some(Err(Errno::last()))
+            }
+        } else {
+            Some(Ok(Entry::from_raw(&*de)))
         }
-        assert_eq!(result, ent.as_mut_ptr());
-        Some(Ok(Entry(ent.assume_init())))
     }
 }
 
@@ -194,7 +178,7 @@ impl Iterator for Iter<'_> {
     type Item = Result<Entry>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        next(self.0)
+        readdir(self.0)
     }
 }
 
@@ -212,7 +196,7 @@ impl Iterator for OwningIter {
     type Item = Result<Entry>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        next(&mut self.0)
+        readdir(&mut self.0)
     }
 }
 
@@ -252,9 +236,18 @@ impl IntoIterator for Dir {
 /// A directory entry, similar to `std::fs::DirEntry`.
 ///
 /// Note that unlike the std version, this may represent the `.` or `..` entries.
-#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)]
-#[repr(transparent)]
-pub struct Entry(dirent);
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
+pub struct Entry {
+    ino: u64,
+    type_: Option<Type>,
+    // On some platforms libc::dirent is a "flexible-length structure", where there may be
+    // data beyond the end of the struct definition.  So it isn't possible to copy it and subsequently
+    // dereference the copy's d_name field.  Nor is it possible for Entry to wrap a &libc::dirent.
+    // At least, not if it is to work with the Iterator trait.  Because the Entry would need to
+    // maintain a mutable reference to the Dir, which would conflict with the iterator's mutable
+    // reference to the same Dir.  So we're forced to copy the name here.
+    name: CString,
+}
 
 /// Type of file referenced by a directory entry
 #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)]
@@ -277,10 +270,28 @@ pub enum Type {
 
 impl Entry {
     /// Returns the inode number (`d_ino`) of the underlying `dirent`.
+    pub fn ino(&self) -> u64 {
+        self.ino
+    }
+
+    /// Returns the bare file name of this directory entry without any other leading path component.
+    pub fn file_name(&self) -> &CStr {
+        self.name.as_c_str()
+    }
+
+    /// Returns the type of this directory entry, if known.
+    ///
+    /// See platform `readdir(3)` or `dirent(5)` manpage for when the file type is known;
+    /// notably, some Linux filesystems don't implement this. The caller should use `stat` or
+    /// `fstat` if this returns `None`.
+    pub fn file_type(&self) -> Option<Type> {
+        self.type_
+    }
+
     #[allow(clippy::useless_conversion)] // Not useless on all OSes
     // The cast is not unnecessary on all platforms.
     #[allow(clippy::unnecessary_cast)]
-    pub fn ino(&self) -> u64 {
+    fn from_raw(de: &libc::dirent) -> Self {
         cfg_if! {
             if #[cfg(any(target_os = "aix",
                          target_os = "emscripten",
@@ -291,38 +302,34 @@ impl Entry {
                          solarish,
                          linux_android,
                          apple_targets))] {
-                self.0.d_ino as u64
+                let ino = de.d_ino as u64;
             } else {
-                u64::from(self.0.d_fileno)
+                let ino = u64::from(de.d_fileno);
             }
         }
-    }
-
-    /// Returns the bare file name of this directory entry without any other leading path component.
-    pub fn file_name(&self) -> &ffi::CStr {
-        unsafe { ffi::CStr::from_ptr(self.0.d_name.as_ptr()) }
-    }
-
-    /// Returns the type of this directory entry, if known.
-    ///
-    /// See platform `readdir(3)` or `dirent(5)` manpage for when the file type is known;
-    /// notably, some Linux filesystems don't implement this. The caller should use `stat` or
-    /// `fstat` if this returns `None`.
-    pub fn file_type(&self) -> Option<Type> {
-        #[cfg(not(any(solarish, target_os = "aix", target_os = "haiku")))]
-        match self.0.d_type {
-            libc::DT_FIFO => Some(Type::Fifo),
-            libc::DT_CHR => Some(Type::CharacterDevice),
-            libc::DT_DIR => Some(Type::Directory),
-            libc::DT_BLK => Some(Type::BlockDevice),
-            libc::DT_REG => Some(Type::File),
-            libc::DT_LNK => Some(Type::Symlink),
-            libc::DT_SOCK => Some(Type::Socket),
-            /* libc::DT_UNKNOWN | */ _ => None,
+        cfg_if! {
+            if #[cfg(not(any(solarish, target_os = "aix", target_os = "haiku")))] {
+                let type_ = match de.d_type {
+                    libc::DT_FIFO => Some(Type::Fifo),
+                    libc::DT_CHR => Some(Type::CharacterDevice),
+                    libc::DT_DIR => Some(Type::Directory),
+                    libc::DT_BLK => Some(Type::BlockDevice),
+                    libc::DT_REG => Some(Type::File),
+                    libc::DT_LNK => Some(Type::Symlink),
+                    libc::DT_SOCK => Some(Type::Socket),
+                    /* libc::DT_UNKNOWN | */ _ => None,
+                };
+            } else {
+                // illumos, Solaris, and Haiku systems do not have the d_type member at all:
+                #[cfg(any(solarish, target_os = "aix", target_os = "haiku"))]
+                let type_ = None;
+            }
         }
+        // Annoyingly, since libc::dirent is open-ended, the easiest way to read the name field in
+        // Rust is to treat it like a pointer.
+        // Safety: safe because we knod that de.d_name is in valid memory.
+        let name = unsafe { CStr::from_ptr(de.d_name.as_ptr()) }.to_owned();
 
-        // illumos, Solaris, and Haiku systems do not have the d_type member at all:
-        #[cfg(any(solarish, target_os = "aix", target_os = "haiku"))]
-        None
+        Entry { ino, type_, name }
     }
 }