fix use-after-free soundness issues

src/descriptor/mod.rs

@@ -1,59 +1,23 @@
 mod err;
 
 pub use self::err::DescriptorError;
-use std::ffi::CStr;
-use std::os::unix::io::RawFd;
+use std::{
+    ffi::CStr,
+    os::fd::{FromRawFd, OwnedFd},
+};
 
-/// # Safety
-///
-/// Implmentations MUST only return valid fds from
-/// these methods. Callers MUST NOT use the fds once the
-/// returning object has fallen out of scope.
-pub unsafe trait Descriptor {
-    /// If the descriptor has a valid fd, return it
-    fn take_raw_fd(&mut self) -> Option<RawFd>;
-
-    /// The constructor function `open` opens the path
-    /// and returns the fd.
-    fn open(
-        path: &CStr,
-        flag: libc::c_int,
-        mode: Option<libc::c_int>,
-    ) -> Result<RawFd, DescriptorError> {
-        // Safety: we've just ensured that path is non-null and the
-        // other params are valid by construction.
-        unsafe {
-            match libc::open(path.as_ptr().cast(), flag, mode.unwrap_or_default()) {
-                -1 => Err(DescriptorError::OpenFail),
-                fd => Ok(fd),
-            }
-        }
-    }
-
-    /// The function `close` leaves the fd.
-    fn close(&mut self) -> Result<(), DescriptorError> {
-        if let Some(fd) = self.take_raw_fd() {
-            // Safety: we take the fd here, ensuring that it cannot
-            // be used or closed again afterwards. N.B. this is only
-            // safe because there is no way for a descriptor to hand
-            // out the raw fd for it to be stored somewhere and then
-            // used by external code.
-            unsafe {
-                match libc::close(fd) {
-                    -1 => Err(DescriptorError::CloseFail),
-                    _ => Ok(()),
-                }
-            }
-        } else {
-            Ok(())
-        }
-    }
-
-    /// The destructor function `drop` call the method `close`
-    /// and log if an error occurred.
-    fn drop(&mut self) {
-        if let Err(e) = self.close() {
-            log::warn!("error closing fd: {:?}", e);
+/// Open the given file as a file descriptor.
+pub fn open(
+    path: &CStr,
+    flag: libc::c_int,
+    mode: Option<libc::c_int>,
+) -> Result<OwnedFd, DescriptorError> {
+    // Safety: we've just ensured that path is non-null and the
+    // other params are valid by construction.
+    unsafe {
+        match libc::open(path.as_ptr().cast(), flag, mode.unwrap_or_default()) {
+            -1 => Err(DescriptorError::OpenFail),
+            fd => Ok(OwnedFd::from_raw_fd(fd)),
         }
     }
 }

src/fork/mod.rs

@@ -1,8 +1,6 @@
 mod err;
 mod pty;
 
-use descriptor::Descriptor;
-
 pub use self::err::{ForkError, Result};
 pub use self::pty::{Master, MasterError};
 pub use self::pty::{Slave, SlaveError};
@@ -132,7 +130,7 @@ impl Fork {
     pub fn is_parent(&self) -> Result<Master> {
         match *self {
             Fork::Child(_) => Err(ForkError::IsChild),
-            Fork::Parent(_, ref master) => Ok(*master),
+            Fork::Parent(_, ref master) => Ok(master.clone()),
         }
     }
 
@@ -145,11 +143,3 @@ impl Fork {
         }
     }
 }
-
-impl Drop for Fork {
-    fn drop(&mut self) {
-        if let Fork::Parent(_, master) = self {
-            Descriptor::drop(master)
-        }
-    }
-}

src/fork/pty/master/err.rs

@@ -12,7 +12,6 @@ pub enum MasterError {
     GrantptError,
     UnlockptError,
     PtsnameError,
-    NoFdError,
 }
 
 impl fmt::Display for MasterError {
@@ -30,7 +29,6 @@ impl Error for MasterError {
             MasterError::GrantptError => "the `grantpt` has a error, errnois set appropriately.",
             MasterError::UnlockptError => "the `grantpt` has a error, errnois set appropriately.",
             MasterError::PtsnameError => "the `ptsname` has a error",
-            MasterError::NoFdError => "already closed, no fd",
         }
     }
 

src/fork/pty/master/mod.rs

@@ -3,126 +3,117 @@ mod err;
 #[cfg(target_os = "macos")]
 mod ptsname_r_macos;
 
-use descriptor::Descriptor;
+use crate::descriptor;
 
 pub use self::err::{MasterError, Result};
-use std::ffi::CStr;
-use std::io;
-use std::os::fd::BorrowedFd;
-use std::os::unix::io::RawFd;
-
-#[derive(Debug, Copy, Clone)]
+use std::{
+    ffi::CStr,
+    io,
+    os::{
+        fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd},
+        unix::io::RawFd,
+    },
+    sync::Arc,
+};
+
+#[derive(Debug, Clone)]
 pub struct Master {
-    pty: Option<RawFd>,
+    pty: Arc<OwnedFd>,
 }
 
 impl Master {
     pub fn new(path: &CStr) -> Result<Self> {
-        match Self::open(path, libc::O_RDWR, None) {
+        match descriptor::open(path, libc::O_RDWR, None) {
             Err(cause) => Err(MasterError::BadDescriptor(cause)),
-            Ok(fd) => Ok(Master { pty: Some(fd) }),
+            Ok(fd) => Ok(Master { pty: Arc::new(fd) }),
         }
     }
 
     /// Extract the raw fd from the underlying object
-    pub fn raw_fd(&self) -> &Option<RawFd> {
-        &self.pty
+    pub fn raw_fd(&self) -> RawFd {
+        self.pty.as_raw_fd()
     }
 
     /// Borrow the raw fd
-    pub fn borrow_fd(&self) -> Option<BorrowedFd<'_>> {
-        // Safety: we only ever close on drop, so this will be
-        //         live the whole time.
-        self.pty.map(|fd| unsafe { BorrowedFd::borrow_raw(fd) })
+    pub fn borrow_fd(&self) -> BorrowedFd<'_> {
+        self.pty.as_fd()
     }
 
     /// Change UID and GID of slave pty associated with master pty whose
     /// fd is provided, to the real UID and real GID of the calling thread.
     pub fn grantpt(&self) -> Result<libc::c_int> {
-        if let Some(fd) = self.pty {
-            unsafe {
-                match libc::grantpt(fd) {
-                    -1 => Err(MasterError::GrantptError),
-                    c => Ok(c),
-                }
+        // Safety: pty is live across the lifetime of this call,
+        // so the fd is valid.
+        unsafe {
+            match libc::grantpt(self.raw_fd()) {
+                -1 => Err(MasterError::GrantptError),
+                c => Ok(c),
             }
-        } else {
-            Err(MasterError::NoFdError)
         }
     }
 
     /// Unlock the slave pty associated with the master to which fd refers.
     pub fn unlockpt(&self) -> Result<libc::c_int> {
-        if let Some(fd) = self.pty {
-            unsafe {
-                match libc::unlockpt(fd) {
-                    -1 => Err(MasterError::UnlockptError),
-                    c => Ok(c),
-                }
+        // Safety: pty is live across the lifetime of this call,
+        // so the fd is valid.
+        unsafe {
+            match libc::unlockpt(self.raw_fd()) {
+                -1 => Err(MasterError::UnlockptError),
+                c => Ok(c),
             }
-        } else {
-            Err(MasterError::NoFdError)
         }
     }
 
     /// Returns a pointer to a static buffer, which will be overwritten on
     /// subsequent calls.
     pub fn ptsname_r(&self, buf: &mut [u8]) -> Result<()> {
-        if let Some(fd) = self.pty {
-            // Safety: the vector's memory is valid for the duration
-            // of the call
-            unsafe {
-                let data: *mut u8 = &mut buf[0];
-
-                #[cfg(any(target_os = "linux", target_os = "android"))]
-                let result = libc::ptsname_r(fd, data as *mut libc::c_char, buf.len());
-
-                #[cfg(target_os = "macos")]
-                let result = ptsname_r_macos::ptsname_r(fd, data as *mut libc::c_char, buf.len());
-
-                match result {
-                    0 => Ok(()),
-                    _ => Err(MasterError::PtsnameError), // should probably capture errno
-                }
+        // Safety: the vector's memory is valid for the duration
+        // of the call
+        unsafe {
+            let data: *mut u8 = &mut buf[0];
+
+            #[cfg(any(target_os = "linux", target_os = "android"))]
+            let result = libc::ptsname_r(self.raw_fd(), data as *mut libc::c_char, buf.len());
+
+            #[cfg(target_os = "macos")]
+            let result = ptsname_r_macos::ptsname_r(fd, data as *mut libc::c_char, buf.len());
+
+            match result {
+                0 => Ok(()),
+                _ => Err(MasterError::PtsnameError), // should probably capture errno
             }
-        } else {
-            Err(MasterError::NoFdError)
         }
     }
 }
 
-unsafe impl Descriptor for Master {
-    fn take_raw_fd(&mut self) -> Option<RawFd> {
-        self.pty.take()
-    }
-}
-
 impl io::Read for Master {
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-        if let Some(fd) = self.pty {
-            unsafe {
-                match libc::read(fd, buf.as_mut_ptr() as *mut libc::c_void, buf.len()) {
-                    -1 => Ok(0),
-                    len => Ok(len as usize),
-                }
+        // Safety: the vector's memory is valid for the duration
+        // of the call
+        unsafe {
+            match libc::read(
+                self.raw_fd(),
+                buf.as_mut_ptr() as *mut libc::c_void,
+                buf.len(),
+            ) {
+                -1 => Ok(0),
+                len => Ok(len as usize),
             }
-        } else {
-            Err(io::Error::other("already closed"))
         }
     }
 }
 
 impl io::Write for Master {
     fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        if let Some(fd) = self.pty {
-            unsafe {
-                match libc::write(fd, buf.as_ptr() as *const libc::c_void, buf.len()) {
-                    -1 => Err(io::Error::last_os_error()),
-                    ret => Ok(ret as usize),
-                }
+        unsafe {
+            match libc::write(
+                self.raw_fd(),
+                buf.as_ptr() as *const libc::c_void,
+                buf.len(),
+            ) {
+                -1 => Err(io::Error::last_os_error()),
+                ret => Ok(ret as usize),
             }
-        } else {
-            Err(io::Error::other("already closed"))
         }
     }
 

src/fork/pty/slave/err.rs

@@ -10,7 +10,6 @@ pub type Result<T> = ::std::result::Result<T, SlaveError>;
 pub enum SlaveError {
     BadDescriptor(DescriptorError),
     Dup2Error,
-    NoFdError,
 }
 
 impl fmt::Display for SlaveError {
@@ -26,7 +25,6 @@ impl Error for SlaveError {
         match *self {
             SlaveError::BadDescriptor(_) => "the descriptor as occured an error",
             SlaveError::Dup2Error => "the `dup2` has a error, errno isset appropriately.",
-            SlaveError::NoFdError => "the pty has already been closed, no fd",
         }
     }