Repository: memorysafety/sudo-rs (rust)

Issue:
wrong exit code when child process is terminated by a signal
relevant test (added in  #111)

https://github.com/memorysafety/sudo-rs/blob/a536a64d75df5dd90f4d90d8a18a59bcf9d1e2f8/test-framework/sudo-compliance-tests/src/child_process/signal_handling/mod.rs#L57-L66

the original sudo exits with code 143 -- the same code `sh -c 'kill $$'` exits with. sudo-rs exits with code 1 in this case

cc @pvdrz 

Pull request description:
Better signal handling
This PR introduces the following changes:

- Handle SIGWINCH as a regular signal as we are not doing any I/O logging.
- Handle SIGALRM by sending SIGHUP, SIGTERM and SIGKILL to the command to match ogsudo's behavior.
- Only overwrite the same signal handlers that ogsudo overwrites.
- If the command was terminated by a signal, send that signal to sudo to terminate itself and generate the correct exit status (aka fix #112)

On a very @japaric fashion, this PR is easier to review commit by commit :P 

Patch:
diff --git a/lib/sudo-common/src/context.rs b/lib/sudo-common/src/context.rs
index e625ef49..ad4de39b 100644
--- a/lib/sudo-common/src/context.rs
+++ b/lib/sudo-common/src/context.rs
@@ -68,6 +68,7 @@ pub struct Context<'a> {
     // system
     pub hostname: String,
     pub current_user: User,
+    pub pid: i32,
 }
 
 pub trait Configuration {
@@ -142,6 +143,7 @@ impl<'a> Context<'a> {
             login: sudo_options.login,
             shell: sudo_options.shell,
             chdir: sudo_options.directory.clone(),
+            pid: sudo_system::Process::process_id(),
         })
     }
 }
diff --git a/lib/sudo-exec/src/lib.rs b/lib/sudo-exec/src/lib.rs
index a3a6add6..b4105ed5 100644
--- a/lib/sudo-exec/src/lib.rs
+++ b/lib/sudo-exec/src/lib.rs
@@ -2,26 +2,26 @@
 use std::{
     ffi::c_int,
     io,
-    os::unix::process::CommandExt,
+    os::unix::process::{CommandExt, ExitStatusExt},
     process::{Command, ExitStatus},
+    time::Duration,
 };
 
 use signal_hook::{
     consts::*,
     iterator::{exfiltrator::WithOrigin, SignalsInfo},
-    low_level::siginfo::{Cause, Origin, Sent},
+    low_level::{
+        emulate_default_handler,
+        siginfo::{Cause, Process, Sent},
+    },
 };
 use sudo_common::context::{Context, Environment};
 use sudo_system::{getpgid, kill};
 
-/// We do not handle `SIGKILL`, `SIGSTOP`, `SIGILL`, `SIGFPE` nor `SIGSEGV` because those should
-/// not be intercepted and replaced. according to `POSIX`.
-// FIXME: are we missing any signals? `SIGEMT`, `SIGLOST` and `SIGPWR` are not exposed by
-// `signal-hook`.
+/// We only handle the signals that ogsudo handles.
 const SIGNALS: &[c_int] = &[
-    SIGABRT, SIGALRM, SIGBUS, SIGCHLD, SIGCONT, SIGHUP, SIGINT, SIGPIPE, SIGPROF, SIGQUIT, SIGSYS,
-    SIGTERM, SIGTRAP, SIGTSTP, SIGTTIN, SIGTTOU, SIGURG, SIGUSR1, SIGUSR2, SIGVTALRM, SIGWINCH,
-    SIGXCPU, SIGXFSZ, SIGIO,
+    SIGINT, SIGQUIT, SIGTSTP, SIGTERM, SIGHUP, SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2, SIGCHLD,
+    SIGCONT, SIGWINCH,
 ];
 
 /// Based on `ogsudo`s `exec_nopty` function.
