Use BorrowedFd instead of RawFd where possible

BorrowedFd ensures that the fd doesn't get closed while it is still
getting used.

Author: bjorn3

src/common/bin_serde.rs

@@ -3,7 +3,10 @@ use sealed::DeSerializeBytes;
 use std::{
     io::{self, Read, Write},
     marker::PhantomData,
-    os::{fd::AsRawFd, unix::net::UnixStream},
+    os::{
+        fd::{AsFd, BorrowedFd},
+        unix::net::UnixStream,
+    },
 };
 
 mod sealed {
@@ -83,9 +86,9 @@ impl<R: DeSerialize, W: DeSerialize> BinPipe<R, W> {
     }
 }
 
-impl<R: DeSerialize, W: DeSerialize> AsRawFd for BinPipe<R, W> {
-    fn as_raw_fd(&self) -> std::os::fd::RawFd {
-        self.sock.as_raw_fd()
+impl<R: DeSerialize, W: DeSerialize> AsFd for BinPipe<R, W> {
+    fn as_fd(&self) -> BorrowedFd {
+        self.sock.as_fd()
     }
 }
 

src/cutils/mod.rs

@@ -1,6 +1,9 @@
 use std::{
     ffi::{CStr, OsStr, OsString},
-    os::unix::prelude::OsStrExt,
+    os::{
+        fd::{AsRawFd, BorrowedFd},
+        unix::prelude::OsStrExt,
+    },
 };
 
 pub fn cerr<Int: Copy + TryInto<libc::c_long>>(res: Int) -> std::io::Result<Int> {
@@ -69,13 +72,13 @@ pub unsafe fn os_string_from_ptr(ptr: *const libc::c_char) -> OsString {
 /// Rust's standard library IsTerminal just directly calls isatty, which
 /// we don't want since this performs IOCTL calls on them and file descriptors are under
 /// the control of the user; so this checks if they are a character device first.
-pub fn safe_isatty(fildes: libc::c_int) -> bool {
+pub fn safe_isatty(fildes: BorrowedFd) -> bool {
     // The Rust standard library doesn't have FileTypeExt on Std{in,out,err}, so we
     // can't just use FileTypeExt::is_char_device and have to resort to libc::fstat.
     let mut maybe_stat = std::mem::MaybeUninit::<libc::stat>::uninit();
 
     // SAFETY: we are passing fstat a pointer to valid memory
-    if unsafe { libc::fstat(fildes, maybe_stat.as_mut_ptr()) } == 0 {
+    if unsafe { libc::fstat(fildes.as_raw_fd(), maybe_stat.as_mut_ptr()) } == 0 {
         // SAFETY: if `fstat` returned 0, maybe_stat will be initialized
         let mode = unsafe { maybe_stat.assume_init() }.st_mode;
 
@@ -84,7 +87,7 @@ pub fn safe_isatty(fildes: libc::c_int) -> bool {
 
         if is_char_device {
             // SAFETY: isatty will return 0 or 1
-            unsafe { libc::isatty(fildes) != 0 }
+            unsafe { libc::isatty(fildes.as_raw_fd()) != 0 }
         } else {
             false
         }
@@ -115,29 +118,15 @@ mod test {
 
     #[test]
     fn test_tty() {
+        use crate::system::term::Pty;
         use std::fs::File;
-        use std::os::fd::AsRawFd;
-        assert!(!super::safe_isatty(
-            File::open("/bin/sh").unwrap().as_raw_fd()
-        ));
-        assert!(!super::safe_isatty(-837492));
-        let (mut leader, mut follower) = Default::default();
-        assert!(
-            unsafe {
-                libc::openpty(
-                    &mut leader,
-                    &mut follower,
-                    std::ptr::null_mut(),
-                    std::ptr::null_mut(),
-                    std::ptr::null_mut(),
-                )
-            } == 0
-        );
-        assert!(super::safe_isatty(leader));
-        assert!(super::safe_isatty(follower));
-        unsafe {
-            libc::close(follower);
-            libc::close(leader);
-        }
+        use std::os::fd::{AsFd, BorrowedFd};
+        assert!(!super::safe_isatty(File::open("/bin/sh").unwrap().as_fd()));
+        assert!(!super::safe_isatty(unsafe {
+            BorrowedFd::borrow_raw(-837492)
+        }));
+        let pty = Pty::open().unwrap();
+        assert!(super::safe_isatty(pty.leader.as_fd()));
+        assert!(super::safe_isatty(pty.follower.as_fd()));
     }
 }

src/exec/event.rs

@@ -1,7 +1,7 @@
 use std::{
     fmt::Debug,
     io,
-    os::fd::{AsRawFd, RawFd},
+    os::fd::{AsFd, AsRawFd, RawFd},
 };
 
 use libc::{c_short, pollfd, POLLIN, POLLOUT};
@@ -103,6 +103,7 @@ pub enum PollEvent {
 }
 
 struct PollFd<T: Process> {
+    // FIXME ensure that the fd is not closed while the event registry is still open
     raw_fd: RawFd,
     event_flags: c_short,
     should_poll: bool,
@@ -126,7 +127,7 @@ impl<T: Process> EventRegistry<T> {
 
     /// Set the `fd` descriptor to be polled for `poll_event` events and produce `event` when `fd` is
     /// ready.
-    pub(super) fn register_event<F: AsRawFd>(
+    pub(super) fn register_event<F: AsFd>(
         &mut self,
         fd: &F,
         poll_event: PollEvent,
@@ -135,7 +136,7 @@ impl<T: Process> EventRegistry<T> {
         let id = EventId(self.poll_fds.len());
 
         self.poll_fds.push(PollFd {
-            raw_fd: fd.as_raw_fd(),
+            raw_fd: fd.as_fd().as_raw_fd(),
             event_flags: match poll_event {
                 PollEvent::Readable => POLLIN,
                 PollEvent::Writable => POLLOUT,

src/exec/use_pty/backchannel.rs

@@ -2,7 +2,7 @@ use std::{
     ffi::c_int,
     io,
     mem::size_of,
-    os::fd::{AsRawFd, RawFd},
+    os::fd::{AsFd, BorrowedFd},
 };
 
 use crate::{
@@ -190,9 +190,9 @@ impl ParentBackchannel {
     }
 }
 
-impl AsRawFd for ParentBackchannel {
-    fn as_raw_fd(&self) -> RawFd {
-        self.socket.as_raw_fd()
+impl AsFd for ParentBackchannel {
+    fn as_fd(&self) -> BorrowedFd {
+        self.socket.as_fd()
     }
 }
 
@@ -309,8 +309,8 @@ impl MonitorBackchannel {
     }
 }
 
-impl AsRawFd for MonitorBackchannel {
-    fn as_raw_fd(&self) -> RawFd {
-        self.socket.as_raw_fd()
+impl AsFd for MonitorBackchannel {
+    fn as_fd(&self) -> BorrowedFd {
+        self.socket.as_fd()
     }
 }

src/exec/use_pty/pipe/mod.rs

@@ -3,7 +3,7 @@ mod ring_buffer;
 use std::{
     io::{self, Read, Write},
     marker::PhantomData,
-    os::fd::AsRawFd,
+    os::fd::AsFd,
 };
 
 use crate::exec::event::{EventHandle, EventRegistry, PollEvent, Process};
@@ -18,7 +18,7 @@ pub(super) struct Pipe<L, R> {
     buffer_rl: Buffer<R, L>,
 }
 
-impl<L: Read + Write + AsRawFd, R: Read + Write + AsRawFd> Pipe<L, R> {
+impl<L: Read + Write + AsFd, R: Read + Write + AsFd> Pipe<L, R> {
     /// Create a new pipe between two read-write types and register them to be polled.
     pub fn new<T: Process>(
         left: L,

src/system/mod.rs

@@ -9,7 +9,7 @@ use std::{
     mem::MaybeUninit,
     ops,
     os::{
-        fd::AsRawFd,
+        fd::{AsFd, AsRawFd},
         unix::{self, prelude::OsStrExt},
     },
     path::{Path, PathBuf},
@@ -72,8 +72,8 @@ impl FileCloser {
         }
     }
 
-    pub(crate) fn except<F: AsRawFd>(&mut self, fd: &F) {
-        self.fds.insert(fd.as_raw_fd() as c_uint);
+    pub(crate) fn except<F: AsFd>(&mut self, fd: &F) {
+        self.fds.insert(fd.as_fd().as_raw_fd() as c_uint);
     }
 
     /// Close every file descriptor that is not one of the IO streams or one of the file
@@ -909,7 +909,10 @@ pub(crate) const ROOT_GROUP_NAME: &str = "wheel";
 mod tests {
     use std::{
         io::{self, Read, Write},
-        os::{fd::AsRawFd, unix::net::UnixStream},
+        os::{
+            fd::{AsFd, AsRawFd},
+            unix::net::UnixStream,
+        },
         process::exit,
     };
 
@@ -1093,8 +1096,8 @@ mod tests {
         );
     }
 
-    fn is_closed<F: AsRawFd>(fd: &F) -> bool {
-        crate::cutils::cerr(unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_GETFD) })
+    fn is_closed<F: AsFd>(fd: &F) -> bool {
+        crate::cutils::cerr(unsafe { libc::fcntl(fd.as_fd().as_raw_fd(), libc::F_GETFD) })
             .is_err_and(|err| err.raw_os_error() == Some(libc::EBADF))
     }
 

src/system/signal/stream.rs

@@ -2,7 +2,7 @@ use std::{
     io,
     mem::MaybeUninit,
     os::{
-        fd::{AsRawFd, RawFd},
+        fd::{AsFd, AsRawFd, BorrowedFd},
         unix::net::UnixStream,
     },
     sync::OnceLock,
@@ -37,7 +37,7 @@ pub(super) unsafe fn send_siginfo(
 /// [`super::SignalHandlerBehavior::Stream`] behavior.
 ///
 /// This is a singleton type. Meaning that there will be only one value of this type during the
-/// execution of a program.  
+/// execution of a program.
 pub(crate) struct SignalStream {
     rx: UnixStream,
     tx: UnixStream,
@@ -105,8 +105,8 @@ pub(crate) fn register_handlers<const N: usize>(
     Ok(handlers.map(|(_, handler)| unsafe { handler.assume_init() }))
 }
 
-impl AsRawFd for SignalStream {
-    fn as_raw_fd(&self) -> RawFd {
-        self.rx.as_raw_fd()
+impl AsFd for SignalStream {
+    fn as_fd(&self) -> BorrowedFd {
+        self.rx.as_fd()
     }
 }

src/system/term/mod.rs

@@ -5,7 +5,7 @@ use std::{
     fmt,
     fs::File,
     io,
-    os::fd::{AsRawFd, FromRawFd, OwnedFd},
+    os::fd::{AsFd, AsRawFd, FromRawFd, OwnedFd},
     ptr::null_mut,
 };
 
@@ -115,9 +115,9 @@ impl io::Write for PtyLeader {
     }
 }
 
-impl AsRawFd for PtyLeader {
-    fn as_raw_fd(&self) -> std::os::fd::RawFd {
-        self.file.as_raw_fd()
+impl AsFd for PtyLeader {
+    fn as_fd(&self) -> std::os::fd::BorrowedFd<'_> {
+        self.file.as_fd()
     }
 }
 
@@ -131,9 +131,9 @@ impl PtyFollower {
     }
 }
 
-impl AsRawFd for PtyFollower {
-    fn as_raw_fd(&self) -> std::os::fd::RawFd {
-        self.file.as_raw_fd()
+impl AsFd for PtyFollower {
+    fn as_fd(&self) -> std::os::fd::BorrowedFd<'_> {
+        self.file.as_fd()
     }
 }
 
@@ -144,11 +144,11 @@ impl From<PtyFollower> for std::process::Stdio {
 }
 
 mod sealed {
-    use std::os::fd::AsRawFd;
+    use std::os::fd::AsFd;
 
     pub(crate) trait Sealed {}
 
-    impl<F: AsRawFd> Sealed for F {}
+    impl<F: AsFd> Sealed for F {}
 }
 
 pub(crate) trait Terminal: sealed::Sealed {
@@ -160,49 +160,49 @@ pub(crate) trait Terminal: sealed::Sealed {
     fn tcgetsid(&self) -> io::Result<ProcessId>;
 }
 
-impl<F: AsRawFd> Terminal for F {
+impl<F: AsFd> Terminal for F {
     /// Get the foreground process group ID associated with this terminal.
     fn tcgetpgrp(&self) -> io::Result<ProcessId> {
         // SAFETY: tcgetpgrp cannot cause UB
-        let id = cerr(unsafe { libc::tcgetpgrp(self.as_raw_fd()) })?;
+        let id = cerr(unsafe { libc::tcgetpgrp(self.as_fd().as_raw_fd()) })?;
         Ok(ProcessId::new(id))
     }
     /// Set the foreground process group ID associated with this terminal to `pgrp`.
     fn tcsetpgrp(&self, pgrp: ProcessId) -> io::Result<()> {
         // SAFETY: tcsetpgrp cannot cause UB
-        cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp.inner()) }).map(|_| ())
+        cerr(unsafe { libc::tcsetpgrp(self.as_fd().as_raw_fd(), pgrp.inner()) }).map(|_| ())
     }
 
     /// Make the given terminal the controlling terminal of the calling process.
     fn make_controlling_terminal(&self) -> io::Result<()> {
         // SAFETY: this is a correct way to call the TIOCSCTTY ioctl, see:
         // https://www.man7.org/linux/man-pages/man2/TIOCNOTTY.2const.html
-        cerr(unsafe { libc::ioctl(self.as_raw_fd(), libc::TIOCSCTTY, 0) })?;
+        cerr(unsafe { libc::ioctl(self.as_fd().as_raw_fd(), libc::TIOCSCTTY, 0) })?;
         Ok(())
     }
 
     /// Get the filename of the tty
     fn ttyname(&self) -> io::Result<OsString> {
         let mut buf: [libc::c_char; 1024] = [0; 1024];
 
-        if !safe_isatty(self.as_raw_fd()) {
+        if !safe_isatty(self.as_fd()) {
             return Err(io::ErrorKind::Unsupported.into());
         }
 
         // SAFETY: `buf` is a valid and initialized pointer, and its  correct length is passed
-        cerr(unsafe { libc::ttyname_r(self.as_raw_fd(), buf.as_mut_ptr(), buf.len()) })?;
+        cerr(unsafe { libc::ttyname_r(self.as_fd().as_raw_fd(), buf.as_mut_ptr(), buf.len()) })?;
         // SAFETY: `buf` will have been initialized by the `ttyname_r` call, if it succeeded
         Ok(unsafe { os_string_from_ptr(buf.as_ptr()) })
     }
 
     /// Rust standard library "IsTerminal" is not secure for setuid programs (CVE-2023-2002)
     fn is_terminal(&self) -> bool {
-        safe_isatty(self.as_raw_fd())
+        safe_isatty(self.as_fd())
     }
 
     fn tcgetsid(&self) -> io::Result<ProcessId> {
         // SAFETY: tcgetsid cannot cause UB
-        let id = cerr(unsafe { libc::tcgetsid(self.as_raw_fd()) })?;
+        let id = cerr(unsafe { libc::tcgetsid(self.as_fd().as_raw_fd()) })?;
         Ok(ProcessId::new(id))
     }
 }