Repository: AFLplusplus/LibAFL (language: rust)

Issue:
Non-zero exit code is considered ok in `CommandExecutor`
In `CommandExecutor` the exit code of the child is not checked and anything exited without a signal is counted as `Ok`. 

https://github.com/AFLplusplus/LibAFL/blob/a8d2e8711bf735f3b6455f4173f5510bf4d04160/libafl/src/executors/command.rs#L357

Is it intentional? Is the executor only interested in crashes like segfaults? If so, shouldn't we add to the configurator?
Because let's say a Rust program panics (with unwinding behavior), it will exit normally with a non-zero code, but something unexpected might have happened in it.


Discussion / hints:
> Is it intentional?

Yes. We consider it as a crash only if it was stopped by signal. because you can't really say which exit status (except for the signals) should be crash or not, that depends on your target.

> Is the executor only interested in crashes like segfaults?

Not just segfaults, but any signals that stopeed the program from executing further including aborting, sigabrt, sigill, ... etc.

> If so, shouldn't we add to the configurator?

Yes it would be nice. Could you send a PR? 
> Yes it would be nice. Could you send a PR?

Sure. Any recommendations for the API design? How about this?
```rust
pub trait CommandConfigurator<I, C = Child>: Sized {
    // ...
    fn exit_kind_from_status(&self, status: &std::process::ExitStatus) -> ExitKind {
        match status.signal() {
            Some(9) => ExitKind::Oom,
            Some(_) => ExitKind::Crash,
            None => ExitKind::Ok,
        }  
    }
}
```
How should it be used?

I think we could have `crash_exit_code(status)` and `oom_exit_code(status)` functions in the builder
Alternatively we could have an optional closure that runs after each execution and returns ExitKind
> How should it be used?

If you mean the user side, they can provide a custom implementation for it as a part of their configurator. In `CommandExecutor` it will look like this:
```diff
 
-        let res = match child
+        let exit_kind = child
             .wait_timeout(self.configurer.exec_timeout())
             .expect("waiting on child failed")
-            .map(|status| status.signal())
-        {
-            // for reference: https://www.man7.org/linux/man-pages/man7/signal.7.html
-            Some(Some(9)) => Ok(ExitKind::Oom),
-            Some(Some(_)) => Ok(ExitKind::Crash),
-            Some(None) => Ok(ExitKind::Ok),
-            None => {
+            .map(|status| self.configurer.exit_kind_from_status(&status))
+            .unwrap_or_else(|| {
                 // if this fails, there is not much we can do. let's hope it failed because the process finished
                 // in the meantime.
                 drop(child.kill());
                 // finally, try to wait to properly clean up system resources.
                 drop(child.wait());
-                Ok(ExitKind::Timeout)
-            }
-        };
+                ExitKind::Timeout
+            });
 
-        if let Ok(exit_kind) = res {
-            self.observers
-                .post_exec_child_all(state, input, &exit_kind)?;
-        }
+        self.observers
+            .post_exec_child_all(state, input, &exit_kind)?;
```
> an optional closure that runs after each execution and returns ExitKind

I think my suggestion is as same as this but as a function in `CommandConfigurator`.

Fix patch:
diff --git a/libafl/src/executors/command.rs b/libafl/src/executors/command.rs
index 7e98568d10..ac1fe6d07f 100644
--- a/libafl/src/executors/command.rs
+++ b/libafl/src/executors/command.rs
@@ -38,7 +38,9 @@ use typed_builder::TypedBuilder;
 
 use super::HasTimeout;
 #[cfg(all(feature = "std", unix))]
