fix: Proper waitpid return value handling (#3)

* fix: Proper handling of bespoke waitpid status

* nit: Improve example

* test: Add tests covering child pid termination by signal

Also prove that suspending and restarting child process results in
normal termination by signal afterwords

* nit: Update examples/error-handling-complete.rs

* chore: Remove leftover lines

* chore: Remove completed TODO

Author: brannondorsey

Cargo.lock

@@ -19,6 +19,7 @@ thiserror = "2.0.11"
 [dev-dependencies]
 criterion = "0.5.1"
 rand = "0.9.0"
+tempfile = "3.18.0"
 
 [[bench]]
 name = "benchmarks"

Cargo.toml

@@ -1,3 +1,7 @@
+//! This example is meant to show you all of the possible errors that can occur
+//! when using `execute_in_isolated_process()`.
+//!
+//! It doesn't actually do anything meaningful
 use MemIsolateError::*;
 use mem_isolate::errors::CallableDidNotExecuteError::*;
 use mem_isolate::errors::CallableExecutedError::*;
@@ -22,6 +26,8 @@ fn main() {
         Err(CallableStatusUnknown(ParentPipeReadFailed(_err))) => {}
         Err(CallableStatusUnknown(CallableProcessDiedDuringExecution)) => {}
         Err(CallableStatusUnknown(UnexpectedChildExitStatus(_status))) => {}
+        Err(CallableStatusUnknown(ChildProcessKilledBySignal(_signal))) => {}
+        Err(CallableStatusUnknown(UnexpectedWaitpidReturnValue(_val))) => {}
     };
 }
 

examples/error-handling-complete.rs

@@ -83,6 +83,28 @@ impl SystemFunctions for RealSystemFunctions {
     }
 }
 
+pub type WaitpidStatus = libc::c_int;
+pub type ExitStatus = libc::c_int;
+pub type Signal = libc::c_int;
+
+#[inline]
+pub fn child_process_exited_on_its_own(waitpid_status: WaitpidStatus) -> Option<ExitStatus> {
+    if libc::WIFEXITED(waitpid_status) {
+        Some(libc::WEXITSTATUS(waitpid_status))
+    } else {
+        None
+    }
+}
+
+#[inline]
+pub fn child_process_killed_by_signal(waitpid_status: WaitpidStatus) -> Option<Signal> {
+    if libc::WIFSIGNALED(waitpid_status) {
+        Some(libc::WTERMSIG(waitpid_status))
+    } else {
+        None
+    }
+}
+
 // For test builds, these functions will be provided by the mock module
 #[cfg(test)]
 mod tests {

src/c.rs

@@ -36,6 +36,7 @@ use thiserror::Error;
 /// [`MemIsolateError`] is the **primary error type returned by the crate**. The
 /// goal is to give the caller context about what happened to their callable if
 /// something went wrong.
+// TODO: Consider making this Send + Sync
 #[derive(Error, Debug, Serialize, Deserialize)]
 pub enum MemIsolateError {
     /// Indicates something went wrong before the callable was executed. Because
@@ -165,6 +166,15 @@ pub enum CallableStatusUnknownError {
     /// indicating the success or failure of the user-supplied callable itself.
     #[error("the callable process exited with an unexpected status: {0}")]
     UnexpectedChildExitStatus(i32),
+
+    /// The child process responsible for executing the user-supplied callable
+    /// was killed by a signal
+    #[error("the callable process was killed by a signal: {0}")]
+    ChildProcessKilledBySignal(i32),
+
+    /// Waitpid returned an unexpected value
+    #[error("waitpid returned an unexpected value: {0}")]
+    UnexpectedWaitpidReturnValue(i32),
 }
 
 fn serialize_os_error<S>(error: &io::Error, serializer: S) -> Result<S::Ok, S::Error>

src/errors.rs

@@ -11,7 +11,10 @@ use std::os::unix::io::FromRawFd;
 mod tests;
 
 mod c;
-use c::{ForkReturn, PipeFds, SystemFunctions};
+use c::{
+    ForkReturn, PipeFds, SystemFunctions, child_process_exited_on_its_own,
+    child_process_killed_by_signal,
+};
 
 pub mod errors;
 pub use errors::MemIsolateError;
@@ -28,9 +31,6 @@ pub use serde::{Serialize, de::DeserializeOwned};
 /// # Safety
 /// This code directly calls glibc functions (via the libc crate) and should
 /// only be used in a Unix environment.
-// TODO: Real error handling with thiserror. Plumbing errors in the child also
-// need to be passed back to the parent. TODO: Benchmark nicing the child
-// process.
 pub fn execute_in_isolated_process<F, T>(callable: F) -> Result<T, MemIsolateError>
 where
     // TODO: Consider restricting to disallow FnMut() closures
@@ -56,7 +56,6 @@ where
     };
 
     // Create a pipe.
-    // TODO: Should I use dup2 somewhere here?
     let PipeFds { read_fd, write_fd } = match sys.pipe() {
         Ok(pipe_fds) => pipe_fds,
         Err(err) => {
@@ -122,28 +121,34 @@ where
             }
 
             // Wait for the child process to exit
-            // TODO: waitpid doesn't return the exit status you expect it to.
-            // See the Linux Programming Interface book for more details.
-            let status = match sys.waitpid(child_pid) {
+            let waitpid_bespoke_status = match sys.waitpid(child_pid) {
                 Ok(status) => status,
                 Err(wait_err) => {
                     return Err(CallableStatusUnknown(WaitFailed(wait_err)));
                 }
             };
 
-            match status {
-                CHILD_EXIT_HAPPY => {}
-                CHILD_EXIT_IF_READ_CLOSE_FAILED => {
-                    return Err(CallableDidNotExecute(ChildPipeCloseFailed(None)));
-                }
-                CHILD_EXIT_IF_WRITE_FAILED => {
-                    return Err(CallableExecuted(ChildPipeWriteFailed(None)));
-                }
-                unhandled_status => {
-                    return Err(CallableStatusUnknown(UnexpectedChildExitStatus(
-                        unhandled_status,
-                    )));
+            if let Some(exit_status) = child_process_exited_on_its_own(waitpid_bespoke_status) {
+                match exit_status {
+                    CHILD_EXIT_HAPPY => {}
+                    CHILD_EXIT_IF_READ_CLOSE_FAILED => {
+                        return Err(CallableDidNotExecute(ChildPipeCloseFailed(None)));
+                    }
+                    CHILD_EXIT_IF_WRITE_FAILED => {
+                        return Err(CallableExecuted(ChildPipeWriteFailed(None)));
+                    }
+                    unhandled_status => {
+                        return Err(CallableStatusUnknown(UnexpectedChildExitStatus(
+                            unhandled_status,
+                        )));
+                    }
                 }
+            } else if let Some(signal) = child_process_killed_by_signal(waitpid_bespoke_status) {
+                return Err(CallableStatusUnknown(ChildProcessKilledBySignal(signal)));
+            } else {
+                return Err(CallableStatusUnknown(UnexpectedWaitpidReturnValue(
+                    waitpid_bespoke_status,
+                )));
             }
 
             // Read from the pipe by wrapping the read fd as a File