diff --git a/changelog/2315.changed.md b/changelog/2315.changed.md new file mode 100644 index 0000000000..bf437876d6 --- /dev/null +++ b/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, +} +``` diff --git a/src/pty.rs b/src/pty.rs index 74f8ecf0df..818b25b1b1 100644 --- a/src/pty.rs +++ b/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>, U: Into>>( winsize: T, @@ -343,14 +348,23 @@ pub unsafe fn forkpty<'a, 'b, T: Into>, U: Into 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) } } diff --git a/test/test_pty.rs b/test/test_pty.rs index 368ec129b0..dda8b3f6ce 100644 --- a/test/test_pty.rs +++ b/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,7 +248,6 @@ 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 @@ -255,21 +255,22 @@ fn test_forkpty() { 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()); } }