Introduce exit reasons for the event loop

pvdrz · pvdrz · commit 3418eb35d937 · 2023-06-17T16:21:29.000-05:00
diff --git a/sudo/lib/exec/event.rs b/sudo/lib/exec/event.rs
@@ -1,4 +1,4 @@
-use std::{io, ops::ControlFlow, os::fd::AsRawFd};
+use std::{io, os::fd::AsRawFd};
 
 use crate::log::dev_error;
 use crate::system::{
@@ -9,8 +9,14 @@ use crate::system::{
 use signal_hook::consts::*;
 
 pub(super) trait EventClosure: Sized {
-    /// Reason why the event loop should break. This is the return type of [`EventDispatcher::event_loop`].
+    /// Reason why the event loop should break.
+    ///
+    /// See [`EventDispatcher::set_break`] for more information.
     type Break;
+    /// Reason why the event loop should exit.
+    ///
+    /// See [`EventDispatcher::set_exit`] for more information.
+    type Exit;
     /// Operation that the closure must do when a signal arrives.
     fn on_signal(&mut self, info: SignalInfo, dispatcher: &mut EventDispatcher<Self>);
 }
@@ -38,7 +44,7 @@ macro_rules! define_signals {
                     })?,)*],
                     poll_set: PollSet::new(),
                     callbacks: Vec::with_capacity(SIGNALS.len()),
-                    status: ControlFlow::Continue(()),
+                    status: Status::Continue,
                 };
 
                 $(
@@ -73,6 +79,53 @@ define_signals! {
     SIGWINCH = 11,
 }
 
+enum Status<T: EventClosure> {
+    Continue,
+    Stop(StopReason<T>),
+}
+
+impl<T: EventClosure> Status<T> {
+    fn is_break(&self) -> bool {
+        matches!(self, Self::Stop(StopReason::Break(_)))
+    }
+
+    fn take_stop(&mut self) -> Option<StopReason<T>> {
+        // If the status ends up to be `Continue`, we are replacing it by another `Continue`.
+        let status = std::mem::replace(self, Self::Continue);
+        match status {
+            Status::Continue => None,
+            Status::Stop(reason) => Some(reason),
+        }
+    }
+
+    fn take_break(&mut self) -> Option<T::Break> {
+        match self.take_stop()? {
+            StopReason::Break(break_reason) => Some(break_reason),
+            reason @ StopReason::Exit(_) => {
+                // Replace back the status because it was not a `Break`.
+                *self = Self::Stop(reason);
+                None
+            }
+        }
+    }
+
+    fn take_exit(&mut self) -> Option<T::Exit> {
+        match self.take_stop()? {
+            reason @ StopReason::Break(_) => {
+                // Replace back the status because it was not an `Exit`.
+                *self = Self::Stop(reason);
+                None
+            }
+            StopReason::Exit(exit_reason) => Some(exit_reason),
+        }
+    }
+}
+
+pub(super) enum StopReason<T: EventClosure> {
+    Break(T::Break),
+    Exit(T::Exit),
+}
+
 #[derive(PartialEq, Eq, Hash, Clone)]
 struct EventId(usize);
 
@@ -83,7 +136,7 @@ pub(super) struct EventDispatcher<T: EventClosure> {
     signal_handlers: [SignalHandler; SIGNALS.len()],
     poll_set: PollSet<EventId>,
     callbacks: Vec<Callback<T>>,
-    status: ControlFlow<T::Break>,
+    status: Status<T>,
 }
 
 impl<T: EventClosure> EventDispatcher<T> {
@@ -107,7 +160,13 @@ impl<T: EventClosure> EventDispatcher<T> {
     ///
     /// This means that the event loop will stop even if other events are ready.
     pub(super) fn set_break(&mut self, reason: T::Break) {
-        self.status = ControlFlow::Break(reason);
+        self.status = Status::Stop(StopReason::Break(reason));
+    }
+
+    /// Stop the event loop when the callbacks for the events that are ready by now have been
+    /// dispatched and set a reason for it.
+    pub(super) fn set_exit(&mut self, reason: T::Exit) {
+        self.status = Status::Stop(StopReason::Exit(reason));
     }
 
     /// Return whether a break reason has been set already. This function will return `false` after
@@ -118,31 +177,27 @@ impl<T: EventClosure> EventDispatcher<T> {
 
     /// Run the event loop for this handler.
     ///
-    /// The event loop will continue indefinitely unless [`EventDispatcher::set_break`] is called.
-    pub(super) fn event_loop(&mut self, state: &mut T) -> T::Break {
+    /// The event loop will continue indefinitely unless you call [`EventDispatcher::set_break`] or
+    /// [`EventDispatcher::set_exit`].
+    pub(super) fn event_loop(&mut self, state: &mut T) -> StopReason<T> {
         loop {
             if let Ok(ids) = self.poll_set.poll() {
                 for EventId(id) in ids {
                     self.callbacks[id](state, self);
 
-                    if let Some(break_reason) = self.check_break() {
-                        return break_reason;
+                    if let Some(reason) = self.status.take_break() {
+                        return StopReason::Break(reason);
                     }
                 }
+                if let Some(reason) = self.status.take_exit() {
+                    return StopReason::Exit(reason);
+                }
+            } else {
+                // FIXME: maybe we shout return the IO error instead.
+                if let Some(reason) = self.status.take_stop() {
+                    return reason;
+                }
             }
-
-            if let Some(break_reason) = self.check_break() {
-                return break_reason;
-            }
-        }
-    }
-
-    pub(super) fn check_break(&mut self) -> Option<T::Break> {
-        // This is OK as we are swapping `Continue(())` by other `Continue(())` if the status is
-        // not `Break`.
-        match std::mem::replace(&mut self.status, ControlFlow::Continue(())) {
-            ControlFlow::Continue(()) => None,
-            ControlFlow::Break(reason) => Some(reason),
         }
     }
 }
diff --git a/sudo/lib/exec/monitor.rs b/sudo/lib/exec/monitor.rs
@@ -308,6 +308,7 @@ fn is_self_terminating(
 
 impl<'a> EventClosure for MonitorClosure<'a> {
     type Break = ();
+    type Exit = ();
 
     fn on_signal(&mut self, info: SignalInfo, dispatcher: &mut EventDispatcher<Self>) {
         dev_info!(
@@ -327,9 +328,7 @@ impl<'a> EventClosure for MonitorClosure<'a> {
             SIGCHLD => {
                 self.handle_sigchld(command_pid);
                 if self.command_pid.is_none() {
-                    // FIXME: This is what ogsudo calls a `loopexit`. Meaning that we should still
-                    // dispatch the remaining events to their callbacks and exit nicely.
-                    dispatcher.set_break(());
+                    dispatcher.set_exit(());
                 }
             }
             // Skip the signal if it was sent by the user and it is self-terminating.
diff --git a/sudo/lib/exec/parent.rs b/sudo/lib/exec/parent.rs
@@ -12,7 +12,7 @@ use crate::system::wait::{waitpid, WaitError, WaitOptions};
 use crate::system::{chown, fork, Group, User};
 use crate::system::{getpgid, interface::ProcessId, signal::SignalInfo};
 
-use super::event::{EventClosure, EventDispatcher};
+use super::event::{EventClosure, EventDispatcher, StopReason};
 use super::monitor::exec_monitor;
 use super::{
     backchannel::{BackchannelPair, MonitorMessage, ParentBackchannel, ParentMessage},
@@ -154,15 +154,10 @@ impl ParentClosure {
     }
 
     fn run(mut self, dispatcher: &mut EventDispatcher<Self>) -> io::Result<ExitReason> {
-        let exit_reason = match dispatcher.event_loop(&mut self) {
-            ParentMessage::IoError(code) => return Err(io::Error::from_raw_os_error(code)),
-            ParentMessage::CommandExit(code) => ExitReason::Code(code),
-            ParentMessage::CommandSignal(signal) => ExitReason::Signal(signal),
-            // We never set this event as the last event
-            ParentMessage::CommandPid(_) => unreachable!(),
-        };
-
-        Ok(exit_reason)
+        match dispatcher.event_loop(&mut self) {
+            StopReason::Break(err) | StopReason::Exit(ParentExit::Backchannel(err)) => Err(err),
+            StopReason::Exit(ParentExit::Command(exit_reason)) => Ok(exit_reason),
+        }
     }
 
     /// Read an event from the backchannel and return the event if it should break the event loop.
@@ -173,9 +168,15 @@ impl ParentClosure {
             // Failed to read command status. This means that something is wrong with the socket
             // and we should stop.
             Err(err) => {
-                dev_error!("could not receive message from monitor: {err}");
-                if !dispatcher.got_break() {
-                    dispatcher.set_break(err.into());
+                // If we get EOF the monitor exited or was killed
+                if err.kind() == io::ErrorKind::UnexpectedEof {
+                    dev_info!("parent received EOF from backchannel");
+                    dispatcher.set_exit(err.into());
+                } else {
+                    dev_error!("could not receive message from monitor: {err}");
+                    if !dispatcher.got_break() {
+                        dispatcher.set_break(err);
+                    }
                 }
             }
             Ok(event) => {
@@ -185,26 +186,24 @@ impl ParentClosure {
                     ParentMessage::CommandPid(pid) => {
                         dev_info!("received command PID ({pid}) from monitor");
                         self.command_pid = pid.into();
-                        // Do not set `CommandPid` as a break reason.
-                        return;
                     }
                     // The command terminated or the monitor was not able to spawn it. We should stop
                     // either way.
-                    ParentMessage::CommandExit(_code) => {
-                        dev_info!("command exited with status code {_code}");
+                    ParentMessage::CommandExit(code) => {
+                        dev_info!("command exited with status code {code}");
+                        dispatcher.set_exit(ExitReason::Code(code).into());
                     }
 
-                    ParentMessage::CommandSignal(_signal) => {
-                        dev_info!("command was terminated by {}", signal_fmt(_signal))
+                    ParentMessage::CommandSignal(signal) => {
+                        dev_info!("command was terminated by {}", signal_fmt(signal));
+                        dispatcher.set_exit(ExitReason::Signal(signal).into());
                     }
-                    ParentMessage::IoError(_code) => {
-                        dev_info!(
-                            "received error ({_code}) for monitor: {}",
-                            io::Error::from_raw_os_error(_code)
-                        )
+                    ParentMessage::IoError(code) => {
+                        let err = io::Error::from_raw_os_error(code);
+                        dev_info!("received error ({code}) for monitor: {err}",);
+                        dispatcher.set_break(err);
                     }
                 }
-                dispatcher.set_break(event);
             }
         }
     }
@@ -253,7 +252,7 @@ impl ParentClosure {
                 Err(err) if err.kind() == io::ErrorKind::BrokenPipe => {
                     dev_error!("broken pipe while writing to monitor over backchannel");
                     // FIXME: maybe we need a different event for backchannel errors.
-                    dispatcher.set_break(err.into());
+                    dispatcher.set_break(err);
                 }
                 // Non critical error, we can retry later.
                 Err(_) => {}
@@ -300,8 +299,28 @@ impl ParentClosure {
     }
 }
 
+enum ParentExit {
+    /// Error while reading from the backchannel.
+    Backchannel(io::Error),
+    /// The command exited.
+    Command(ExitReason),
+}
+
+impl From<io::Error> for ParentExit {
+    fn from(err: io::Error) -> Self {
+        Self::Backchannel(err)
+    }
+}
+
+impl From<ExitReason> for ParentExit {
+    fn from(reason: ExitReason) -> Self {
+        Self::Command(reason)
+    }
+}
+
 impl EventClosure for ParentClosure {
-    type Break = ParentMessage;
+    type Break = io::Error;
+    type Exit = ParentExit;
 
     fn on_signal(&mut self, info: SignalInfo, _dispatcher: &mut EventDispatcher<Self>) {
         dev_info!(
