From a8a19c060f7086da1e5531ebf3b3fd015ef2bc5f Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Wed, 25 Oct 2023 23:03:39 +1100 Subject: [PATCH 1/3] Simplify unix implementation - Remove second param of `set_nonblocking` since it is always set to `true` - Remove `is_pipe_without_access_mode_check`, change `is_pipe` to only take one parameter and add fn `get_access_mode` for checking access mode separately. - Use `ManuallyDrop::new(File::from_raw_fd(..))` to construct a `File` with no `Drop` so that we can call its `metadata()` and its `try_clone()` fn without having to implement them ourselves using `unsafe` code in `dup*`. Signed-off-by: Jiahao XU --- src/unix.rs | 135 ++++++++++++++++++---------------------------------- 1 file changed, 45 insertions(+), 90 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index f7ac7fd..8530a48 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -5,16 +5,13 @@ use std::{ fmt::Write as _, fs::{self, File}, io::{self, Read, Write}, - mem::MaybeUninit, + mem::{ManuallyDrop, MaybeUninit}, os::unix::{ ffi::{OsStrExt, OsStringExt}, prelude::*, }, path::{Path, PathBuf}, - sync::{ - atomic::{AtomicBool, Ordering}, - Arc, - }, + sync::Arc, thread::{Builder, JoinHandle}, }; @@ -23,16 +20,6 @@ use libc::c_int; use crate::Command; -/// Lowest file descriptor used in `Selector::try_clone`. -/// -/// # Notes -/// -/// Usually fds 0, 1 and 2 are standard in, out and error. Some application -/// blindly assume this to be true, which means using any one of those a select -/// could result in some interesting and unexpected errors. Avoid that by using -/// an fd that doesn't have a pre-determined usage. -const LOWEST_FD: libc::c_int = 3; - #[derive(Debug)] pub struct Client { /// This fd is set to be nonblocking @@ -95,7 +82,7 @@ impl Client { // File in Rust is always closed-on-exec as long as it's opened by // `File::open` or `fs::OpenOptions::open`. - set_nonblocking(file.as_raw_fd(), true)?; + set_nonblocking(file.as_raw_fd())?; let client = Self { read: file.try_clone()?, @@ -165,10 +152,10 @@ impl Client { fn from_fifo(path: &Path) -> Option { let file = open_file_rw(path).ok()?; - if is_pipe_without_access_mode_check(file.as_raw_fd()) { + if is_pipe(&file)? { // File in Rust is always closed-on-exec as long as it's opened by // `File::open` or `fs::OpenOptions::open`. - set_nonblocking(file.as_raw_fd(), true).ok()?; + set_nonblocking(file.as_raw_fd()).ok()?; Some(Self { read: file.try_clone().ok()?, @@ -188,6 +175,9 @@ impl Client { let read = read.parse().ok()?; let write = write.parse().ok()?; + let read = ManuallyDrop::new(File::from_raw_fd(read)); + let write = ManuallyDrop::new(File::from_raw_fd(write)); + // Ok so we've got two integers that look like file descriptors, but // for extra sanity checking let's see if they actually look like // instances of a pipe before we return the client. @@ -195,17 +185,33 @@ impl Client { // If we're called from `make` *without* the leading + on our rule // then we'll have `MAKEFLAGS` env vars but won't actually have // access to the file descriptors. - if is_pipe(read, true) && is_pipe(write, false) { - let read = dup_with_cloexec(read).ok()?; - let write = dup_with_cloexec(write).ok()?; - - // Set read and write end to nonblocking - set_nonblocking(read, true).ok()?; - set_nonblocking(write, true).ok()?; - - Some(Self::from_fds(read, write)) - } else { - None + match ( + is_pipe(&read), + is_pipe(&write), + get_access_mode(&read), + get_access_mode(&write), + ) { + ( + Some(true), + Some(true), + Some(libc::O_RDONLY) | Some(libc::O_RDWR), + Some(libc::O_WRONLY) | Some(libc::O_RDWR), + ) => { + let read = read.try_clone().ok()?; + let write = write.try_clone().ok()?; + + // Set read and write end to nonblocking + set_nonblocking(read.as_raw_fd()).ok()?; + set_nonblocking(write.as_raw_fd()).ok()?; + + Some(Self { + read, + write, + path: None, + owns_fifo: false, + }) + } + _ => None, } } @@ -425,8 +431,8 @@ fn create_pipe(nonblocking: bool) -> io::Result<[RawFd; 2]> { set_cloexec(pipes[1], true)?; if nonblocking { - set_nonblocking(pipes[0], true)?; - set_nonblocking(pipes[1], true)?; + set_nonblocking(pipes[0])?; + set_nonblocking(pipes[1])?; } Ok(pipes) @@ -439,47 +445,17 @@ fn set_cloexec(fd: c_int, set: bool) -> io::Result<()> { Ok(()) } -fn set_nonblocking(fd: c_int, set: bool) -> io::Result<()> { - let status_flag = if set { libc::O_NONBLOCK } else { 0 }; - +fn set_nonblocking(fd: c_int) -> io::Result<()> { // F_SETFL can only set the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and // O_NONBLOCK flags. // // For pipe, only O_NONBLOCK is meaningful, so it is ok to // not issue a F_GETFL fcntl syscall. - cvt(unsafe { libc::fcntl(fd, libc::F_SETFL, status_flag) })?; + cvt(unsafe { libc::fcntl(fd, libc::F_SETFL, libc::O_NONBLOCK) })?; Ok(()) } -fn dup(fd: c_int) -> io::Result { - // EINTR - cvt_retry_on_interrupt(move || unsafe { libc::dup(fd) }) -} - -fn dup_with_cloexec(fd: RawFd) -> io::Result { - static F_DUPFD_CLOEXEC_AVAILBILITY: AtomicBool = AtomicBool::new(true); - - if F_DUPFD_CLOEXEC_AVAILBILITY.load(Ordering::Relaxed) { - match cvt(unsafe { libc::fcntl(fd, libc::F_DUPFD_CLOEXEC, LOWEST_FD) }) { - Err(err) - if err.raw_os_error() == Some(libc::ENOSYS) - // If the flag F_DUPFD_CLOEXEC is invalid, then it might - // return EINVAL. - || err.raw_os_error() == Some(libc::EINVAL) => - { - F_DUPFD_CLOEXEC_AVAILBILITY.store(false, Ordering::Relaxed) - } - res => return res, - } - } - - // Fallback to dup + set_cloexec - let new_fd = dup(fd)?; - set_cloexec(new_fd, true)?; - Ok(new_fd) -} - fn cvt(t: c_int) -> io::Result { if t == -1 { Err(io::Error::last_os_error()) @@ -497,38 +473,17 @@ fn cvt_retry_on_interrupt(f: impl Fn() -> c_int) -> io::Result { } } -fn is_pipe_without_access_mode_check(fd: RawFd) -> bool { - let mut stat = MaybeUninit::::uninit(); - - if unsafe { libc::fstat(fd, stat.as_mut_ptr()) } == -1 { - return false; - } - - // Safety: - // - // libc::fstat succeeds, stat is initialized - let stat = unsafe { stat.assume_init() }; - (stat.st_mode & libc::S_IFMT) == libc::S_IFIFO +fn is_pipe(file: &File) -> Option { + Some(file.metadata().ok()?.file_type().is_fifo()) } -fn is_pipe(fd: RawFd, readable: bool) -> bool { - if !is_pipe_without_access_mode_check(fd) { - return false; - } - - let ret = unsafe { libc::fcntl(fd, libc::F_GETFL) }; +fn get_access_mode(file: &File) -> Option { + let ret = unsafe { libc::fcntl(file.as_raw_fd(), libc::F_GETFL) }; if ret == -1 { - return false; + return None; } - let status_flags = ret; - let access_mode = if readable { - libc::O_RDONLY - } else { - libc::O_WRONLY - }; - - (status_flags & libc::O_ACCMODE) == access_mode + Some(ret & libc::O_ACCMODE) } /// NOTE that this is a blocking syscall, it will block From 906867793255c6e9d054a56d03ce8a393da2d6ee Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Wed, 25 Oct 2023 23:08:18 +1100 Subject: [PATCH 2/3] Fix linux impl of `create_pipe` Signed-off-by: Jiahao XU --- src/unix.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 8530a48..f557a10 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -412,15 +412,17 @@ fn create_pipe(nonblocking: bool) -> io::Result<[RawFd; 2]> { // with as many kernels/glibc implementations as possible. #[cfg(target_os = "linux")] { + use std::sync::{AtomicBool, Ordering::Relaxed}; + static PIPE2_AVAILABLE: AtomicBool = AtomicBool::new(true); - if PIPE2_AVAILABLE.load(Ordering::Relaxed) { + if PIPE2_AVAILABLE.load(Relaxed) { let flags = libc::O_CLOEXEC | if nonblocking { libc::O_NONBLOCK } else { 0 }; match cvt(unsafe { libc::pipe2(pipes.as_mut_ptr(), flags) }) { Ok(_) => return Ok(pipes), Err(err) if err.raw_os_error() != Some(libc::ENOSYS) => return Err(err), // err.raw_os_error() == Some(libc::ENOSYS) - _ => PIPE2_AVAILABLE.store(false, Ordering::Relaxed), + _ => PIPE2_AVAILABLE.store(false, Relaxed), } } } From b806d55c643449dd632e425f011149482c780d77 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Wed, 25 Oct 2023 23:09:23 +1100 Subject: [PATCH 3/3] Fix import of atomics Signed-off-by: Jiahao XU --- src/unix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/unix.rs b/src/unix.rs index f557a10..d99afd2 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -412,7 +412,7 @@ fn create_pipe(nonblocking: bool) -> io::Result<[RawFd; 2]> { // with as many kernels/glibc implementations as possible. #[cfg(target_os = "linux")] { - use std::sync::{AtomicBool, Ordering::Relaxed}; + use std::sync::atomic::{AtomicBool, Ordering::Relaxed}; static PIPE2_AVAILABLE: AtomicBool = AtomicBool::new(true); if PIPE2_AVAILABLE.load(Relaxed) {