-use crate::executors::{Executor, ExitKind};
+use crate::executors::Executor;
+#[cfg(all(feature = "std", any(unix, doc)))]
+use crate::executors::ExitKind;
 use crate::{
     corpus::Corpus,
     executors::{hooks::ExecutorHooksTuple, HasObservers},
@@ -337,8 +339,6 @@ where
     OT: Debug + ObserversTuple<I, S>,
 {
     fn execute_input_with_command(&mut self, state: &mut S, input: &I) -> Result<ExitKind, Error> {
-        use std::os::unix::prelude::ExitStatusExt;
-
         use wait_timeout::ChildExt;
 
         *state.executions_mut() += 1;
@@ -346,29 +346,21 @@ where
 
         let mut child = self.configurer.spawn_child(input)?;
 
-        let res = match child
+        let exit_kind = child
             .wait_timeout(self.configurer.exec_timeout())
             .expect("waiting on child failed")
-            .map(|status| status.signal())
-        {
-            // for reference: https://www.man7.org/linux/man-pages/man7/signal.7.html
-            Some(Some(9)) => Ok(ExitKind::Oom),
-            Some(Some(_)) => Ok(ExitKind::Crash),
-            Some(None) => Ok(ExitKind::Ok),
-            None => {
+            .map(|status| self.configurer.exit_kind_from_status(&status))
+            .unwrap_or_else(|| {
                 // if this fails, there is not much we can do. let's hope it failed because the process finished
                 // in the meantime.
                 drop(child.kill());
                 // finally, try to wait to properly clean up system resources.
                 drop(child.wait());
-                Ok(ExitKind::Timeout)
-            }
-        };
+                ExitKind::Timeout
+            });
 
-        if let Ok(exit_kind) = res {
-            self.observers
-                .post_exec_child_all(state, input, &exit_kind)?;
-        }
+        self.observers
+            .post_exec_child_all(state, input, &exit_kind)?;
 
         if let Some(h) = &mut self.configurer.stdout_observer() {
             let mut stdout = Vec::new();
@@ -392,7 +384,7 @@ where
             let obs = observers.index_mut(h);
             obs.observe_stderr(&stderr);
         }
-        res
+        Ok(exit_kind)
     }
 }
 
@@ -809,7 +801,7 @@ impl CommandExecutorBuilder {
 ///     MyExecutor.into_executor(())
 /// }
 /// ```
-#[cfg(all(feature = "std", any(unix, doc)))]
+#[cfg(all(feature = "std", unix))]
 pub trait CommandConfigurator<I, C = Child>: Sized {
     /// Get the stdout
     fn stdout_observer(&self) -> Option<Handle<StdOutObserver>> {
@@ -828,6 +820,18 @@ pub trait CommandConfigurator<I, C = Child>: Sized {
     /// Set the timeout duration for execution of the child process.
     fn exec_timeout_mut(&mut self) -> &mut Duration;
 
+    /// Maps the exit status of the child process to an `ExitKind`.
+    #[inline]
+    fn exit_kind_from_status(&self, status: &std::process::ExitStatus) -> ExitKind {
+        use crate::std::os::unix::process::ExitStatusExt;
+        match status.signal() {
+            // for reference: https://www.man7.org/linux/man-pages/man7/signal.7.html
+            Some(9) => ExitKind::Oom,
+            Some(_) => ExitKind::Crash,
+            None => ExitKind::Ok,
+        }
+    }
+
     /// Create an `Executor` from this `CommandConfigurator`.
     fn into_executor<OT, S>(self, observers: OT) -> CommandExecutor<OT, S, Self, (), C>
     where
diff --git a/libafl/src/executors/mod.rs b/libafl/src/executors/mod.rs
index 993a85fe2e..d8c9c345e3 100644
--- a/libafl/src/executors/mod.rs
+++ b/libafl/src/executors/mod.rs
@@ -5,7 +5,7 @@ use alloc::vec::Vec;
 use core::{fmt::Debug, time::Duration};
 
 pub use combined::CombinedExecutor;
-#[cfg(all(feature = "std", any(unix, doc)))]
+#[cfg(all(feature = "std", unix))]
 pub use command::CommandExecutor;
 pub use differential::DiffExecutor;
 #[cfg(all(feature = "std", feature = "fork", unix))]
@@ -23,7 +23,7 @@ pub use with_observers::WithObservers;
 use crate::{observers::ObserversTuple, state::UsesState, Error};
 
 pub mod combined;
-#[cfg(all(feature = "std", any(unix, doc)))]
+#[cfg(all(feature = "std", unix))]
 pub mod command;
 pub mod differential;
 #[cfg(all(feature = "std", feature = "fork", unix))]
diff --git a/libafl/src/observers/stdio.rs b/libafl/src/observers/stdio.rs
index 950e38d5c7..bab111c9d4 100644
--- a/libafl/src/observers/stdio.rs
+++ b/libafl/src/observers/stdio.rs
@@ -2,7 +2,10 @@
 //!
 //! The [`StdOutObserver`] and [`StdErrObserver`] observers look at the stdout of a program
 //! The executor must explicitly support these observers.
-//! For example, they are supported on the [`crate::executors::CommandExecutor`].
+#![cfg_attr(
+    all(feature = "std", unix),
+    doc = r"For example, they are supported on the [`crate::executors::CommandExecutor`]."
+)]
 
 use alloc::borrow::Cow;
 use std::vec::Vec;


Test patch: