Remove untracked signal modification functions (#1423)

Author: bjorn3

src/exec/mod.rs

@@ -206,14 +206,12 @@ pub enum ExitReason {
 fn exec_command(
     mut command: Command,
     original_set: Option<SignalSet>,
-    original_signal: Option<SignalsState>,
+    mut original_signal: SignalsState,
     mut errpipe_tx: BinPipe<i32>,
 ) -> ! {
     // Restore the signal handlers of modified signals
-    if let Some(mut original_signal) = original_signal {
-        if let Err(err) = original_signal.restore() {
-            dev_warn!("cannot restore signal states: {err}");
-        }
+    if let Err(err) = original_signal.restore() {
+        dev_warn!("cannot restore signal states: {err}");
     }
 
     // Restore the signal mask now that the handlers have been setup.

src/exec/no_pty.rs

@@ -33,13 +33,7 @@ pub(super) fn exec_no_pty(
 ) -> io::Result<ExitReason> {
     // FIXME (ogsudo): Initialize the policy plugin's session here.
 
-    let mut original_signals = match SignalsState::save() {
-        Ok(original_signals) => Some(original_signals),
-        Err(err) => {
-            dev_warn!("cannot save state original signals: {err}");
-            None
-        }
-    };
+    let original_signals = SignalsState::save()?;
 
     // Block all the signals until we are done setting up the signal handlers so we don't miss
     // SIGCHLD.
@@ -78,7 +72,7 @@ pub(super) fn exec_no_pty(
         sudo_pid,
         errpipe_rx,
         &mut registry,
-        &mut original_signals,
+        original_signals,
     )?;
 
     // Restore the signal mask now that the handlers have been setup.
@@ -104,6 +98,7 @@ struct ExecClosure {
     sudo_pid: ProcessId,
     parent_pgrp: ProcessId,
     errpipe_rx: BinPipe<i32>,
+    original_signals: SignalsState,
     signal_stream: &'static SignalStream,
     signal_handlers: [SignalHandler; ExecClosure::SIGNALS.len()],
 }
@@ -119,21 +114,22 @@ impl ExecClosure {
         sudo_pid: ProcessId,
         errpipe_rx: BinPipe<i32>,
         registry: &mut EventRegistry<Self>,
-        original_signals: &mut Option<SignalsState>,
+        mut original_signals: SignalsState,
     ) -> io::Result<Self> {
         registry.register_event(&errpipe_rx, PollEvent::Readable, |_| ExecEvent::ErrPipe);
 
         let signal_stream = SignalStream::init()?;
 
         registry.register_event(signal_stream, PollEvent::Readable, |_| ExecEvent::Signal);
 
-        let signal_handlers = register_handlers(Self::SIGNALS, original_signals)?;
+        let signal_handlers = register_handlers(Self::SIGNALS, &mut original_signals)?;
 
         Ok(Self {
             command_pid: Some(command_pid),
             errpipe_rx,
             sudo_pid,
             parent_pgrp: getpgrp(),
+            original_signals,
             signal_stream,
             signal_handlers,
         })
@@ -161,7 +157,7 @@ impl ExecClosure {
     }
 
     /// Suspend the main process.
-    fn suspend_parent(&self, signal: SignalNumber) {
+    fn suspend_parent(&mut self, signal: SignalNumber) {
         let mut opt_tty = UserTerm::open().ok();
         let mut opt_pgrp = None;
 
@@ -199,9 +195,13 @@ impl ExecClosure {
         }
 
         let sigtstp_handler = if signal == SIGTSTP {
-            SignalHandler::register_untracked(signal, SignalHandlerBehavior::Default)
-                .map_err(|err| dev_warn!("cannot set handler for {}: {err}", signal_fmt(signal)))
-                .ok()
+            SignalHandler::register(
+                signal,
+                SignalHandlerBehavior::Default,
+                &mut self.original_signals,
+            )
+            .map_err(|err| dev_warn!("cannot set handler for {}: {err}", signal_fmt(signal)))
+            .ok()
         } else {
             None
         };

src/exec/use_pty/monitor.rs

@@ -41,7 +41,7 @@ pub(super) fn exec_monitor(
     foreground: bool,
     backchannel: &mut MonitorBackchannel,
     original_set: Option<SignalSet>,
-    mut original_signals: Option<SignalsState>,
+    mut original_signals: SignalsState,
 ) -> io::Result<Infallible> {
     // SIGTTIN and SIGTTOU are ignored here but the docs state that it shouldn't
     // be possible to receive them in the first place. Investigate
@@ -235,7 +235,7 @@ impl<'a> MonitorClosure<'a> {
         errpipe_rx: BinPipe<i32>,
         backchannel: &'a mut MonitorBackchannel,
         registry: &mut EventRegistry<Self>,
-        original_signals: &mut Option<SignalsState>,
+        original_signals: &mut SignalsState,
     ) -> io::Result<Self> {
         // Store the pgid of the monitor.
         let monitor_pgrp = getpgrp();

src/exec/use_pty/parent.rs

@@ -35,13 +35,7 @@ pub(in crate::exec) fn exec_pty(
     // Allocate a pseudoterminal.
     let pty = get_pty(pty_owner)?;
 
-    let mut original_signals = match SignalsState::save() {
-        Ok(original_signals) => Some(original_signals),
-        Err(err) => {
-            dev_warn!("cannot save state original signals: {err}");
-            None
-        }
-    };
+    let mut original_signals = SignalsState::save()?;
 
     // Create backchannels to communicate with the monitor.
     let mut backchannels = BackchannelPair::new().map_err(|err| {
@@ -256,7 +250,7 @@ pub(in crate::exec) fn exec_pty(
         term_raw,
         preserve_oflag,
         &mut registry,
-        &mut original_signals,
+        original_signals,
     )?;
 
     // Restore the signal mask now that the handlers have been setup.
@@ -327,6 +321,7 @@ struct ParentClosure {
     backchannel: ParentBackchannel,
     message_queue: VecDeque<MonitorMessage>,
     backchannel_write_handle: EventHandle,
+    original_signals: SignalsState,
     signal_stream: &'static SignalStream,
     signal_handlers: [SignalHandler; ParentClosure::SIGNALS.len()],
 }
@@ -349,7 +344,7 @@ impl ParentClosure {
         term_raw: bool,
         preserve_oflag: bool,
         registry: &mut EventRegistry<Self>,
-        original_signals: &mut Option<SignalsState>,
+        mut original_signals: SignalsState,
     ) -> io::Result<Self> {
         // Enable nonblocking assertions as we will poll this inside the event loop.
         backchannel.set_nonblocking_asserts(true);
@@ -365,7 +360,7 @@ impl ParentClosure {
 
         registry.register_event(signal_stream, PollEvent::Readable, |_| ParentEvent::Signal);
 
-        let signal_handlers = register_handlers(Self::SIGNALS, original_signals)?;
+        let signal_handlers = register_handlers(Self::SIGNALS, &mut original_signals)?;
 
         Ok(Self {
             monitor_pid: Some(monitor_pid),
@@ -380,6 +375,7 @@ impl ParentClosure {
             backchannel,
             message_queue: VecDeque::new(),
             backchannel_write_handle,
+            original_signals,
             signal_stream,
             signal_handlers,
         })
@@ -540,10 +536,13 @@ impl ParentClosure {
         registry: &mut EventRegistry<Self>,
     ) -> Option<SignalNumber> {
         // Ignore `SIGCONT` while suspending to avoid resuming the terminal twice.
-        let sigcont_handler =
-            SignalHandler::register_untracked(SIGCONT, SignalHandlerBehavior::Ignore)
-                .map_err(|err| dev_warn!("cannot set handler for SIGCONT: {err}"))
-                .ok();
+        let sigcont_handler = SignalHandler::register(
+            SIGCONT,
+            SignalHandlerBehavior::Ignore,
+            &mut self.original_signals,
+        )
+        .map_err(|err| dev_warn!("cannot set handler for SIGCONT: {err}"))
+        .ok();
 
         if let SIGTTOU | SIGTTIN = signal {
             // If sudo is already the foreground process we can resume the command in the
@@ -584,9 +583,13 @@ impl ParentClosure {
         }
 
         let signal_handler = if signal != SIGSTOP {
-            SignalHandler::register_untracked(signal, SignalHandlerBehavior::Default)
-                .map_err(|err| dev_warn!("cannot set handler for {}: {err}", signal_fmt(signal)))
-                .ok()
+            SignalHandler::register(
+                signal,
+                SignalHandlerBehavior::Default,
+                &mut self.original_signals,
+            )
+            .map_err(|err| dev_warn!("cannot set handler for {}: {err}", signal_fmt(signal)))
+            .ok()
         } else {
             None
         };

src/system/signal/handler.rs

@@ -16,14 +16,6 @@ pub(crate) struct SignalHandler {
 impl SignalHandler {
     const FORBIDDEN: &'static [SignalNumber] = &[SIGKILL, SIGSTOP];
 
-    #[inline]
-    pub(crate) fn register_untracked(
-        signal: SignalNumber,
-        behavior: SignalHandlerBehavior,
-    ) -> io::Result<Self> {
-        Self::register(signal, behavior, &mut None)
-    }
-
     /// Register a new handler for the given signal with the provided behavior.
     ///
     /// # Panics
@@ -35,7 +27,7 @@ impl SignalHandler {
     pub(crate) fn register(
         signal: SignalNumber,
         behavior: SignalHandlerBehavior,
-        state: &mut Option<SignalsState>,
+        state: &mut SignalsState,
     ) -> io::Result<Self> {
         if Self::FORBIDDEN.contains(&signal) {
             panic!(
@@ -46,9 +38,7 @@ impl SignalHandler {
 
         let action = SignalAction::new(behavior)?;
         let original_action = action.register(signal)?;
-        if let Some(state) = state {
-            state.updated(signal)?;
-        }
+        state.updated(signal)?;
 
         Ok(Self {
             signal,

src/system/signal/mod.rs

@@ -8,7 +8,7 @@ mod stream;
 pub(crate) use handler::{SignalHandler, SignalHandlerBehavior};
 pub(crate) use set::SignalSet;
 pub(crate) use state::SignalsState;
-pub(crate) use stream::{register_handlers, register_handlers_untracked, SignalStream};
+pub(crate) use stream::{register_handlers, SignalStream};
 
 use std::borrow::Cow;
 use std::ffi::c_int;

src/system/signal/stream.rs

@@ -85,17 +85,10 @@ impl SignalStream {
     }
 }
 
-#[inline]
-pub(crate) fn register_handlers_untracked<const N: usize>(
-    signals: [SignalNumber; N],
-) -> io::Result<[SignalHandler; N]> {
-    register_handlers(signals, &mut None)
-}
-
 #[track_caller]
 pub(crate) fn register_handlers<const N: usize>(
     signals: [SignalNumber; N],
-    original_signals: &mut Option<SignalsState>,
+    original_signals: &mut SignalsState,
 ) -> io::Result<[SignalHandler; N]> {
     let mut handlers = signals.map(|signal| (signal, MaybeUninit::uninit()));
 

src/visudo/mod.rs

@@ -20,7 +20,7 @@ use crate::{
     system::{
         file::{create_temporary_dir, Chown, FileLock},
         interface::{GroupId, UserId},
-        signal::{consts::*, register_handlers_untracked, SignalStream},
+        signal::{consts::*, register_handlers, SignalStream, SignalsState},
         Hostname, User,
     },
 };
@@ -197,7 +197,10 @@ fn run(file_arg: Option<&str>, perms: bool, owner: bool) -> io::Result<()> {
 
     let signal_stream = SignalStream::init()?;
 
-    let handlers = register_handlers_untracked([SIGTERM, SIGHUP, SIGINT, SIGQUIT])?;
+    let handlers = register_handlers(
+        [SIGTERM, SIGHUP, SIGINT, SIGQUIT],
+        &mut SignalsState::save()?,
+    )?;
 
     let tmp_dir = create_temporary_dir()?;
     let tmp_path = tmp_dir.join("sudoers");