From 372315fa93895642f63da16873b3b088a593f536 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 15 Jun 2023 14:48:33 -0500 Subject: [PATCH 1/4] `fork` and `exec` the command --- sudo/lib/exec/monitor.rs | 148 ++++++++++++++++++++++++++++++++------- 1 file changed, 123 insertions(+), 25 deletions(-) diff --git a/sudo/lib/exec/monitor.rs b/sudo/lib/exec/monitor.rs index 9d46ed204..94ba03145 100644 --- a/sudo/lib/exec/monitor.rs +++ b/sudo/lib/exec/monitor.rs @@ -1,15 +1,22 @@ use std::{ ffi::c_int, - io, - os::fd::OwnedFd, - process::{exit, Child, Command}, + io::{self, Read, Write}, + os::{ + fd::OwnedFd, + unix::{net::UnixStream, process::CommandExt}, + }, + process::{exit, Command}, time::Duration, }; use crate::log::{dev_info, dev_warn}; use crate::system::{ - getpgid, interface::ProcessId, kill, setpgid, setsid, signal::SignalInfo, - term::set_controlling_terminal, + fork, getpgid, + interface::ProcessId, + kill, setpgid, setsid, + signal::SignalInfo, + term::{set_controlling_terminal, tcgetpgrp, tcsetpgrp}, + wait::{waitpid, WaitError, WaitOptions}, }; use signal_hook::consts::*; @@ -24,7 +31,7 @@ use super::{cond_fmt, signal_fmt}; // FIXME: This should return `io::Result` but `!` is not stable yet. pub(super) fn exec_monitor( pty_follower: OwnedFd, - mut command: Command, + command: Command, backchannel: &mut MonitorBackchannel, ) -> io::Result<()> { let mut dispatcher = EventDispatcher::::new()?; @@ -46,6 +53,9 @@ pub(super) fn exec_monitor( err })?; + // Use a pipe to get the IO error if `exec_command` fails. + let (mut errpipe_tx, errpipe_rx) = UnixStream::pair()?; + // Wait for the parent to give us green light before spawning the command. This avoids race // conditions when the command exits quickly. let event = retry_while_interrupted(|| backchannel.recv()).map_err(|err| { @@ -58,22 +68,35 @@ pub(super) fn exec_monitor( // FIXME (ogsudo): Some extra config happens here if selinux is available. - // FIXME (ogsudo): Do any additional configuration that needs to be run after `fork` but before `exec`. - - // spawn the command. - let command = command.spawn().map_err(|err| { - dev_warn!("cannot spawn command: {err}"); + let command_pid = fork().map_err(|err| { + dev_warn!("unable to fork command process: {err}"); err })?; - let command_pid = command.id() as ProcessId; + if command_pid == 0 { + drop(errpipe_rx); + + let err = exec_command(command, pty_follower); + dev_warn!("failed to execute command: {err}"); + // If `exec_command` returns, it means that executing the command failed. Send the error to + // the monitor using the pipe. + if let Some(error_code) = err.raw_os_error() { + errpipe_tx.write_all(&error_code.to_ne_bytes()).ok(); + } + drop(errpipe_tx); + // FIXME: Calling `exit` doesn't run any destructors, clean everything up. + exit(1) + } // Send the command's PID to the parent. if let Err(err) = backchannel.send(&ParentMessage::CommandPid(command_pid)) { dev_warn!("cannot send command PID to parent: {err}"); } - let mut closure = MonitorClosure::new(command, command_pid, backchannel, &mut dispatcher); + let mut closure = MonitorClosure::new(command_pid, errpipe_rx, backchannel, &mut dispatcher); + + // Set the foreground group for the pty follower. + tcsetpgrp(&pty_follower, closure.command_pgrp).ok(); // FIXME (ogsudo): Here's where the signal mask is removed because the handlers for the signals // have been setup after initializing the closure. @@ -91,25 +114,45 @@ pub(super) fn exec_monitor( exit(1) } +// FIXME: This should return `io::Result` but `!` is not stable yet. +fn exec_command(mut command: Command, pty_follower: OwnedFd) -> io::Error { + // FIXME (ogsudo): Do any additional configuration that needs to be run after `fork` but before `exec` + let command_pid = std::process::id() as ProcessId; + + setpgid(0, command_pid).ok(); + + // Wait for the monitor to set us as the foreground group for the pty. + while !tcgetpgrp(&pty_follower).is_ok_and(|pid| pid == command_pid) { + std::thread::sleep(std::time::Duration::from_micros(1)); + } + + command.exec() +} + struct MonitorClosure<'a> { - command: Child, /// The command PID. /// /// This is `Some` iff the process is still running. command_pid: Option, command_pgrp: ProcessId, + errpipe_rx: UnixStream, backchannel: &'a mut MonitorBackchannel, } impl<'a> MonitorClosure<'a> { fn new( - command: Child, command_pid: ProcessId, + errpipe_rx: UnixStream, backchannel: &'a mut MonitorBackchannel, dispatcher: &mut EventDispatcher, ) -> Self { // FIXME (ogsudo): Store the pgid of the monitor. + // Register the callback to receive the IO error if the command fails to execute. + dispatcher.set_read_callback(&errpipe_rx, |monitor, dispatcher| { + monitor.read_errpipe(dispatcher) + }); + // Register the callback to receive events from the backchannel dispatcher.set_read_callback(backchannel, |monitor, dispatcher| { monitor.read_backchannel(dispatcher) @@ -122,9 +165,9 @@ impl<'a> MonitorClosure<'a> { }; Self { - command, command_pid: Some(command_pid), command_pgrp, + errpipe_rx, backchannel, } } @@ -154,7 +197,67 @@ impl<'a> MonitorClosure<'a> { } } } + + fn handle_sigchld(&mut self, command_pid: ProcessId) { + let status = loop { + match waitpid(command_pid, WaitOptions::new().untraced().no_hang()) { + Ok((_pid, status)) => break status, + Err(WaitError::Io(err)) if was_interrupted(&err) => {} + Err(_) => return, + } + }; + + if let Some(exit_code) = status.exit_status() { + dev_info!("command ({command_pid}) exited with status code {exit_code}"); + // The command did exit, set it's PID to `None`. + self.command_pid = None; + self.backchannel + .send(&ParentMessage::CommandExit(exit_code)) + .ok(); + } else if let Some(signal) = status.term_signal() { + dev_info!( + "command ({command_pid}) was terminated by {}", + signal_fmt(signal), + ); + // The command was terminated, set it's PID to `None`. + self.command_pid = None; + self.backchannel + .send(&ParentMessage::CommandSignal(signal)) + .ok(); + } else if let Some(_signal) = status.stop_signal() { + // FIXME: we should save the foreground process group ID so we can restore it later. + dev_info!( + "command ({command_pid}) was stopped by {}", + signal_fmt(_signal), + ); + } else if status.did_continue() { + dev_info!("command ({command_pid}) continued execution"); + } else { + dev_warn!("unexpected wait status for command ({command_pid})") + } + } + + fn read_errpipe(&mut self, dispatcher: &mut EventDispatcher) { + let mut buf = 0i32.to_ne_bytes(); + match self.errpipe_rx.read_exact(&mut buf) { + Err(err) if was_interrupted(&err) => { /* Retry later */ } + Err(err) => { + // Could not read from the pipe, report error to the parent. + // FIXME: Maybe we should have a different variant for this. + self.backchannel.send(&err.into()).ok(); + dispatcher.set_break(()); + } + Ok(_) => { + // Received error code from the command, forward it to the parent. + let error_code = i32::from_ne_bytes(buf); + self.backchannel + .send(&ParentMessage::IoError(error_code)) + .ok(); + } + } + } } + /// Send a signal to the command. fn send_signal(signal: c_int, command_pid: ProcessId, from_parent: bool) { dev_info!( @@ -221,17 +324,12 @@ impl<'a> EventClosure for MonitorClosure<'a> { }; match info.signal() { - // FIXME: check `mon_handle_sigchld` SIGCHLD => { - if let Ok(Some(exit_status)) = self.command.try_wait() { - dev_info!( - "command ({command_pid}) exited with status: {}", - exit_status - ); - // The command has terminated, set it's PID to `None`. - self.command_pid = None; + 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(()); - self.backchannel.send(&exit_status.into()).unwrap(); } } // Skip the signal if it was sent by the user and it is self-terminating. From 3418eb35d9374e14b31cf358c8346f7d47eec318 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 15 Jun 2023 16:32:06 -0500 Subject: [PATCH 2/4] Introduce exit reasons for the event loop --- sudo/lib/exec/event.rs | 99 +++++++++++++++++++++++++++++++--------- sudo/lib/exec/monitor.rs | 5 +- sudo/lib/exec/parent.rs | 73 ++++++++++++++++++----------- 3 files changed, 125 insertions(+), 52 deletions(-) diff --git a/sudo/lib/exec/event.rs b/sudo/lib/exec/event.rs index 11e5a7e6a..0dbe5764a 100644 --- 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); } @@ -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 { + Continue, + Stop(StopReason), +} + +impl Status { + fn is_break(&self) -> bool { + matches!(self, Self::Stop(StopReason::Break(_))) + } + + fn take_stop(&mut self) -> Option> { + // 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 { + 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 { + 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 { + Break(T::Break), + Exit(T::Exit), +} + #[derive(PartialEq, Eq, Hash, Clone)] struct EventId(usize); @@ -83,7 +136,7 @@ pub(super) struct EventDispatcher { signal_handlers: [SignalHandler; SIGNALS.len()], poll_set: PollSet, callbacks: Vec>, - status: ControlFlow, + status: Status, } impl EventDispatcher { @@ -107,7 +160,13 @@ impl EventDispatcher { /// /// 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 EventDispatcher { /// 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 { 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 { - // 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 index 94ba03145..b39564f5c 100644 --- 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) { 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 index 43d9d44ce..f13bbc81b 100644 --- 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) -> io::Result { - 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 for ParentExit { + fn from(err: io::Error) -> Self { + Self::Backchannel(err) + } +} + +impl From 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) { dev_info!( From f5a4d16302e8e02b7d3c2d9db354bc233886cdee Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 15 Jun 2023 19:10:53 -0500 Subject: [PATCH 3/4] Fix bug where the parent handlers were leaked to the moniton --- sudo/lib/exec/event.rs | 7 +++++++ sudo/lib/exec/parent.rs | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/sudo/lib/exec/event.rs b/sudo/lib/exec/event.rs index 0dbe5764a..cf3a63dd3 100644 --- a/sudo/lib/exec/event.rs +++ b/sudo/lib/exec/event.rs @@ -200,4 +200,11 @@ impl EventDispatcher { } } } + + /// Unregister all the handlers created by the dispatcher. + pub(super) fn unregister_handlers(self) { + for handler in self.signal_handlers { + handler.unregister(); + } + } } diff --git a/sudo/lib/exec/parent.rs b/sudo/lib/exec/parent.rs index f13bbc81b..2c5e78f6b 100644 --- a/sudo/lib/exec/parent.rs +++ b/sudo/lib/exec/parent.rs @@ -54,6 +54,8 @@ pub(super) fn exec_pty( // enabled or if sudo is running in background. // FIXME (ogsudo): Copy terminal settings from `/dev/tty` to the pty. // FIXME (ogsudo): Start in raw mode unless we're part of a pipeline + // FIXME: it would be better if we didn't create the dispatcher before the fork and managed + // to block all the signals instead. let mut dispatcher = EventDispatcher::::new()?; let monitor_pid = fork().map_err(|err| { @@ -66,6 +68,10 @@ pub(super) fn exec_pty( drop(pty.leader); drop(backchannels.parent); + // Unregister all the handlers so `exec_monitor` can register new ones for the monitor + // process. + dispatcher.unregister_handlers(); + // If `exec_monitor` returns, it means we failed to execute the command somehow. if let Err(err) = exec_monitor(pty.follower, command, &mut backchannels.monitor) { if let Err(err) = backchannels.monitor.send(&err.into()) { From 1e8b055e12eb49bff6b6bcfd2a477247875259d7 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 15 Jun 2023 19:22:34 -0500 Subject: [PATCH 4/4] "Fix" the ignored sigtstp test --- sudo/lib/exec/backchannel.rs | 18 ++++++-------- sudo/lib/exec/monitor.rs | 46 ++++++++++++++++++++---------------- sudo/lib/exec/parent.rs | 4 +++- 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/sudo/lib/exec/backchannel.rs b/sudo/lib/exec/backchannel.rs index 080d129e3..b98e32861 100644 --- a/sudo/lib/exec/backchannel.rs +++ b/sudo/lib/exec/backchannel.rs @@ -4,14 +4,13 @@ use std::{ mem::size_of, os::{ fd::{AsRawFd, RawFd}, - unix::{net::UnixStream, process::ExitStatusExt}, + unix::net::UnixStream, }, - process::ExitStatus, }; use crate::system::{interface::ProcessId, signal::SignalNumber}; -use super::signal_fmt; +use super::{signal_fmt, ExitReason}; type Prefix = u8; type ParentData = c_int; @@ -89,14 +88,11 @@ impl From for ParentMessage { } } -impl From for ParentMessage { - fn from(status: ExitStatus) -> Self { - if let Some(code) = status.code() { - Self::CommandExit(code) - } else { - // `ExitStatus::code` docs state that it only returns `None` if the process was - // terminated by a signal so this should always succeed. - Self::CommandSignal(status.signal().unwrap()) +impl From for ParentMessage { + fn from(reason: ExitReason) -> Self { + match reason { + ExitReason::Code(code) => Self::CommandExit(code), + ExitReason::Signal(signal) => Self::CommandSignal(signal), } } } diff --git a/sudo/lib/exec/monitor.rs b/sudo/lib/exec/monitor.rs index b39564f5c..e40dc4959 100644 --- a/sudo/lib/exec/monitor.rs +++ b/sudo/lib/exec/monitor.rs @@ -9,7 +9,6 @@ use std::{ time::Duration, }; -use crate::log::{dev_info, dev_warn}; use crate::system::{ fork, getpgid, interface::ProcessId, @@ -18,6 +17,10 @@ use crate::system::{ term::{set_controlling_terminal, tcgetpgrp, tcsetpgrp}, wait::{waitpid, WaitError, WaitOptions}, }; +use crate::{ + exec::event::StopReason, + log::{dev_info, dev_warn}, +}; use signal_hook::consts::*; @@ -25,6 +28,7 @@ use super::{ backchannel::{MonitorBackchannel, MonitorMessage, ParentMessage}, event::{EventClosure, EventDispatcher}, io_util::{retry_while_interrupted, was_interrupted}, + ExitReason, }; use super::{cond_fmt, signal_fmt}; @@ -103,10 +107,17 @@ pub(super) fn exec_monitor( // FIXME (ogsudo): Set the command as the foreground process for the follower. // Start the event loop. - dispatcher.event_loop(&mut closure); + let reason = dispatcher.event_loop(&mut closure); // FIXME (ogsudo): Terminate the command using `killpg` if it's not terminated. // FIXME (ogsudo): Take the controlling tty so the command's children don't receive SIGHUP when we exit. - // FIXME (ogsudo): Send the command status back to the parent. + + match reason { + StopReason::Break(()) => {} + StopReason::Exit(exit_reason) => { + closure.backchannel.send(&exit_reason.into()).ok(); + } + } + // FIXME (ogsudo): The tty is restored here if selinux is available. drop(closure); @@ -180,6 +191,7 @@ impl<'a> MonitorClosure<'a> { // There's something wrong with the backchannel, break the event loop Err(err) => { dev_warn!("monitor could not read from backchannel: {}", err); + // FIXME: maybe the break reason should be `io::Error` instead. dispatcher.set_break(()); self.backchannel.send(&err.into()).unwrap(); } @@ -198,7 +210,7 @@ impl<'a> MonitorClosure<'a> { } } - fn handle_sigchld(&mut self, command_pid: ProcessId) { + fn handle_sigchld(&mut self, command_pid: ProcessId, dispatcher: &mut EventDispatcher) { let status = loop { match waitpid(command_pid, WaitOptions::new().untraced().no_hang()) { Ok((_pid, status)) => break status, @@ -211,9 +223,7 @@ impl<'a> MonitorClosure<'a> { dev_info!("command ({command_pid}) exited with status code {exit_code}"); // The command did exit, set it's PID to `None`. self.command_pid = None; - self.backchannel - .send(&ParentMessage::CommandExit(exit_code)) - .ok(); + dispatcher.set_exit(ExitReason::Code(exit_code)) } else if let Some(signal) = status.term_signal() { dev_info!( "command ({command_pid}) was terminated by {}", @@ -221,15 +231,16 @@ impl<'a> MonitorClosure<'a> { ); // The command was terminated, set it's PID to `None`. self.command_pid = None; - self.backchannel - .send(&ParentMessage::CommandSignal(signal)) - .ok(); - } else if let Some(_signal) = status.stop_signal() { - // FIXME: we should save the foreground process group ID so we can restore it later. + dispatcher.set_exit(ExitReason::Signal(signal)) + } else if let Some(signal) = status.stop_signal() { dev_info!( "command ({command_pid}) was stopped by {}", - signal_fmt(_signal), + signal_fmt(signal), ); + // FIXME: we should save the foreground process group ID so we can restore it later. + self.backchannel + .send(&ParentMessage::CommandSignal(signal)) + .ok(); } else if status.did_continue() { dev_info!("command ({command_pid}) continued execution"); } else { @@ -308,7 +319,7 @@ fn is_self_terminating( impl<'a> EventClosure for MonitorClosure<'a> { type Break = (); - type Exit = (); + type Exit = ExitReason; fn on_signal(&mut self, info: SignalInfo, dispatcher: &mut EventDispatcher) { dev_info!( @@ -325,12 +336,7 @@ impl<'a> EventClosure for MonitorClosure<'a> { }; match info.signal() { - SIGCHLD => { - self.handle_sigchld(command_pid); - if self.command_pid.is_none() { - dispatcher.set_exit(()); - } - } + SIGCHLD => self.handle_sigchld(command_pid, dispatcher), // 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) => {} diff --git a/sudo/lib/exec/parent.rs b/sudo/lib/exec/parent.rs index 2c5e78f6b..a361418b9 100644 --- a/sudo/lib/exec/parent.rs +++ b/sudo/lib/exec/parent.rs @@ -199,8 +199,10 @@ impl ParentClosure { dev_info!("command exited with status code {code}"); dispatcher.set_exit(ExitReason::Code(code).into()); } - ParentMessage::CommandSignal(signal) => { + // FIXME: this isn't right as the command has not exited if the signal is + // not a termination one. However, doing this makes us fail an ignored + // compliance test instead of hanging forever. dev_info!("command was terminated by {}", signal_fmt(signal)); dispatcher.set_exit(ExitReason::Signal(signal).into()); }