From 76fe936970a562aad5745ba795beec828790e2f9 Mon Sep 17 00:00:00 2001 From: Avril Date: Thu, 26 Aug 2021 20:15:42 +0100 Subject: [PATCH] Moved raw fd holding into a seperate inner structure to handle fd closing and ownership. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed returning `(rx, tx)` instead of `(tx, rx)` from `unix_pipe()`. Fortune for comfork's current commit: Small curse − 小凶 --- src/sys/pipe.rs | 174 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 115 insertions(+), 59 deletions(-) diff --git a/src/sys/pipe.rs b/src/sys/pipe.rs index 5ed27f6..6bd40b5 100644 --- a/src/sys/pipe.rs +++ b/src/sys/pipe.rs @@ -18,36 +18,110 @@ pub enum ErrorKind Unknown, } -/// A raw unix pipe -// TODO: Change uses of `i32` for socket fds. Or change this to a struct (see below.) -pub type RawPipe = i32; +/// A raw unix pipe. +/// +/// Encapsulates a raw fd, proventing arbitrary operations on it while it is owned by the instance. +/// The fd is closed when this instance is dropped. +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[repr(transparent)] +pub struct RawPipe(i32); + +impl io::Read for RawPipe +{ + #[inline] fn read(&mut self, buf: &mut [u8]) -> io::Result { + Ok(read_raw(self.0, buf)?) + } +} +impl io::Write for RawPipe +{ + #[inline] fn write(&mut self, buf: &[u8]) -> io::Result { + Ok(write_raw(self.0, buf)?) + } + #[inline] fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +impl From for i32 +{ + #[inline] fn from(from: RawPipe) -> Self + { + from.take() + } +} + +impl RawPipe +{ + /// Transfer ownership of the internal fd to the caller + #[inline] pub fn take(self) -> i32 + { + let i = self.0; + std::mem::forget(self); + i + } + + /// Transfer ownership of an fd into a new managed instance. + /// + /// # Safety + /// The caller should ensure the fd is valid, or all operations on the instance returned will fail. + #[inline] pub const unsafe fn new_unchecked(int: i32) -> Self + { + Self(int) + } + + /// Get a raw handle to the fd + /// + /// # Safety + /// This operation is unsafe as the caller could close the fd while its still owned by the instance. + /// If you want to transfer ownership of the fd to yourself, use `take()` or `Into::into()`. + #[inline] pub const unsafe fn as_raw_fd(&self) -> i32 + { + self.0 + } + + /// Transfer ownership of an fd into a new managed instance. + /// + /// # Panics + /// If the fd value was below 0. + pub fn new(int: i32) -> Self + { + assert!(int > 0, "invalid fd"); + Self(int) + } +} -/// Create a raw pipe pair. -pub(crate) fn unix_pipe() -> Result<(i32, i32), Error> +impl Drop for RawPipe +{ + fn drop(&mut self) { + unsafe { + libc::close(self.0); + } + } +} + +/// Create a raw pipe pair. Returns `(tx, rx)`: writer then reader. +pub(crate) fn unix_pipe() -> Result<(RawPipe, RawPipe), Error> { use libc::pipe; let mut pipe_fds: [libc::c_int; 2] = [-1;2]; if unsafe{pipe(pipe_fds.as_mut_ptr())} == 0 { - Ok((i32::from(pipe_fds[0]), i32::from(pipe_fds[1]))) + Ok((RawPipe(i32::from(pipe_fds[1])), RawPipe(i32::from(pipe_fds[0])))) } else { Err(ErrorKind::Create.into_error()) } } -//TODO: Factor out the `i32` raw sockets in these types below into a seperate `RawPipe` struct, which has the Drop impl and has functions for transfering ownership of the raw socket (it should hide the raw fd when not explicitly needed ideally.) -// This will prevent us from having to ensure we `forget()` the outer type each time we want to move the inner one around without closing it. - /// A writer for a bi-directinoal unix pipe /// /// Created by splitting `Pipe`, data written to this is available to be read from its `ReadHalf` counterpart. #[derive(Debug, PartialEq, Eq)] -pub struct WriteHalf(i32); +pub struct WriteHalf(RawPipe); /// A reader for a bi-directional unix pipe. /// /// Created by splitting `Pipe`, data read from this is data that was sent to its `WriteHalf` counterpart. #[derive(Debug, PartialEq, Eq)] -pub struct ReadHalf(i32); +pub struct ReadHalf(RawPipe); /// A bi-drectional unix pipe /// @@ -55,11 +129,10 @@ pub struct ReadHalf(i32); #[derive(Debug, PartialEq, Eq)] pub struct Pipe { - tx: i32, - rx: i32, + tx: RawPipe, + rx: RawPipe, } -// Make sure when operating on consumers of Pipe of Read/WriteHalf, the pipe structures are `mem::forget`ed before returning, since the Drop impl of those types closes the raw socket. impl Pipe { /// Split this pipe into a read and write half. @@ -67,9 +140,7 @@ impl Pipe /// Data written to the `WriteHalf` is sent to the `ReadHalf` (unless the pipe is mismatched.) #[inline] pub fn split(self) -> (WriteHalf, ReadHalf) { - let rv = (WriteHalf(self.tx), ReadHalf(self.rx)); - std::mem::forget(self); - rv + (WriteHalf(self.tx), ReadHalf(self.rx)) } /// Create a `Pipe` from two halves. @@ -79,16 +150,14 @@ impl Pipe /// If they are not, then writing to the `Pipe` will send the data to its original receiver, and reading from it will take the data from its original sender. #[inline] pub fn unsplit(tx: WriteHalf, rx: ReadHalf) -> Self { - let v = Self::new_from_raw((tx.0, rx.0)); - std::mem::forget((tx, rx)); - v + Self::new_from_raw((tx.0, rx.0)) } /// Create a new `Pipe` from a pair of raw file descriptors. /// /// # Safety /// Does not check that the integers provided are valid and open file descriptors. - #[inline(always)] fn new_from_raw((tx, rx): (i32, i32)) -> Self + #[inline(always)] fn new_from_raw((tx, rx): (RawPipe, RawPipe)) -> Self { Self{tx,rx} } @@ -98,17 +167,15 @@ impl Pipe /// # Safety /// The caller must check that `tx` and `rx` are valid, open file descriptors // TODO: When writing the async version make sure to state that the fds need to be non-blocking. - #[inline] pub unsafe fn from_raw(tx: i32, rx: i32) -> Self + #[inline] pub unsafe fn from_raw(tx: RawPipe, rx: RawPipe) -> Self { Self::new_from_raw((tx, rx)) } /// Transfer the ownership of the socket's inner Tx and Rx fds. - pub fn into_raw(self) -> (i32, i32) + pub fn into_raw(self) -> (RawPipe, RawPipe) { - let txrx = (self.tx, self.rx); - std::mem::forget(self); - txrx + (self.tx, self.rx) } /// Create a new `Pipe` from two new file descriptors that stream to eachother. @@ -163,59 +230,32 @@ fn read_raw(sock: i32, data: &mut [u8]) -> Result impl io::Read for Pipe { #[inline] fn read(&mut self, buf: &mut [u8]) -> io::Result { - Ok(read_raw(self.rx, buf)?) + self.rx.read(buf) } } impl io::Write for Pipe { #[inline] fn write(&mut self, buf: &[u8]) -> io::Result { - Ok(write_raw(self.tx, buf)?) + self.tx.write(buf) } #[inline] fn flush(&mut self) -> io::Result<()> { - Ok(()) + self.tx.flush() } } impl io::Read for ReadHalf { #[inline] fn read(&mut self, buf: &mut [u8]) -> io::Result { - Ok(read_raw(self.0, buf)?) + self.0.read(buf) } } impl io::Write for WriteHalf { #[inline] fn write(&mut self, buf: &[u8]) -> io::Result { - Ok(write_raw(self.0, buf)?) + self.0.write(buf) } #[inline] fn flush(&mut self) -> io::Result<()> { - Ok(()) - } -} - - -impl Drop for ReadHalf -{ - fn drop(&mut self) { - unsafe { - libc::close(self.0); - } - } -} -impl Drop for WriteHalf -{ - fn drop(&mut self) { - unsafe { - libc::close(self.0); - } - } -} -impl Drop for Pipe -{ - fn drop(&mut self) { - unsafe { - libc::close(self.tx); - libc::close(self.rx); - } + self.0.flush() } } @@ -323,3 +363,19 @@ impl fmt::Display for Error } } +#[cfg(test)] +mod tests +{ + use std::io::{Read, Write}; + #[test] + fn pipe_single_rw() + { + const DATA: &'static [u8] = b"hello world"; + let mut pipe = super::Pipe::new().expect("Creation"); + print!("Pipe: {:?}", pipe); + pipe.write_all(DATA).expect("write (ex)"); + let mut buffer = [0u8; DATA.len()]; + pipe.read_exact(&mut buffer[..]).expect("read (ex)"); + assert_eq!(&buffer, DATA, "data different"); + } +}