From ba53115b41fd783e7dad6aadaa4c65655d634387 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Mon, 2 Sep 2024 23:14:14 +0200 Subject: [PATCH 01/10] some changes to the _nobg functions: - it was incorrect to label tcsetattr_nobg as a safe fn - clarify why the signal handler is safe --- src/system/term/user_term.rs | 67 +++++++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/src/system/term/user_term.rs b/src/system/term/user_term.rs index 7e5b4446a..ce43a0055 100644 --- a/src/system/term/user_term.rs +++ b/src/system/term/user_term.rs @@ -61,12 +61,13 @@ extern "C" fn on_sigttou(_signal: c_int, _info: *mut siginfo_t, _: *mut c_void) } /// This is like `tcsetattr` but it only suceeds if we are in the foreground process group. -fn tcsetattr_nobg(fd: c_int, flags: c_int, tp: *const termios) -> io::Result<()> { +/// # Safety +/// +/// The arguments to this function have to be valid arguments to `tcsetattr`. +unsafe fn tcsetattr_nobg(fd: c_int, flags: c_int, tp: *const termios) -> io::Result<()> { // This function is based around the fact that we receive `SIGTTOU` if we call `tcsetattr` and // we are not in the foreground process group. - let mut original_action = MaybeUninit::::uninit(); - let action = { let mut raw: libc::sigaction = make_zeroed_sigaction(); // Call `on_sigttou` if `SIGTTOU` arrives. @@ -74,19 +75,37 @@ fn tcsetattr_nobg(fd: c_int, flags: c_int, tp: *const termios) -> io::Result<()> // Exclude any other signals from the set raw.sa_mask = { let mut sa_mask = MaybeUninit::::uninit(); - unsafe { sigemptyset(sa_mask.as_mut_ptr()) }; - unsafe { sa_mask.assume_init() } + // SAFETY: sa_mask is a valid and dereferenceble pointer; it will + // become initialized by `sigemptyset` + unsafe { + sigemptyset(sa_mask.as_mut_ptr()); + sa_mask.assume_init() + } }; raw.sa_flags = 0; raw }; // Reset `GOT_SIGTTOU`. GOT_SIGTTOU.store(false, Ordering::SeqCst); + // Set `action` as the action for `SIGTTOU` and store the original action in `original_action` // to restore it later. - unsafe { sigaction(SIGTTOU, &action, original_action.as_mut_ptr()) }; + // + // SAFETY: `original_action` is a valid pointer; second, the `action` installed (on_sigttou): + // - is itself a safe function + // - only updates an atomic variable, so cannot violate memory unsafety that way + // - doesn't call any async-unsafe functions (refer to signal-safety(7)) + // Therefore it can safely be installed as a signal handler. + // Furthermore, `sigaction` will initialize `original_action`. + let original_action = unsafe { + let mut original_action = MaybeUninit::::uninit(); + sigaction(SIGTTOU, &action, original_action.as_mut_ptr()); + original_action.assume_init() + }; + // Call `tcsetattr` until it suceeds and ignore interruptions if we did not receive `SIGTTOU`. let result = loop { + // SAFETY: is the responsibility of the caller of `tcsetattr_nobg` match cerr(unsafe { tcsetattr(fd, flags, tp) }) { Ok(_) => break Ok(()), Err(err) => { @@ -97,8 +116,13 @@ fn tcsetattr_nobg(fd: c_int, flags: c_int, tp: *const termios) -> io::Result<()> } } }; + // Restore the original action. - unsafe { sigaction(SIGTTOU, original_action.as_ptr(), std::ptr::null_mut()) }; + // + // SAFETY: `original_action` is a valid pointer, and was initialized by the preceding + // call to `sigaction` (and not subsequently altered, since it is not mut). The third parameter + // is allowed to be NULL (this means we ignore the previously-installed handler) + unsafe { sigaction(SIGTTOU, &original_action, std::ptr::null_mut()) }; result } @@ -176,7 +200,7 @@ impl UserTerm { unsafe { cfsetispeed(&mut tt_dst, speed) }; } - tcsetattr_nobg(dst, TCSAFLUSH, &tt_dst)?; + unsafe { tcsetattr_nobg(dst, TCSAFLUSH, &tt_dst) }?; cerr(unsafe { ioctl(src, TIOCGWINSZ, &mut wsize) })?; cerr(unsafe { ioctl(dst, TIOCSWINSZ, &wsize) })?; @@ -201,7 +225,7 @@ impl UserTerm { term.c_cflag |= ISIG; } - tcsetattr_nobg(fd, TCSADRAIN, &term)?; + unsafe { tcsetattr_nobg(fd, TCSADRAIN, &term) }?; self.changed = true; Ok(()) @@ -215,7 +239,7 @@ impl UserTerm { if self.changed { let fd = self.tty.as_raw_fd(); let flags = if flush { TCSAFLUSH } else { TCSADRAIN }; - tcsetattr_nobg(fd, flags, self.original_termios.as_ptr())?; + unsafe { tcsetattr_nobg(fd, flags, self.original_termios.as_ptr()) }?; self.changed = false; } @@ -227,8 +251,6 @@ impl UserTerm { // This function is based around the fact that we receive `SIGTTOU` if we call `tcsetpgrp` and // we are not in the foreground process group. - let mut original_action = MaybeUninit::::uninit(); - let action = { let mut raw: libc::sigaction = make_zeroed_sigaction(); // Call `on_sigttou` if `SIGTTOU` arrives. @@ -236,17 +258,28 @@ impl UserTerm { // Exclude any other signals from the set raw.sa_mask = { let mut sa_mask = MaybeUninit::::uninit(); - unsafe { sigemptyset(sa_mask.as_mut_ptr()) }; - unsafe { sa_mask.assume_init() } + // SAFETY: see tcsetattr_nobg + unsafe { + sigemptyset(sa_mask.as_mut_ptr()); + sa_mask.assume_init() + } }; raw.sa_flags = 0; raw }; + // Reset `GOT_SIGTTOU`. GOT_SIGTTOU.store(false, Ordering::SeqCst); + // Set `action` as the action for `SIGTTOU` and store the original action in `original_action` // to restore it later. - unsafe { sigaction(SIGTTOU, &action, original_action.as_mut_ptr()) }; + // SAFETY: see tcsetattr_nobg + let original_action = unsafe { + let mut original_action = MaybeUninit::::uninit(); + sigaction(SIGTTOU, &action, original_action.as_mut_ptr()); + original_action.assume_init() + }; + // Call `tcsetattr` until it suceeds and ignore interruptions if we did not receive `SIGTTOU`. let result = loop { match self.tty.tcsetpgrp(pgrp) { @@ -259,8 +292,10 @@ impl UserTerm { } } }; + // Restore the original action. - unsafe { sigaction(SIGTTOU, original_action.as_ptr(), std::ptr::null_mut()) }; + // SAFETY: see tcsetattr_nobg + unsafe { sigaction(SIGTTOU, &original_action, std::ptr::null_mut()) }; result } From 5440dff9b8def05a7758505804bc018ca365d258 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Mon, 2 Sep 2024 23:43:21 +0200 Subject: [PATCH 02/10] refactor SIGTTOU-catching code --- src/system/term/user_term.rs | 66 +++++++----------------------------- 1 file changed, 13 insertions(+), 53 deletions(-) diff --git a/src/system/term/user_term.rs b/src/system/term/user_term.rs index ce43a0055..aab4a7f08 100644 --- a/src/system/term/user_term.rs +++ b/src/system/term/user_term.rs @@ -56,10 +56,6 @@ const LOCAL_FLAGS: tcflag_t = ISIG static GOT_SIGTTOU: AtomicBool = AtomicBool::new(false); -extern "C" fn on_sigttou(_signal: c_int, _info: *mut siginfo_t, _: *mut c_void) { - GOT_SIGTTOU.store(true, Ordering::SeqCst); -} - /// This is like `tcsetattr` but it only suceeds if we are in the foreground process group. /// # Safety /// @@ -68,6 +64,17 @@ unsafe fn tcsetattr_nobg(fd: c_int, flags: c_int, tp: *const termios) -> io::Res // This function is based around the fact that we receive `SIGTTOU` if we call `tcsetattr` and // we are not in the foreground process group. + // SAFETY: is the responsibility of the caller of `tcsetattr_nobg` + let setattr = || cerr(unsafe { tcsetattr(fd, flags, tp) }).map(|_| ()); + + catching_sigttou(setattr) +} + +fn catching_sigttou(mut function: impl FnMut() -> io::Result<()>) -> io::Result<()> { + extern "C" fn on_sigttou(_signal: c_int, _info: *mut siginfo_t, _: *mut c_void) { + GOT_SIGTTOU.store(true, Ordering::SeqCst); + } + let action = { let mut raw: libc::sigaction = make_zeroed_sigaction(); // Call `on_sigttou` if `SIGTTOU` arrives. @@ -105,8 +112,7 @@ unsafe fn tcsetattr_nobg(fd: c_int, flags: c_int, tp: *const termios) -> io::Res // Call `tcsetattr` until it suceeds and ignore interruptions if we did not receive `SIGTTOU`. let result = loop { - // SAFETY: is the responsibility of the caller of `tcsetattr_nobg` - match cerr(unsafe { tcsetattr(fd, flags, tp) }) { + match function() { Ok(_) => break Ok(()), Err(err) => { let got_sigttou = GOT_SIGTTOU.load(Ordering::SeqCst); @@ -251,53 +257,7 @@ impl UserTerm { // This function is based around the fact that we receive `SIGTTOU` if we call `tcsetpgrp` and // we are not in the foreground process group. - let action = { - let mut raw: libc::sigaction = make_zeroed_sigaction(); - // Call `on_sigttou` if `SIGTTOU` arrives. - raw.sa_sigaction = on_sigttou as sighandler_t; - // Exclude any other signals from the set - raw.sa_mask = { - let mut sa_mask = MaybeUninit::::uninit(); - // SAFETY: see tcsetattr_nobg - unsafe { - sigemptyset(sa_mask.as_mut_ptr()); - sa_mask.assume_init() - } - }; - raw.sa_flags = 0; - raw - }; - - // Reset `GOT_SIGTTOU`. - GOT_SIGTTOU.store(false, Ordering::SeqCst); - - // Set `action` as the action for `SIGTTOU` and store the original action in `original_action` - // to restore it later. - // SAFETY: see tcsetattr_nobg - let original_action = unsafe { - let mut original_action = MaybeUninit::::uninit(); - sigaction(SIGTTOU, &action, original_action.as_mut_ptr()); - original_action.assume_init() - }; - - // Call `tcsetattr` until it suceeds and ignore interruptions if we did not receive `SIGTTOU`. - let result = loop { - match self.tty.tcsetpgrp(pgrp) { - Ok(()) => break Ok(()), - Err(err) => { - let got_sigttou = GOT_SIGTTOU.load(Ordering::SeqCst); - if got_sigttou || err.kind() != io::ErrorKind::Interrupted { - break Err(err); - } - } - } - }; - - // Restore the original action. - // SAFETY: see tcsetattr_nobg - unsafe { sigaction(SIGTTOU, &original_action, std::ptr::null_mut()) }; - - result + catching_sigttou(|| self.tty.tcsetpgrp(pgrp)) } } From 128b0fe3c42abc29337b150f58ec07ae2f6e0b72 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Tue, 3 Sep 2024 00:12:30 +0200 Subject: [PATCH 03/10] add safety comments and refactor code so the safety comments don't obstruct the code flow --- src/system/term/user_term.rs | 51 ++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/src/system/term/user_term.rs b/src/system/term/user_term.rs index aab4a7f08..8132e2ec7 100644 --- a/src/system/term/user_term.rs +++ b/src/system/term/user_term.rs @@ -153,6 +153,9 @@ impl UserTerm { pub(crate) fn get_size(&self) -> io::Result { let mut term_size = MaybeUninit::::uninit(); + // SAFETY: This passes a valid file descriptor and valid pointer (of + // the correct type) to the TIOCGWINSZ ioctl; see: + // https://man7.org/linux/man-pages/man2/TIOCGWINSZ.2const.html cerr(unsafe { ioctl( self.tty.as_raw_fd(), @@ -161,6 +164,7 @@ impl UserTerm { ) })?; + // SAFETY: if we arrived at this point, `term_size` was initialized. Ok(unsafe { term_size.assume_init() }) } @@ -169,15 +173,16 @@ impl UserTerm { let src = self.tty.as_raw_fd(); let dst = dst.as_raw_fd(); - let mut tt_src = MaybeUninit::::uninit(); - let mut tt_dst = MaybeUninit::::uninit(); - let mut wsize = MaybeUninit::::uninit(); + // SAFETY: tt_src and tt_dst will be initialized by `tcgetattr`. + let (tt_src, mut tt_dst) = unsafe { + let mut tt_src = MaybeUninit::::uninit(); + let mut tt_dst = MaybeUninit::::uninit(); - cerr(unsafe { tcgetattr(src, tt_src.as_mut_ptr()) })?; - cerr(unsafe { tcgetattr(dst, tt_dst.as_mut_ptr()) })?; + cerr(tcgetattr(src, tt_src.as_mut_ptr()))?; + cerr(tcgetattr(dst, tt_dst.as_mut_ptr()))?; - let tt_src = unsafe { tt_src.assume_init() }; - let mut tt_dst = unsafe { tt_dst.assume_init() }; + (tt_src.assume_init(), tt_dst.assume_init()) + }; // Clear select input, output, control and local flags. tt_dst.c_iflag &= !INPUT_FLAGS; @@ -195,21 +200,31 @@ impl UserTerm { tt_dst.c_cc.copy_from_slice(&tt_src.c_cc); // Copy speed from `src`. - { - let mut speed = unsafe { cfgetospeed(&tt_src) }; + // + // SAFETY: the cfXXXXspeed calls are passed valid pointers and + // cannot cause UB even if the speed would be incorrect. + unsafe { + let mut speed = cfgetospeed(&tt_src); // Zero output speed closes the connection. if speed == libc::B0 { speed = libc::B38400; } - unsafe { cfsetospeed(&mut tt_dst, speed) }; - speed = unsafe { cfgetispeed(&tt_src) }; - unsafe { cfsetispeed(&mut tt_dst, speed) }; + cfsetospeed(&mut tt_dst, speed); + + speed = cfgetispeed(&tt_src); + cfsetispeed(&mut tt_dst, speed); } + // SAFETY: dst is a valid file descriptor and `tt_dst` is an + // initialized struct obtained through tcgetattr; so this is safe to + // pass to `tcsetattr`. unsafe { tcsetattr_nobg(dst, TCSAFLUSH, &tt_dst) }?; - cerr(unsafe { ioctl(src, TIOCGWINSZ, &mut wsize) })?; - cerr(unsafe { ioctl(dst, TIOCSWINSZ, &wsize) })?; + let mut wsize = MaybeUninit::::uninit(); + // SAFETY: TIOCGWINSZ ioctl expects one argument of type *mut winsize + cerr(unsafe { ioctl(src, TIOCGWINSZ, wsize.as_mut_ptr()) })?; + // SAFETY: wsize has been initialized by the TIOCGWINSZ ioctl + cerr(unsafe { ioctl(dst, TIOCSWINSZ, wsize.as_ptr()) })?; Ok(()) } @@ -219,18 +234,24 @@ impl UserTerm { pub fn set_raw_mode(&mut self, with_signals: bool) -> io::Result<()> { let fd = self.tty.as_raw_fd(); + // Retrieve the original terminal (if we haven't done so already) if !self.changed { + // SAFETY: `termios` is a valid pointer to pass to tcgetattr cerr(unsafe { tcgetattr(fd, self.original_termios.as_mut_ptr()) })?; } // Retrieve the original terminal. + // SAFETY: tcgetattr will have initialized `termios` let mut term = unsafe { self.original_termios.assume_init() }; // Set terminal to raw mode. + // SAFETY: `term` is a valid, initialized pointer to ` struct of type `termios`, which + // was previously obtained through `tcgetattr`. unsafe { cfmakeraw(&mut term) }; // Enable terminal signals. if with_signals { term.c_cflag |= ISIG; } + // SAFETY: `fd` is a valid file descriptor for the tty; for `term`: same as above. unsafe { tcsetattr_nobg(fd, TCSADRAIN, &term) }?; self.changed = true; @@ -245,6 +266,8 @@ impl UserTerm { if self.changed { let fd = self.tty.as_raw_fd(); let flags = if flush { TCSAFLUSH } else { TCSADRAIN }; + // SAFETY: `fd` is a valid file descriptor for the tty; and `termios` is a valid pointer + // that was obtained through `tcgetattr`. unsafe { tcsetattr_nobg(fd, flags, self.original_termios.as_ptr()) }?; self.changed = false; } From d9bae735f6dbe8f7ad83d46a33f1fccffb498529 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Tue, 3 Sep 2024 00:26:01 +0200 Subject: [PATCH 04/10] add safety to comments to term/mod --- src/system/term/mod.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/system/term/mod.rs b/src/system/term/mod.rs index 951f73c27..f5566883f 100644 --- a/src/system/term/mod.rs +++ b/src/system/term/mod.rs @@ -34,6 +34,11 @@ impl Pty { // Create two integers to hold the file descriptors for each side of the pty. let (mut leader, mut follower) = (0, 0); + // SAFETY: + // - openpty is passed two valid pointers as its first two arguments + // - path is a valid array that can hold PATH_MAX characters; and casting `u8` to `i8` is + // valid since all values are initialized to zero. + // - the last two arguments are allowed to be NULL cerr(unsafe { libc::openpty( &mut leader, @@ -60,9 +65,11 @@ impl Pty { Ok(Self { path, leader: PtyLeader { + // SAFETY: `leader` has been set by `openpty` file: unsafe { OwnedFd::from_raw_fd(leader) }.into(), }, follower: PtyFollower { + // SAFETY: `follower` has been set by `openpty` file: unsafe { OwnedFd::from_raw_fd(follower) }.into(), }, }) @@ -75,6 +82,11 @@ pub(crate) struct PtyLeader { impl PtyLeader { pub(crate) fn set_size(&self, term_size: &TermSize) -> io::Result<()> { + // SAFETY: the TIOCSWINSZ expects an initialized pointer of type `winsize` + // https://www.man7.org/linux/man-pages/man2/TIOCSWINSZ.2const.html + // + // An object of type TermSize is safe to cast to `winsize` since it is a + // repr(transparent) "newtype" struct. cerr(unsafe { ioctl( self.file.as_raw_fd(), @@ -151,15 +163,19 @@ pub(crate) trait Terminal: sealed::Sealed { impl Terminal for F { /// Get the foreground process group ID associated with this terminal. fn tcgetpgrp(&self) -> io::Result { + // SAFETY: tcgetpgrp cannot cause UB cerr(unsafe { libc::tcgetpgrp(self.as_raw_fd()) }) } /// Set the foreground process group ID associated with this terminalto `pgrp`. fn tcsetpgrp(&self, pgrp: ProcessId) -> io::Result<()> { + // SAFETY: tcsetpgrp cannot cause UB cerr(unsafe { libc::tcsetpgrp(self.as_raw_fd(), pgrp) }).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) })?; Ok(()) } @@ -172,7 +188,9 @@ impl Terminal for F { 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() as _, 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()) }) } @@ -182,6 +200,7 @@ impl Terminal for F { } fn tcgetsid(&self) -> io::Result { + // SAFETY: tcgetsid cannot cause UB cerr(unsafe { libc::tcgetsid(self.as_raw_fd()) }) } } From f678e84679f157cf8f67daffa9dd4f0f04de5fa8 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Tue, 3 Sep 2024 00:47:44 +0200 Subject: [PATCH 05/10] add safety comments to signal code --- src/system/signal/info.rs | 5 +++++ src/system/signal/set.rs | 9 +++++++++ src/system/signal/stream.rs | 5 +++++ 3 files changed, 19 insertions(+) diff --git a/src/system/signal/info.rs b/src/system/signal/info.rs index 31c31dbd2..667762638 100644 --- a/src/system/signal/info.rs +++ b/src/system/signal/info.rs @@ -21,6 +21,11 @@ impl SignalInfo { /// Gets the PID that sent the signal. pub(crate) fn pid(&self) -> ProcessId { // FIXME: some signals don't set si_pid. + // + // SAFETY: this just fetches the `si_pid` field; since this is an integer, + // even if the information is nonsense it will not cause UB. Note that + // that a `ProcessId` does not have as type invariant that it always holds a valid + // process id, only that it is the appropriate type for storing such ids. unsafe { self.info.si_pid() } } diff --git a/src/system/signal/set.rs b/src/system/signal/set.rs index c566b2c7c..3356bc73d 100644 --- a/src/system/signal/set.rs +++ b/src/system/signal/set.rs @@ -41,8 +41,11 @@ impl SignalAction { pub(super) fn register(&self, signal: SignalNumber) -> io::Result { let mut original_action = MaybeUninit::::zeroed(); + // SAFETY: `sigaction` expects a valid pointer, which we provide; the typecast is valid + // since SignalAction is a repr(transparent) newtype struct. cerr(unsafe { libc::sigaction(signal, &self.raw, original_action.as_mut_ptr().cast()) })?; + // SAFETY: `sigaction` will have properly initialized `original_action`. Ok(unsafe { original_action.assume_init() }) } } @@ -58,8 +61,10 @@ impl SignalSet { pub(crate) fn empty() -> io::Result { let mut set = MaybeUninit::::zeroed(); + // SAFETY: same as above cerr(unsafe { libc::sigemptyset(set.as_mut_ptr().cast()) })?; + // SAFETY: `sigemptyset` will have initialized `set` Ok(unsafe { set.assume_init() }) } @@ -67,16 +72,20 @@ impl SignalSet { pub(crate) fn full() -> io::Result { let mut set = MaybeUninit::::zeroed(); + // SAFETY: same as above cerr(unsafe { libc::sigfillset(set.as_mut_ptr().cast()) })?; + // SAFETY: `sigfillset` will have initialized `set` Ok(unsafe { set.assume_init() }) } fn sigprocmask(&self, how: libc::c_int) -> io::Result { let mut original_set = MaybeUninit::::zeroed(); + // SAFETY: same as above cerr(unsafe { libc::sigprocmask(how, &self.raw, original_set.as_mut_ptr().cast()) })?; + // SAFETY: `sigprocmask` will have initialized `set` Ok(unsafe { original_set.assume_init() }) } diff --git a/src/system/signal/stream.rs b/src/system/signal/stream.rs index f9236f3a6..abaf4e9a6 100644 --- a/src/system/signal/stream.rs +++ b/src/system/signal/stream.rs @@ -67,6 +67,9 @@ impl SignalStream { pub(crate) fn recv(&self) -> io::Result { let mut info = MaybeUninit::::uninit(); let fd = self.rx.as_raw_fd(); + // SAFETY: type invariant for `SignalStream` ensures that `fd` is a valid file descriptor; + // furthermore, `info` is a valid pointer to `siginfo_t` (by virtue of `SignalInfo` being a + // transparent newtype for it), which has room for `SignalInfo::SIZE` bytes. let bytes = cerr(unsafe { libc::recv(fd, info.as_mut_ptr().cast(), SignalInfo::SIZE, 0) })?; if bytes as usize != SignalInfo::SIZE { @@ -97,6 +100,8 @@ pub(crate) fn register_handlers( })?; } + // SAFETY: if the above for-loop has terminated, every handler will have + // been written to via "MaybeUnit::new", and so is initialized. Ok(handlers.map(|(_, handler)| unsafe { handler.assume_init() })) } From 119eb03f7e30b25e6b2fee561451feb55f3e64c5 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Tue, 3 Sep 2024 16:57:45 +0200 Subject: [PATCH 06/10] rewrite MaybeUnInit+bool -> Option --- src/system/term/user_term.rs | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/system/term/user_term.rs b/src/system/term/user_term.rs index 8132e2ec7..bbd7af633 100644 --- a/src/system/term/user_term.rs +++ b/src/system/term/user_term.rs @@ -136,8 +136,7 @@ fn catching_sigttou(mut function: impl FnMut() -> io::Result<()>) -> io::Result< /// Type to manipulate the settings of the user's terminal. pub struct UserTerm { tty: File, - original_termios: MaybeUninit, - changed: bool, + original_termios: Option, } impl UserTerm { @@ -145,8 +144,7 @@ impl UserTerm { pub fn open() -> io::Result { Ok(Self { tty: OpenOptions::new().read(true).write(true).open("/dev/tty")?, - original_termios: MaybeUninit::uninit(), - changed: false, + original_termios: None, }) } @@ -235,13 +233,18 @@ impl UserTerm { let fd = self.tty.as_raw_fd(); // Retrieve the original terminal (if we haven't done so already) - if !self.changed { - // SAFETY: `termios` is a valid pointer to pass to tcgetattr - cerr(unsafe { tcgetattr(fd, self.original_termios.as_mut_ptr()) })?; - } - // Retrieve the original terminal. - // SAFETY: tcgetattr will have initialized `termios` - let mut term = unsafe { self.original_termios.assume_init() }; + let mut term = if let Some(termios) = self.original_termios { + termios + } else { + // SAFETY: `termios` is a valid pointer to pass to tcgetattr; if that calls succeeds, + // it will have initialized the `termios` strucutre + *self.original_termios.insert(unsafe { + let mut termios = MaybeUninit::uninit(); + cerr(tcgetattr(fd, termios.as_mut_ptr()))?; + termios.assume_init() + }) + }; + // Set terminal to raw mode. // SAFETY: `term` is a valid, initialized pointer to ` struct of type `termios`, which // was previously obtained through `tcgetattr`. @@ -253,7 +256,6 @@ impl UserTerm { // SAFETY: `fd` is a valid file descriptor for the tty; for `term`: same as above. unsafe { tcsetattr_nobg(fd, TCSADRAIN, &term) }?; - self.changed = true; Ok(()) } @@ -263,13 +265,12 @@ impl UserTerm { /// This change is done after waiting for all the queued output to be written. To discard the /// queued input `flush` must be set to `true`. pub fn restore(&mut self, flush: bool) -> io::Result<()> { - if self.changed { + if let Some(termios) = self.original_termios.take() { let fd = self.tty.as_raw_fd(); let flags = if flush { TCSAFLUSH } else { TCSADRAIN }; // SAFETY: `fd` is a valid file descriptor for the tty; and `termios` is a valid pointer // that was obtained through `tcgetattr`. - unsafe { tcsetattr_nobg(fd, flags, self.original_termios.as_ptr()) }?; - self.changed = false; + unsafe { tcsetattr_nobg(fd, flags, &termios) }?; } Ok(()) From 2251961e1d0cbb056853642483a872c1498d3d5b Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Mon, 7 Oct 2024 17:33:02 +0200 Subject: [PATCH 07/10] incorporate typos spotted by @pvdrz --- src/system/term/user_term.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/system/term/user_term.rs b/src/system/term/user_term.rs index bbd7af633..877878461 100644 --- a/src/system/term/user_term.rs +++ b/src/system/term/user_term.rs @@ -237,7 +237,7 @@ impl UserTerm { termios } else { // SAFETY: `termios` is a valid pointer to pass to tcgetattr; if that calls succeeds, - // it will have initialized the `termios` strucutre + // it will have initialized the `termios` structure *self.original_termios.insert(unsafe { let mut termios = MaybeUninit::uninit(); cerr(tcgetattr(fd, termios.as_mut_ptr()))?; @@ -246,7 +246,7 @@ impl UserTerm { }; // Set terminal to raw mode. - // SAFETY: `term` is a valid, initialized pointer to ` struct of type `termios`, which + // SAFETY: `term` is a valid, initialized pointer to struct of type `termios`, which // was previously obtained through `tcgetattr`. unsafe { cfmakeraw(&mut term) }; // Enable terminal signals. From b4dd7419f279d3d799126c67ed43972218fda34d Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Tue, 8 Oct 2024 17:43:59 +0200 Subject: [PATCH 08/10] Ensure the si_pid field is only accessed when initialized The si_pid field would be logged even when it is uninitialized. It also obvious that it isn't possible to trick sudo into thinking that the signal it received was from itself due to the uninitialized value it's own pid by chance. This is not possible as the check is prefixed by is_user_signaled and all signals where is_user_signaled are true have si_pid initialized. Using an option ensures that this check is never forgotten. --- src/exec/no_pty.rs | 17 +++++++--------- src/exec/use_pty/monitor.rs | 21 +++++++++---------- src/exec/use_pty/parent.rs | 24 ++++++++++++---------- src/system/signal/info.rs | 40 +++++++++++++++++++++++++++---------- 4 files changed, 61 insertions(+), 41 deletions(-) diff --git a/src/exec/no_pty.rs b/src/exec/no_pty.rs index 93175b7cc..b7c8e2019 100644 --- a/src/exec/no_pty.rs +++ b/src/exec/no_pty.rs @@ -14,7 +14,7 @@ use crate::{ }, }; use crate::{ - exec::{handle_sigchld, opt_fmt, signal_fmt}, + exec::{handle_sigchld, signal_fmt}, log::{dev_error, dev_info, dev_warn}, system::{ fork, getpgid, getpgrp, @@ -238,12 +238,7 @@ impl ExecClosure { } }; - dev_info!( - "received{} {} from {}", - opt_fmt(info.is_user_signaled(), " user signaled"), - info.signal(), - info.pid() - ); + dev_info!("received{}", info); let Some(command_pid) = self.command_pid else { dev_info!("command was terminated, ignoring signal"); @@ -256,9 +251,11 @@ impl ExecClosure { // FIXME: we should handle SIGWINCH here if we want to support I/O plugins that // react on window change events. - // Skip the signal if it was sent by the user and it is self-terminating. - if info.is_user_signaled() && self.is_self_terminating(info.pid()) { - return; + if let Some(pid) = info.signaler_pid() { + if self.is_self_terminating(pid) { + // Skip the signal if it was sent by the user and it is self-terminating. + return; + } } if signal == SIGALRM { diff --git a/src/exec/use_pty/monitor.rs b/src/exec/use_pty/monitor.rs index bb321db43..20e06226c 100644 --- a/src/exec/use_pty/monitor.rs +++ b/src/exec/use_pty/monitor.rs @@ -381,12 +381,7 @@ impl<'a> MonitorClosure<'a> { } }; - dev_info!( - "monitor received{} {} from {}", - opt_fmt(info.is_user_signaled(), " user signaled"), - info.signal(), - info.pid() - ); + dev_info!("monitor received{}", info); // Don't do anything if the command has terminated already let Some(command_pid) = self.command_pid else { @@ -396,10 +391,16 @@ impl<'a> MonitorClosure<'a> { match info.signal() { SIGCHLD => handle_sigchld(self, registry, "command", command_pid), - // Skip the signal if it was sent by the user and it is self-terminating. - _ if info.is_user_signaled() - && is_self_terminating(info.pid(), command_pid, self.command_pgrp) => {} - signal => self.send_signal(signal, command_pid, false), + signal => { + if let Some(pid) = info.signaler_pid() { + if is_self_terminating(pid, command_pid, self.command_pgrp) { + // Skip the signal if it was sent by the user and it is self-terminating. + return; + } + } + + self.send_signal(signal, command_pid, false) + } } } } diff --git a/src/exec/use_pty/parent.rs b/src/exec/use_pty/parent.rs index 37e530df6..aed7b9dbb 100644 --- a/src/exec/use_pty/parent.rs +++ b/src/exec/use_pty/parent.rs @@ -7,7 +7,7 @@ use crate::exec::event::{EventHandle, EventRegistry, PollEvent, Process, StopRea use crate::exec::use_pty::monitor::exec_monitor; use crate::exec::use_pty::SIGCONT_FG; use crate::exec::{ - cond_fmt, handle_sigchld, opt_fmt, signal_fmt, terminate_process, ExecOutput, HandleSigchld, + cond_fmt, handle_sigchld, signal_fmt, terminate_process, ExecOutput, HandleSigchld, ProcessOutput, }; use crate::exec::{ @@ -617,12 +617,7 @@ impl ParentClosure { } }; - dev_info!( - "parent received{} {} from {}", - opt_fmt(info.is_user_signaled(), " user signaled"), - info.signal(), - info.pid() - ); + dev_info!("parent received{}", info); let Some(monitor_pid) = self.monitor_pid else { dev_info!("monitor was terminated, ignoring signal"); @@ -639,10 +634,17 @@ impl ParentClosure { dev_warn!("cannot resize terminal: {}", err); } } - // Skip the signal if it was sent by the user and it is self-terminating. - _ if info.is_user_signaled() && self.is_self_terminating(info.pid()) => {} - // FIXME: check `send_command_status` - signal => self.schedule_signal(signal, registry), + signal => { + if let Some(pid) = info.signaler_pid() { + if self.is_self_terminating(pid) { + // Skip the signal if it was sent by the user and it is self-terminating. + return; + } + } + + // FIXME: check `send_command_status` + self.schedule_signal(signal, registry) + } } } diff --git a/src/system/signal/info.rs b/src/system/signal/info.rs index 667762638..26c3a937c 100644 --- a/src/system/signal/info.rs +++ b/src/system/signal/info.rs @@ -1,3 +1,5 @@ +use std::fmt; + use crate::system::interface::ProcessId; use super::SignalNumber; @@ -13,20 +15,18 @@ impl SignalInfo { /// Returns whether the signal was sent by the user or not. pub(crate) fn is_user_signaled(&self) -> bool { - // FIXME: we should check if si_code is equal to SI_USER but for some reason the latter it - // is not available in libc. + // This matches the definition of the SI_FROMUSER macro. self.info.si_code <= 0 } /// Gets the PID that sent the signal. - pub(crate) fn pid(&self) -> ProcessId { - // FIXME: some signals don't set si_pid. - // - // SAFETY: this just fetches the `si_pid` field; since this is an integer, - // even if the information is nonsense it will not cause UB. Note that - // that a `ProcessId` does not have as type invariant that it always holds a valid - // process id, only that it is the appropriate type for storing such ids. - unsafe { self.info.si_pid() } + pub(crate) fn signaler_pid(&self) -> Option { + if self.is_user_signaled() { + // SAFETY: si_pid is always initialized if the signal is user signaled. + unsafe { Some(self.info.si_pid()) } + } else { + None + } } /// Gets the signal number. @@ -34,3 +34,23 @@ impl SignalInfo { self.info.si_signo } } + +impl fmt::Display for SignalInfo { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{} {} from ", + if self.is_user_signaled() { + " user signaled" + } else { + "" + }, + self.signal(), + )?; + if let Some(pid) = self.signaler_pid() { + write!(f, "{pid}") + } else { + write!(f, "") + } + } +} From 671f181820714fbea003eac215211d5568b52b6e Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Wed, 9 Oct 2024 09:18:09 +0200 Subject: [PATCH 09/10] Make is_user_signaled a private function --- src/system/signal/info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/system/signal/info.rs b/src/system/signal/info.rs index 26c3a937c..bc6d44793 100644 --- a/src/system/signal/info.rs +++ b/src/system/signal/info.rs @@ -14,7 +14,7 @@ impl SignalInfo { pub(super) const SIZE: usize = std::mem::size_of::(); /// Returns whether the signal was sent by the user or not. - pub(crate) fn is_user_signaled(&self) -> bool { + fn is_user_signaled(&self) -> bool { // This matches the definition of the SI_FROMUSER macro. self.info.si_code <= 0 } From 6f25af5693da624ba281720974df7115430d9ad7 Mon Sep 17 00:00:00 2001 From: "Marc R. Schoolderman" Date: Wed, 9 Oct 2024 15:13:15 +0200 Subject: [PATCH 10/10] apply suggestions from code review Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com> --- src/system/term/mod.rs | 8 ++++---- src/system/term/user_term.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/system/term/mod.rs b/src/system/term/mod.rs index f5566883f..ae0bba5d7 100644 --- a/src/system/term/mod.rs +++ b/src/system/term/mod.rs @@ -65,11 +65,11 @@ impl Pty { Ok(Self { path, leader: PtyLeader { - // SAFETY: `leader` has been set by `openpty` + // SAFETY: `openpty` has set `leader` to an open fd suitable for assuming ownership by `OwnedFd`. file: unsafe { OwnedFd::from_raw_fd(leader) }.into(), }, follower: PtyFollower { - // SAFETY: `follower` has been set by `openpty` + // SAFETY: `openpty` has set `follower` to an open fd suitable for assuming ownership by `OwnedFd`. file: unsafe { OwnedFd::from_raw_fd(follower) }.into(), }, }) @@ -166,7 +166,7 @@ impl Terminal for F { // SAFETY: tcgetpgrp cannot cause UB cerr(unsafe { libc::tcgetpgrp(self.as_raw_fd()) }) } - /// Set the foreground process group ID associated with this terminalto `pgrp`. + /// 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) }).map(|_| ()) @@ -189,7 +189,7 @@ impl Terminal for F { } // 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() as _, buf.len()) })?; + cerr(unsafe { libc::ttyname_r(self.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()) }) } diff --git a/src/system/term/user_term.rs b/src/system/term/user_term.rs index 877878461..86758c790 100644 --- a/src/system/term/user_term.rs +++ b/src/system/term/user_term.rs @@ -56,7 +56,7 @@ const LOCAL_FLAGS: tcflag_t = ISIG static GOT_SIGTTOU: AtomicBool = AtomicBool::new(false); -/// This is like `tcsetattr` but it only suceeds if we are in the foreground process group. +/// This is like `tcsetattr` but it only succeeds if we are in the foreground process group. /// # Safety /// /// The arguments to this function have to be valid arguments to `tcsetattr`. @@ -246,7 +246,7 @@ impl UserTerm { }; // Set terminal to raw mode. - // SAFETY: `term` is a valid, initialized pointer to struct of type `termios`, which + // SAFETY: `term` is a valid, initialized struct of type `termios`, which // was previously obtained through `tcgetattr`. unsafe { cfmakeraw(&mut term) }; // Enable terminal signals.