feat: I/O safety for 'sys/termios' & 'pty'

Author: SteveLauC

Cargo.toml

@@ -108,7 +108,3 @@ path = "test/test_clearenv.rs"
 name = "test-mount"
 path = "test/test_mount.rs"
 harness = false
-
-[[test]]
-name = "test-ptymaster-drop"
-path = "test/test_ptymaster_drop.rs"

src/pty.rs

@@ -16,78 +16,58 @@ use crate::{fcntl, unistd, Result};
 
 /// Representation of a master/slave pty pair
 ///
-/// This is returned by `openpty`.  Note that this type does *not* implement `Drop`, so the user
-/// must manually close the file descriptors.
-#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+/// This is returned by [`openpty`].
+#[derive(Debug)]
 pub struct OpenptyResult {
     /// The master port in a virtual pty pair
-    pub master: RawFd,
+    pub master: OwnedFd,
     /// The slave port in a virtual pty pair
-    pub slave: RawFd,
+    pub slave: OwnedFd,
 }
 
 feature! {
 #![feature = "process"]
 /// Representation of a master with a forked pty
 ///
-/// This is returned by `forkpty`. Note that this type does *not* implement `Drop`, so the user
-/// must manually close the file descriptors.
-#[derive(Clone, Copy, Debug)]
+/// This is returned by [`forkpty`].
+#[derive(Debug)]
 pub struct ForkptyResult {
     /// The master port in a virtual pty pair
-    pub master: RawFd,
+    pub master: OwnedFd,
     /// Metadata about forked process
     pub fork_result: ForkResult,
 }
 }
 
 /// Representation of the Master device in a master/slave pty pair
 ///
-/// While this datatype is a thin wrapper around `RawFd`, it enforces that the available PTY
-/// functions are given the correct file descriptor. Additionally this type implements `Drop`,
-/// so that when it's consumed or goes out of scope, it's automatically cleaned-up.
-#[derive(Debug, Eq, Hash, PartialEq)]
-pub struct PtyMaster(RawFd);
+/// While this datatype is a thin wrapper around `OwnedFd`, it enforces that the available PTY
+/// functions are given the correct file descriptor.
+#[derive(Debug)]
+pub struct PtyMaster(OwnedFd);
 
 impl AsRawFd for PtyMaster {
     fn as_raw_fd(&self) -> RawFd {
-        self.0
+        self.0.as_raw_fd()
     }
 }
 
 impl IntoRawFd for PtyMaster {
     fn into_raw_fd(self) -> RawFd {
         let fd = self.0;
-        mem::forget(self);
-        fd
-    }
-}
-
-impl Drop for PtyMaster {
-    fn drop(&mut self) {
-        // On drop, we ignore errors like EINTR and EIO because there's no clear
-        // way to handle them, we can't return anything, and (on FreeBSD at
-        // least) the file descriptor is deallocated in these cases.  However,
-        // we must panic on EBADF, because it is always an error to close an
-        // invalid file descriptor.  That frequently indicates a double-close
-        // condition, which can cause confusing errors for future I/O
-        // operations.
-        let e = unistd::close(self.0);
-        if e == Err(Errno::EBADF) {
-            panic!("Closing an invalid file descriptor!");
-        };
+        fd.into_raw_fd()
     }
 }
 
 impl io::Read for PtyMaster {
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-        unistd::read(self.0, buf).map_err(io::Error::from)
+        unistd::read(self.0.as_raw_fd(), buf).map_err(io::Error::from)
     }
 }
 
 impl io::Write for PtyMaster {
     fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        unistd::write(self.0, buf).map_err(io::Error::from)
+        unistd::write(self.0.as_raw_fd(), buf).map_err(io::Error::from)
     }
     fn flush(&mut self) -> io::Result<()> {
         Ok(())
@@ -96,13 +76,13 @@ impl io::Write for PtyMaster {
 
 impl io::Read for &PtyMaster {
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-        unistd::read(self.0, buf).map_err(io::Error::from)
+        unistd::read(self.0.as_raw_fd(), buf).map_err(io::Error::from)
     }
 }
 
 impl io::Write for &PtyMaster {
     fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        unistd::write(self.0, buf).map_err(io::Error::from)
+        unistd::write(self.0.as_raw_fd(), buf).map_err(io::Error::from)
     }
     fn flush(&mut self) -> io::Result<()> {
         Ok(())
@@ -164,7 +144,7 @@ pub fn posix_openpt(flags: fcntl::OFlag) -> Result<PtyMaster> {
         return Err(Errno::last());
     }
 
-    Ok(PtyMaster(fd))
+    Ok(PtyMaster(unsafe {OwnedFd::from_raw_fd(fd) }))
 }
 
 /// Get the name of the slave pseudoterminal (see
@@ -308,8 +288,8 @@ pub fn openpty<
 
     unsafe {
         Ok(OpenptyResult {
-            master: master.assume_init(),
-            slave: slave.assume_init(),
+            master: OwnedFd::from_raw_fd(master.assume_init()),
+            slave: OwnedFd::from_raw_fd(slave.assume_init()),
         })
     }
 }
