refactor: redefine ForkptyResult to avoid uninited master field (#2315)

* refactor: redefine ForkptyResult to avoid uninited master field

* docs: safety

Author: SteveLauC

changelog/2315.changed.md

@@ -0,0 +1,12 @@
+Change the `ForkptyResult` type to the following repr so that the uninitialized 
+`master` field won't be accessed in the child process:
+
+```rs
+pub enum ForkptyResult {
+    Parent {
+        child: Pid,
+        master: OwnedFd,
+    },
+    Child,
+}
+```

src/pty.rs

@@ -12,8 +12,6 @@ use std::os::unix::prelude::*;
 use crate::errno::Errno;
 #[cfg(not(target_os = "aix"))]
 use crate::sys::termios::Termios;
-#[cfg(feature = "process")]
-use crate::unistd::ForkResult;
 #[cfg(all(feature = "process", not(target_os = "aix")))]
 use crate::unistd::Pid;
 use crate::{fcntl, unistd, Result};
@@ -31,15 +29,19 @@ pub struct OpenptyResult {
 
 feature! {
 #![feature = "process"]
-/// Representation of a master with a forked pty
-///
-/// This is returned by [`forkpty`].
+/// A successful result of [`forkpty()`].
 #[derive(Debug)]
-pub struct ForkptyResult {
-    /// The master port in a virtual pty pair
-    pub master: OwnedFd,
-    /// Metadata about forked process
-    pub fork_result: ForkResult,
+pub enum ForkptyResult {
+    /// This is the parent process of the underlying fork.
+    Parent {
+        /// The PID of the fork's child process
+        child: Pid,
+        /// A file descriptor referring to master side of the pseudoterminal of
+        /// the child process.
+        master: OwnedFd,
+    },
+    /// This is the child process of the underlying fork.
+    Child,
 }
 }
 
@@ -300,9 +302,7 @@ pub fn openpty<
 
 feature! {
 #![feature = "process"]
-/// Create a new pseudoterminal, returning the master file descriptor and forked pid.
-/// in `ForkptyResult`
-/// (see [`forkpty`](https://man7.org/linux/man-pages/man3/forkpty.3.html)).
+/// Create a new process operating in a pseudoterminal.
 ///
 /// If `winsize` is not `None`, the window size of the slave will be set to
 /// the values in `winsize`. If `termios` is not `None`, the pseudoterminal's
@@ -319,6 +319,11 @@ feature! {
 /// special care must be taken to only invoke code you can control and audit.
 ///
 /// [async-signal-safe]: https://man7.org/linux/man-pages/man7/signal-safety.7.html
+///
+/// # Reference
+///
+/// * [FreeBSD](https://man.freebsd.org/cgi/man.cgi?query=forkpty)
+/// * [Linux](https://man7.org/linux/man-pages/man3/forkpty.3.html)
 #[cfg(not(target_os = "aix"))]
 pub unsafe fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b Termios>>>(
     winsize: T,
@@ -343,14 +348,23 @@ pub unsafe fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b T
 
     let res = unsafe { libc::forkpty(master.as_mut_ptr(), ptr::null_mut(), term, win) };
 
-    let fork_result = Errno::result(res).map(|res| match res {
-        0 => ForkResult::Child,
-        res => ForkResult::Parent { child: Pid::from_raw(res) },
-    })?;
+    let success_ret = Errno::result(res)?;
+    let forkpty_result = match success_ret {
+        // In the child process
+        0 => ForkptyResult::Child,
+        // In the parent process
+        child_pid => {
+            // SAFETY:
+            // 1. The master buffer is guaranteed to be initialized in the parent process
+            // 2. OwnedFd::from_raw_fd won't panic as the fd is a valid file descriptor
+            let master = unsafe { OwnedFd::from_raw_fd( master.assume_init() ) };
+            ForkptyResult::Parent {
+                    master,
+                    child: Pid::from_raw(child_pid),
+            }
+        }
+    };
 
-    Ok(ForkptyResult {
-        master: unsafe { OwnedFd::from_raw_fd( master.assume_init() ) },
-        fork_result,
-    })
+    Ok(forkpty_result)
 }
 }

test/test_pty.rs

@@ -8,6 +8,7 @@ use nix::fcntl::{open, OFlag};
 use nix::pty::*;
 use nix::sys::stat;
 use nix::sys::termios::*;
+use nix::sys::wait::WaitStatus;
 use nix::unistd::{pause, write};
 
 /// Test equivalence of `ptsname` and `ptsname_r`
@@ -247,29 +248,29 @@ fn test_openpty_with_termios() {
 fn test_forkpty() {
     use nix::sys::signal::*;
     use nix::sys::wait::wait;
-    use nix::unistd::ForkResult::*;
     // forkpty calls openpty which uses ptname(3) internally.
     let _m0 = crate::PTSNAME_MTX.lock();
     // forkpty spawns a child process
     let _m1 = crate::FORK_MTX.lock();
 
     let string = "naninani\n";
     let echoed_string = "naninani\r\n";
-    let pty = unsafe { forkpty(None, None).unwrap() };
-    match pty.fork_result {
-        Child => {
+    let res = unsafe { forkpty(None, None).unwrap() };
+    match res {
+        ForkptyResult::Child => {
             write(stdout(), string.as_bytes()).unwrap();
             pause(); // we need the child to stay alive until the parent calls read
             unsafe {
                 _exit(0);
             }
         }
-        Parent { child } => {
+        ForkptyResult::Parent { child, master } => {
             let mut buf = [0u8; 10];
             assert!(child.as_raw() > 0);
-            crate::read_exact(&pty.master, &mut buf);
+            crate::read_exact(&master, &mut buf);
             kill(child, SIGTERM).unwrap();
-            wait().unwrap(); // keep other tests using generic wait from getting our child
+            let status = wait().unwrap(); // keep other tests using generic wait from getting our child
+            assert_eq!(status, WaitStatus::Signaled(child, SIGTERM, false));
             assert_eq!(&buf, echoed_string.as_bytes());
         }
     }