@@ -40,74 +40,87 @@ pub fn run_command(ctx: Context<'_>, env: Environment) -> io::Result<ExitStatus>
     let mut signals = SignalsInfo::<WithOrigin>::new(SIGNALS)?;
 
     loop {
-        for Origin {
-            signal,
-            process,
-            cause,
-            ..
-        } in signals.pending()
-        {
-            match signal {
+        // First we check if the command is done.
+        if let Some(status) = cmd.try_wait()? {
+            if let Some(signal) = status.signal() {
+                // If the command terminated because of a signal, we send this signal to sudo
+                // itself to match the original sudo behavior. If we fail we just return the status
+                // code.
+                if kill(ctx.pid, signal) != -1 {
+                    // Given that we overwrote the default handlers for all the signals, we must
+                    // emulate them to handle the signal we just sent correctly.
+                    for info in signals.pending() {
+                        emulate_default_handler(info.signal)?;
+                    }
+                }
+            }
+
+            return Ok(status);
+        }
+
+        // Then we check any pending signals that we received.
+        for info in signals.pending() {
+            let user_signaled = info.cause == Cause::Sent(Sent::User);
+            match info.signal {
                 SIGCHLD => {
                     // FIXME: check `handle_sigchld_nopty`
-                    cmd.kill()?;
-                }
-                SIGWINCH => {
-                    // FIXME: check `handle_sigwinch`
-                    cmd.kill()?;
+                    // We just wait until all the children are done.
+                    continue;
                 }
-                SIGINT | SIGQUIT | SIGTSTP => {
-                    if cause != Cause::Sent(Sent::User) {
+                SIGWINCH | SIGINT | SIGQUIT | SIGTSTP => {
+                    // Skip the signal if it was not sent by the user or if it is self-terminating.
+                    if !user_signaled || is_self_terminating(info.process, cmd_pid, ctx.pid) {
                         continue;
                     }
-
-                    if let Some(process) = process {
-                        if process.pid != 0 {
-                            if process.pid == cmd_pid {
-                                continue;
-                            }
-                            let process_grp = getpgid(process.pid);
-                            if process_grp != -1 {
-                                // FIXME: we should also check that the process group is not the
-                                // sudo PID.
-                                if process_grp == cmd_pid {
-                                    continue;
-                                }
-                            }
-                        }
-                    }
                 }
                 _ => {
-                    if cause == Cause::Sent(Sent::User) {
-                        if let Some(process) = process {
-                            if process.pid != 0 {
-                                if process.pid == cmd_pid {
-                                    continue;
-                                }
-                                let process_grp = getpgid(process.pid);
-                                if process_grp != -1 {
-                                    // FIXME: we should also check that the process group is not the
-                                    // sudo PID.
-                                    if process_grp == cmd_pid {
-                                        continue;
-                                    }
-                                }
-                            }
-                        }
+                    // Skip the signal if it was sent by the user and it is self-terminating.
+                    if user_signaled && is_self_terminating(info.process, cmd_pid, ctx.pid) {
+                        continue;
                     }
                 }
             }
 
-            if signal == SIGALRM {
-                // FIXME: check `terminate_command` to match behavior.
-                cmd.kill()?;
-            } else if kill(cmd_pid, signal) != 0 {
+            let status = if info.signal == SIGALRM {
+                // Kill the command with increasing urgency.
+                // Based on `terminate_command`.
+                kill(cmd_pid, SIGHUP);
+                kill(cmd_pid, SIGTERM);
+                std::thread::sleep(Duration::from_secs(2));
+                kill(cmd_pid, SIGKILL)
+            } else {
+                kill(cmd_pid, info.signal)
+            };
+
+            if status != 0 {
                 eprintln!("kill failed");
             }
         }
+    }
+}
 
-        if let Some(code) = cmd.try_wait()? {
-            return Ok(code);
+/// Decides if the signal sent by `process` is self-terminating.
+///
+/// A signal is self-terminating if the PID of the `process`:
+/// - is the same PID of the command, or
+/// - is in the process group of the command and either sudo of the command are the leader.
+fn is_self_terminating(process: Option<Process>, cmd_pid: i32, sudo_pid: i32) -> bool {
+    if let Some(process) = process {
+        if process.pid != 0 {
+            if process.pid == cmd_pid {
+                return true;
+            }
+            let grp_leader = getpgid(process.pid);
+
+            if grp_leader != -1 {
+                if grp_leader == cmd_pid || grp_leader == sudo_pid {
+                    return true;
+                }
+            } else {
+                eprintln!("Could not fetch process group ID");
+            }
         }
     }
+
+    false
 }


Test patch:
diff --git a/lib/sudo-env/tests/env_tests.rs b/lib/sudo-env/tests/env_tests.rs
index a8b2237f..a0b57d51 100644
--- a/lib/sudo-env/tests/env_tests.rs
+++ b/lib/sudo-env/tests/env_tests.rs
@@ -132,6 +132,7 @@ fn create_test_context<'a>(sudo_options: &'a SudoOptions) -> Context<'a> {
         login: sudo_options.login,
         shell: sudo_options.shell,
         chdir: sudo_options.directory.clone(),
+        pid: std::process::id() as i32,
     }
 }