@@ -364,7 +344,7 @@ pub unsafe fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b T
     })?;
 
     Ok(ForkptyResult {
-        master: master.assume_init(),
+        master: OwnedFd::from_raw_fd(master.assume_init()),
         fork_result,
     })
 }

src/sys/termios.rs

@@ -222,7 +222,7 @@ use libc::{self, c_int, tcflag_t};
 use std::cell::{Ref, RefCell};
 use std::convert::From;
 use std::mem;
-use std::os::unix::io::RawFd;
+use std::os::unix::io::{AsFd, AsRawFd};
 
 #[cfg(feature = "process")]
 use crate::unistd::Pid;
@@ -1143,10 +1143,10 @@ pub fn cfmakesane(termios: &mut Termios) {
 /// `tcgetattr()` returns a `Termios` structure with the current configuration for a port. Modifying
 /// this structure *will not* reconfigure the port, instead the modifications should be done to
 /// the `Termios` structure and then the port should be reconfigured using `tcsetattr()`.
-pub fn tcgetattr(fd: RawFd) -> Result<Termios> {
+pub fn tcgetattr<Fd: AsFd>(fd: &Fd) -> Result<Termios> {
     let mut termios = mem::MaybeUninit::uninit();
 
-    let res = unsafe { libc::tcgetattr(fd, termios.as_mut_ptr()) };
+    let res = unsafe { libc::tcgetattr(fd.as_fd().as_raw_fd(), termios.as_mut_ptr()) };
 
     Errno::result(res)?;
 
@@ -1159,53 +1159,53 @@ pub fn tcgetattr(fd: RawFd) -> Result<Termios> {
 /// `tcsetattr()` reconfigures the given port based on a given `Termios` structure. This change
 /// takes affect at a time specified by `actions`. Note that this function may return success if
 /// *any* of the parameters were successfully set, not only if all were set successfully.
-pub fn tcsetattr(fd: RawFd, actions: SetArg, termios: &Termios) -> Result<()> {
+pub fn tcsetattr<Fd: AsFd>(fd: &Fd, actions: SetArg, termios: &Termios) -> Result<()> {
     let inner_termios = termios.get_libc_termios();
     Errno::result(unsafe {
-        libc::tcsetattr(fd, actions as c_int, &*inner_termios)
+        libc::tcsetattr(fd.as_fd().as_raw_fd(), actions as c_int, &*inner_termios)
     })
     .map(drop)
 }
 
 /// Block until all output data is written (see
 /// [tcdrain(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcdrain.html)).
-pub fn tcdrain(fd: RawFd) -> Result<()> {
-    Errno::result(unsafe { libc::tcdrain(fd) }).map(drop)
+pub fn tcdrain<Fd: AsFd>(fd: &Fd) -> Result<()> {
+    Errno::result(unsafe { libc::tcdrain(fd.as_fd().as_raw_fd()) }).map(drop)
 }
 
 /// Suspend or resume the transmission or reception of data (see
 /// [tcflow(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcflow.html)).
 ///
 /// `tcflow()` suspends of resumes the transmission or reception of data for the given port
 /// depending on the value of `action`.
-pub fn tcflow(fd: RawFd, action: FlowArg) -> Result<()> {
-    Errno::result(unsafe { libc::tcflow(fd, action as c_int) }).map(drop)
+pub fn tcflow<Fd: AsFd>(fd: &Fd, action: FlowArg) -> Result<()> {
+    Errno::result(unsafe { libc::tcflow(fd.as_fd().as_raw_fd(), action as c_int) }).map(drop)
 }
 
 /// Discard data in the output or input queue (see
 /// [tcflush(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcflush.html)).
 ///
 /// `tcflush()` will discard data for a terminal port in the input queue, output queue, or both
 /// depending on the value of `action`.
-pub fn tcflush(fd: RawFd, action: FlushArg) -> Result<()> {
-    Errno::result(unsafe { libc::tcflush(fd, action as c_int) }).map(drop)
+pub fn tcflush<Fd: AsFd>(fd: &Fd, action: FlushArg) -> Result<()> {
+    Errno::result(unsafe { libc::tcflush(fd.as_fd().as_raw_fd(), action as c_int) }).map(drop)
 }
 
 /// Send a break for a specific duration (see
 /// [tcsendbreak(3p)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcsendbreak.html)).
 ///
 /// When using asynchronous data transmission `tcsendbreak()` will transmit a continuous stream
 /// of zero-valued bits for an implementation-defined duration.
-pub fn tcsendbreak(fd: RawFd, duration: c_int) -> Result<()> {
-    Errno::result(unsafe { libc::tcsendbreak(fd, duration) }).map(drop)
+pub fn tcsendbreak<Fd: AsFd>(fd: &Fd, duration: c_int) -> Result<()> {
+    Errno::result(unsafe { libc::tcsendbreak(fd.as_fd().as_raw_fd(), duration) }).map(drop)
 }
 
 feature! {
 #![feature = "process"]
 /// Get the session controlled by the given terminal (see
 /// [tcgetsid(3)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/tcgetsid.html)).
-pub fn tcgetsid(fd: RawFd) -> Result<Pid> {
-    let res = unsafe { libc::tcgetsid(fd) };
+pub fn tcgetsid<Fd: AsFd>(fd: &Fd) -> Result<Pid> {
+    let res = unsafe { libc::tcgetsid(fd.as_fd().as_raw_fd()) };
 
     Errno::result(res).map(Pid::from_raw)
 }

test/sys/test_termios.rs

@@ -1,17 +1,17 @@
-use std::os::unix::prelude::*;
+use std::os::unix::io::{AsFd, AsRawFd};
 use tempfile::tempfile;
 
 use nix::errno::Errno;
 use nix::fcntl;
 use nix::pty::openpty;
 use nix::sys::termios::{self, tcgetattr, LocalFlags, OutputFlags};
-use nix::unistd::{close, read, write};
+use nix::unistd::{read, write};
 
-/// Helper function analogous to `std::io::Write::write_all`, but for `RawFd`s
-fn write_all(f: RawFd, buf: &[u8]) {
+/// Helper function analogous to `std::io::Write::write_all`, but for `Fd`s
+fn write_all<Fd: AsFd>(f: &Fd, buf: &[u8]) {
     let mut len = 0;
     while len < buf.len() {
-        len += write(f, &buf[len..]).unwrap();
+        len += write(f.as_fd().as_raw_fd(), &buf[len..]).unwrap();
     }
 }
 
@@ -22,27 +22,19 @@ fn test_tcgetattr_pty() {
     let _m = crate::PTSNAME_MTX.lock();
 
     let pty = openpty(None, None).expect("openpty failed");
-    termios::tcgetattr(pty.slave).unwrap();
-    close(pty.master).expect("closing the master failed");
-    close(pty.slave).expect("closing the slave failed");
+    termios::tcgetattr(&pty.slave).unwrap();
 }
 
 // Test tcgetattr on something that isn't a terminal
 #[test]
 fn test_tcgetattr_enotty() {
     let file = tempfile().unwrap();
     assert_eq!(
-        termios::tcgetattr(file.as_raw_fd()).err(),
+        termios::tcgetattr(&file).err(),
         Some(Errno::ENOTTY)
     );
 }
 
-// Test tcgetattr on an invalid file descriptor
-#[test]
-fn test_tcgetattr_ebadf() {
-    assert_eq!(termios::tcgetattr(-1).err(), Some(Errno::EBADF));
-}
-
 // Test modifying output flags
 #[test]
 fn test_output_flags() {
@@ -52,11 +44,7 @@ fn test_output_flags() {
     // Open one pty to get attributes for the second one
     let mut termios = {
         let pty = openpty(None, None).expect("openpty failed");
-        assert!(pty.master > 0);
-        assert!(pty.slave > 0);
-        let termios = tcgetattr(pty.slave).expect("tcgetattr failed");
-        close(pty.master).unwrap();
-        close(pty.slave).unwrap();
+        let termios = tcgetattr(&pty.slave).expect("tcgetattr failed");
         termios
     };
 
@@ -73,19 +61,15 @@ fn test_output_flags() {
 
     // Open a pty
     let pty = openpty(None, &termios).unwrap();
-    assert!(pty.master > 0);
-    assert!(pty.slave > 0);
 
     // Write into the master
     let string = "foofoofoo\r";
-    write_all(pty.master, string.as_bytes());
+    write_all(&pty.master, string.as_bytes());
 
     // Read from the slave verifying that the output has been properly transformed
     let mut buf = [0u8; 10];
-    crate::read_exact(pty.slave, &mut buf);
+    crate::read_exact(&pty.slave, &mut buf);
     let transformed_string = "foofoofoo\n";
-    close(pty.master).unwrap();
-    close(pty.slave).unwrap();
     assert_eq!(&buf, transformed_string.as_bytes());
 }
 
@@ -98,11 +82,7 @@ fn test_local_flags() {
     // Open one pty to get attributes for the second one
     let mut termios = {
         let pty = openpty(None, None).unwrap();
-        assert!(pty.master > 0);
-        assert!(pty.slave > 0);
-        let termios = tcgetattr(pty.slave).unwrap();
-        close(pty.master).unwrap();
-        close(pty.slave).unwrap();
+        let termios = tcgetattr(&pty.slave).unwrap();
         termios
     };
 
@@ -114,23 +94,19 @@ fn test_local_flags() {
 
     // Open a new pty with our modified termios settings
     let pty = openpty(None, &termios).unwrap();
-    assert!(pty.master > 0);
-    assert!(pty.slave > 0);
 
     // Set the master is in nonblocking mode or reading will never return.
-    let flags = fcntl::fcntl(pty.master, fcntl::F_GETFL).unwrap();
+    let flags = fcntl::fcntl(pty.master.as_raw_fd(), fcntl::F_GETFL).unwrap();
     let new_flags =
         fcntl::OFlag::from_bits_truncate(flags) | fcntl::OFlag::O_NONBLOCK;
-    fcntl::fcntl(pty.master, fcntl::F_SETFL(new_flags)).unwrap();
+    fcntl::fcntl(pty.master.as_raw_fd(), fcntl::F_SETFL(new_flags)).unwrap();
 
     // Write into the master
     let string = "foofoofoo\r";
-    write_all(pty.master, string.as_bytes());
+    write_all(&pty.master, string.as_bytes());
 
     // Try to read from the master, which should not have anything as echoing was disabled.
     let mut buf = [0u8; 10];
-    let read = read(pty.master, &mut buf).unwrap_err();
-    close(pty.master).unwrap();
-    close(pty.slave).unwrap();
+    let read = read(pty.master.as_raw_fd(), &mut buf).unwrap_err();
     assert_eq!(read, Errno::EAGAIN);
 }

test/test.rs

@@ -66,16 +66,16 @@ mod test_unistd;
 
 use nix::unistd::{chdir, getcwd, read};
 use parking_lot::{Mutex, RwLock, RwLockWriteGuard};
-use std::os::unix::io::RawFd;
+use std::os::unix::io::{AsFd, AsRawFd};
 use std::path::PathBuf;
 
-/// Helper function analogous to `std::io::Read::read_exact`, but for `RawFD`s
-fn read_exact(f: RawFd, buf: &mut [u8]) {
+/// Helper function analogous to `std::io::Read::read_exact`, but for `Fd`s
+fn read_exact<Fd: AsFd>(f: &Fd, buf: &mut [u8]) {
     let mut len = 0;
     while len < buf.len() {
         // get_mut would be better than split_at_mut, but it requires nightly
         let (_, remaining) = buf.split_at_mut(len);
-        len += read(f, remaining).unwrap();
+        len += read(f.as_fd().as_raw_fd(), remaining).unwrap();
     }
 }