Make read_dir always open ".", rather than using dup. (#151)

`dup` creates a new file descriptor that shares a position with the
old file descriptor. To ensure that independent `read_dir` calls start
from the beginning, always use an open of "." rather than `dup`.

Author: Pat Hickey

cap-primitives/src/posish/fs/read_dir_inner.rs

@@ -1,11 +1,9 @@
 use crate::fs::{
     open_dir_for_reading, open_dir_for_reading_unchecked, open_entry_impl, read_dir_unchecked,
-    remove_dir_unchecked, remove_file_unchecked, stat_unchecked, target_o_path, DirEntryInner,
-    FollowSymlinks, Metadata, OpenOptions, ReadDir,
+    remove_dir_unchecked, remove_file_unchecked, stat_unchecked, DirEntryInner, FollowSymlinks,
+    Metadata, OpenOptions, ReadDir,
 };
 use posish::fs::Dir;
-#[cfg(not(target_os = "wasi"))]
-use posish::io::dup;
 #[cfg(unix)]
 use std::os::unix::{
     ffi::OsStrExt,
@@ -36,22 +34,16 @@ impl ReadDirInner {
     }
 
     pub(crate) fn read_base_dir(start: &fs::File) -> io::Result<Self> {
-        if !target_o_path().is_empty() {
-            // On platforms that use `O_PATH`, open ".".
-            Ok(Self {
-                posish: Arc::new(Dir::from(open_dir_for_reading_unchecked(
-                    start,
-                    Component::CurDir.as_ref(),
-                )?)?),
-            })
-        } else {
-            // On platforms that lack `O_PATH`, we have a full file descriptor,
-            // so just `dup` it (because we use `fdopendir` which assumes
-            // ownership_).
-            Ok(Self {
-                posish: Arc::new(Dir::from(dup(start)?)?),
-            })
-        }
+        // Open ".", to obtain a new independent file descriptor. Don't use
+        // `dup` since in that case the resulting file descriptor would share
+        // a current position with the original, and `read_dir` calls after
+        // the first `read_dir` call wouldn't start from the beginning.
+        Ok(Self {
+            posish: Arc::new(Dir::from(open_dir_for_reading_unchecked(
+                start,
+                Component::CurDir.as_ref(),
+            )?)?),
+        })
     }
 
     pub(crate) fn new_unchecked(start: &fs::File, path: &Path) -> io::Result<Self> {
@@ -92,8 +84,8 @@ impl Iterator for ReadDirInner {
     fn next(&mut self) -> Option<Self::Item> {
         loop {
             let entry = match self.posish.read()? {
-                Err(e) => return Some(Err(e)),
                 Ok(entry) => entry,
+                Err(e) => return Some(Err(e)),
             };
             let file_name = entry.file_name().to_bytes();
             if file_name != Component::CurDir.as_os_str().as_bytes()

tests/readdir.rs

@@ -0,0 +1,84 @@
+use cap_std::fs::{Dir, DirEntry};
+use std::collections::HashMap;
+use std::path::Path;
+
+#[test]
+fn test_dir_entries() {
+    let tmpdir = tempfile::tempdir().expect("construct tempdir");
+
+    let entries = dir_entries(&tmpdir.path());
+    assert_eq!(entries.len(), 0, "empty dir");
+
+    let _f1 = std::fs::File::create(tmpdir.path().join("file1")).expect("create file1");
+
+    let entries = dir_entries(&tmpdir.path());
+    assert!(
+        entries.get("file1").is_some(),
+        "directory contains `file1`: {:?}",
+        entries
+    );
+    assert_eq!(entries.len(), 1);
+
+    let _f2 = std::fs::File::create(tmpdir.path().join("file2")).expect("create file1");
+    let entries = dir_entries(&tmpdir.path());
+    assert!(
+        entries.get("file1").is_some(),
+        "directory contains `file1`: {:?}",
+        entries
+    );
+    assert!(
+        entries.get("file2").is_some(),
+        "directory contains `file2`: {:?}",
+        entries
+    );
+    assert_eq!(entries.len(), 2);
+}
+
+#[test]
+fn test_reread_entries() {
+    let tmpdir = tempfile::tempdir().expect("construct tempdir");
+    let dir = unsafe { Dir::open_ambient_dir(tmpdir.path()).unwrap() };
+
+    let entries = read_entries(&dir);
+    assert_eq!(entries.len(), 0, "empty dir");
+
+    let _f1 = std::fs::File::create(tmpdir.path().join("file1")).expect("create file1");
+
+    let entries = read_entries(&dir);
+    assert!(
+        entries.get("file1").is_some(),
+        "directory contains `file1`: {:?}",
+        entries
+    );
+    assert_eq!(entries.len(), 1);
+
+    let _f2 = std::fs::File::create(tmpdir.path().join("file2")).expect("create file1");
+    let entries = read_entries(&dir);
+    assert!(
+        entries.get("file1").is_some(),
+        "directory contains `file1`: {:?}",
+        entries
+    );
+    assert!(
+        entries.get("file2").is_some(),
+        "directory contains `file2`: {:?}",
+        entries
+    );
+    assert_eq!(entries.len(), 2);
+}
+
+fn dir_entries(path: &Path) -> HashMap<String, DirEntry> {
+    let dir = unsafe { Dir::open_ambient_dir(path).unwrap() };
+    read_entries(&dir)
+}
+
+fn read_entries(dir: &Dir) -> HashMap<String, DirEntry> {
+    let mut out = HashMap::new();
+    for e in dir.entries().unwrap() {
+        let e = e.expect("non-error entry");
+        let name = e.file_name().to_str().expect("utf8 filename").to_owned();
+        assert!(out.get(&name).is_none(), "name already read: {}", name);
+        out.insert(name, e);
+    }
+    out
+}