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

@@ -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,
@@ -237,12 +237,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");
@@ -255,9 +250,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

@@ -380,12 +380,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 {
@@ -395,10 +390,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::{
@@ -616,12 +616,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");
@@ -638,10 +633,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;
@@ -13,24 +15,42 @@ 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<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>")
+        }
+    }
+}