From ba4dd464f5548aa3180dbe1e8144062221d6463d Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Mon, 12 Dec 2022 17:06:13 -0500 Subject: [PATCH 1/3] fs: Fix #50619 (again) and add a regression test Bug #50619 was fixed by adding an end_of_stream flag in #50630. Unfortunately, that fix only applied to the readdir_r() path. When I switched Linux to use readdir() in #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. --- library/std/src/fs/tests.rs | 26 ++++++++++++++++ library/std/src/sys/unix/fs.rs | 57 ++++++++++++---------------------- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/library/std/src/fs/tests.rs b/library/std/src/fs/tests.rs index 4748ac9d97ef8..6d42adca5314a 100644 --- a/library/std/src/fs/tests.rs +++ b/library/std/src/fs/tests.rs @@ -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 {} +} diff --git a/library/std/src/sys/unix/fs.rs b/library/std/src/sys/unix/fs.rs index fb8d06c66820c..26a99f9137742 100644 --- a/library/std/src/sys/unix/fs.rs +++ b/library/std/src/sys/unix/fs.rs @@ -243,17 +243,15 @@ struct InnerReadDir { pub struct ReadDir { inner: Arc, - #[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 {} @@ -594,6 +592,10 @@ impl Iterator for ReadDir { target_os = "illumos" ))] fn next(&mut self) -> Option> { + if self.end_of_stream { + return None; + } + unsafe { loop { // As of POSIX.1-2017, readdir() is not required to be thread safe; only @@ -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))), @@ -1363,18 +1369,7 @@ pub fn readdir(path: &Path) -> io::Result { } 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)) } } @@ -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}; @@ -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( From 1550a2506d1ad6e3810aa806a6fee59ff4aa6074 Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Wed, 14 Dec 2022 09:54:55 -0500 Subject: [PATCH 2/3] fs/tests: Explicitly kill the zombie rather than sleeping until it dies --- library/std/src/fs/tests.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/library/std/src/fs/tests.rs b/library/std/src/fs/tests.rs index 6d42adca5314a..5c6a16c4bb16c 100644 --- a/library/std/src/fs/tests.rs +++ b/library/std/src/fs/tests.rs @@ -1572,16 +1572,18 @@ fn test_eq_direntry_metadata() { #[test] #[cfg(target_os = "linux")] fn test_read_dir_infinite_loop() { + use crate::io::ErrorKind; 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 }; + // Create a zombie child process + let Ok(mut 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)); + // Make sure the process is (un)dead + match child.kill() { + // InvalidInput means the child already exited + Err(e) if e.kind() != ErrorKind::InvalidInput => return, + _ => {} + } // open() on this path will succeed, but readdir() will fail let id = child.id(); From 9fb7c5ae5e66703240ce4242f55b66a066e0a93c Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Wed, 14 Dec 2022 10:01:42 -0500 Subject: [PATCH 3/3] fs/tests: Fail fast on duplicate errors rather than looping indefinitely --- library/std/src/fs/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/fs/tests.rs b/library/std/src/fs/tests.rs index 5c6a16c4bb16c..b385ebde43979 100644 --- a/library/std/src/fs/tests.rs +++ b/library/std/src/fs/tests.rs @@ -1592,6 +1592,6 @@ fn test_read_dir_infinite_loop() { // 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 {} + // Check for duplicate errors + assert!(dir.filter(|e| e.is_err()).take(2).count() < 2); }