Safety comments: PR2 (#864)

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 {

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)
+            }
         }
     }
 }

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)
+            }
         }
     }
 

src/system/signal/info.rs

@@ -1,3 +1,5 @@
+use std::fmt;
+
 use crate::system::interface::ProcessId;
 
 use super::SignalNumber;
@@ -12,20 +14,43 @@ impl SignalInfo {
     pub(super) const SIZE: usize = std::mem::size_of::<Self>();
 
     /// 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.
+    fn is_user_signaled(&self) -> bool {
+        // 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.
-        unsafe { self.info.si_pid() }
+    pub(crate) fn signaler_pid(&self) -> Option<ProcessId> {
+        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.
     pub(crate) fn signal(&self) -> SignalNumber {
         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, "<none>")
+        }
+    }
+}

src/system/signal/set.rs

@@ -41,8 +41,11 @@ impl SignalAction {
     pub(super) fn register(&self, signal: SignalNumber) -> io::Result<Self> {
         let mut original_action = MaybeUninit::<Self>::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,25 +61,31 @@ impl SignalSet {
     pub(crate) fn empty() -> io::Result<Self> {
         let mut set = MaybeUninit::<Self>::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() })
     }
 
     /// Create a set containing all the signals.
     pub(crate) fn full() -> io::Result<Self> {
         let mut set = MaybeUninit::<Self>::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<Self> {
         let mut original_set = MaybeUninit::<Self>::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() })
     }
 

src/system/signal/stream.rs

@@ -67,6 +67,9 @@ impl SignalStream {
     pub(crate) fn recv(&self) -> io::Result<SignalInfo> {
         let mut info = MaybeUninit::<SignalInfo>::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<const N: usize>(
             })?;
     }
 
+    // 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() }))
 }
 

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: `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: `openpty` has set `follower` to an open fd suitable for assuming ownership by `OwnedFd`.
                 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<F: AsRawFd> Terminal for F {
     /// Get the foreground process group ID associated with this terminal.
     fn tcgetpgrp(&self) -> io::Result<ProcessId> {
+        // 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(|_| ())
     }
 
     /// 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<F: AsRawFd> Terminal for F {
             return Err(io::ErrorKind::Unsupported.into());
         }
 
-        cerr(unsafe { libc::ttyname_r(self.as_raw_fd(), buf.as_mut_ptr() as _, buf.len()) })?;
+        // 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()) })?;
+        // 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<F: AsRawFd> Terminal for F {
     }
 
     fn tcgetsid(&self) -> io::Result<ProcessId> {
+        // SAFETY: tcgetsid cannot cause UB
         cerr(unsafe { libc::tcgetsid(self.as_raw_fd()) })
     }
 }