Skip to content

Commit

Permalink
fs: Fix rust-lang#50619 (again) and add a regression test
Browse files Browse the repository at this point in the history
Bug rust-lang#50619 was fixed by adding an end_of_stream flag in rust-lang#50630.
Unfortunately, that fix only applied to the readdir_r() path.  When I
switched Linux to use readdir() in rust-lang#92778, I inadvertently reintroduced
the bug on that platform.  Other platforms that had always used
readdir() were presumably never fixed.

This patch enables end_of_stream for all platforms, and adds a
Linux-specific regression test that should hopefully prevent the bug
from being reintroduced again.
  • Loading branch information
tavianator committed Dec 12, 2022
1 parent 37d7de3 commit ba4dd46
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 38 deletions.
26 changes: 26 additions & 0 deletions library/std/src/fs/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1567,3 +1567,29 @@ fn test_eq_direntry_metadata() {
assert_eq!(ft1, ft2);
}
}

/// Regression test for https://github.com/rust-lang/rust/issues/50619.
#[test]
#[cfg(target_os = "linux")]
fn test_read_dir_infinite_loop() {
use crate::process::Command;
use crate::thread::sleep;
use crate::time::Duration;

// Create a child process
let Ok(child) = Command::new("echo").spawn() else { return };

// Wait for it to (probably) become a zombie. We can't use wait() because
// that will reap the process.
sleep(Duration::from_millis(10));

// open() on this path will succeed, but readdir() will fail
let id = child.id();
let path = format!("/proc/{id}/net");

// Skip the test if we can't open the directory in the first place
let Ok(dir) = fs::read_dir(path) else { return };

// Iterate through the directory
for _ in dir {}
}
57 changes: 19 additions & 38 deletions library/std/src/sys/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,17 +243,15 @@ struct InnerReadDir {

pub struct ReadDir {
inner: Arc<InnerReadDir>,
#[cfg(not(any(
target_os = "android",
target_os = "linux",
target_os = "solaris",
target_os = "illumos",
target_os = "fuchsia",
target_os = "redox",
)))]
end_of_stream: bool,
}

impl ReadDir {
fn new(inner: InnerReadDir) -> Self {
Self { inner: Arc::new(inner), end_of_stream: false }
}
}

struct Dir(*mut libc::DIR);

unsafe impl Send for Dir {}
Expand Down Expand Up @@ -594,6 +592,10 @@ impl Iterator for ReadDir {
target_os = "illumos"
))]
fn next(&mut self) -> Option<io::Result<DirEntry>> {
if self.end_of_stream {
return None;
}

unsafe {
loop {
// As of POSIX.1-2017, readdir() is not required to be thread safe; only
Expand All @@ -604,8 +606,12 @@ impl Iterator for ReadDir {
super::os::set_errno(0);
let entry_ptr = readdir64(self.inner.dirp.0);
if entry_ptr.is_null() {
// null can mean either the end is reached or an error occurred.
// So we had to clear errno beforehand to check for an error now.
// We either encountered an error, or reached the end. Either way,
// the next call to next() should return None.
self.end_of_stream = true;

// To distinguish between errors and end-of-directory, we had to clear
// errno beforehand to check for an error now.
return match super::os::errno() {
0 => None,
e => Some(Err(Error::from_raw_os_error(e))),
Expand Down Expand Up @@ -1363,18 +1369,7 @@ pub fn readdir(path: &Path) -> io::Result<ReadDir> {
} else {
let root = path.to_path_buf();
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,
})
Ok(ReadDir::new(inner))
}
}

Expand Down Expand Up @@ -1755,7 +1750,6 @@ mod remove_dir_impl {
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::common::small_c_string::run_path_with_cstr;
use crate::sys::{cvt, cvt_r};

Expand Down Expand Up @@ -1827,21 +1821,8 @@ mod remove_dir_impl {
// 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,
))
let inner = InnerReadDir { dirp, root: dummy_root };
Ok((ReadDir::new(inner), new_parent_fd))
}

#[cfg(any(
Expand Down

0 comments on commit ba4dd46

Please sign in to comment.