Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 112 additions & 2 deletions crates/libafl/src/executors/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,15 +380,17 @@ where
T: CommandConfigurator<Child> + HasTimeout + Debug,
OT: ObserversTuple<I, S>,
{
fn execute_input_with_command<TB: ToTargetBytesConverter<I, S>>(
/// Use this when wrapping with `WithObservers` - build `CommandExecutor` with
/// empty observers `()`, wrap with `WithObservers`, and let the fuzzer manage
/// the observer lifecycle.
fn execute_command_only<TB: ToTargetBytesConverter<I, S>>(
&mut self,
target_bytes_converter: &mut TB,
state: &mut S,
input: &I,
) -> Result<ExitKind, Error> {
use wait_timeout::ChildExt;

self.observers_mut().pre_exec_all(state, input)?;
*state.executions_mut() += 1;
let mut child = self
.configurator
Expand Down Expand Up @@ -427,6 +429,19 @@ where
self.observers_mut().index_mut(&stdout_handle).observe(buf);
}

Ok(exit_kind)
}

fn execute_input_with_command<TB: ToTargetBytesConverter<I, S>>(
&mut self,
target_bytes_converter: &mut TB,
state: &mut S,
input: &I,
) -> Result<ExitKind, Error> {
self.observers_mut().pre_exec_all(state, input)?;

let exit_kind = self.execute_command_only(target_bytes_converter, state, input)?;

self.observers_mut()
.post_exec_child_all(state, input, &exit_kind)?;
Ok(exit_kind)
Comment on lines +441 to 447
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execute_input_with_command calls self.observers_mut().pre_exec_all(...) and then calls execute_command_only, which currently also calls pre_exec_all(...). This results in a guaranteed double pre_exec for every command execution (even without WithObservers), which can corrupt observer state and adds overhead. Make execute_command_only truly “command only” by removing observer lifecycle calls from it, and keep pre_exec_all in only one place (either the caller or the callee, but not both).

Suggested change
self.observers_mut().pre_exec_all(state, input)?;
let exit_kind = self.execute_command_only(target_bytes_converter, state, input)?;
self.observers_mut()
.post_exec_child_all(state, input, &exit_kind)?;
Ok(exit_kind)
self.execute_command_only(target_bytes_converter, state, input)

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -955,4 +970,99 @@ mod tests {

assert!(executor.observers.0.output.is_some());
}

/// Test that WithObservers correctly calls pre_exec and post_exec on an observer list (tuple)
#[test]
#[cfg_attr(miri, ignore)]
fn test_with_observers() {
use libafl_bolts::tuples::{Handled, MatchNameRef};
use tuple_list::tuple_list;

use crate::{
executors::{HasObservers, WithObservers},
observers::TimeObserver,
};

let mut mgr: SimpleEventManager<NopInput, _, NopState<NopInput>> =
SimpleEventManager::new(SimpleMonitor::new(|status| {
log::info!("{status}");
}));

// Create CommandExecutor with EMPTY observers
#[cfg(windows)]
let executor = CommandExecutor::builder()
.program("cmd")
.arg("/c")
.arg("echo")
.input(InputLocation::Arg { argnum: 2 });
#[cfg(not(windows))]
let executor = CommandExecutor::builder()
.program("echo")
.input(InputLocation::Arg { argnum: 0 });

let executor = executor.build::<BytesInput, (), _>(()).unwrap();

// Assert that the CommandExecutor has NO observers (empty tuple)
{
use crate::executors::HasObservers;
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In test_with_observers, the inner block re-imports crate::executors::HasObservers even though it is already imported at the top of the function (use crate::executors::{HasObservers, WithObservers}), making the inner use redundant. With #![deny(unused)] enabled for tests, this redundant import is likely to trigger an unused_imports error. Removing the inner use crate::executors::HasObservers; should avoid the lint failure.

Suggested change
use crate::executors::HasObservers;

Copilot uses AI. Check for mistakes.
let inner_observers: &() = &*executor.observers();
assert_eq!(
inner_observers,
&(),
"CommandExecutor should have empty observers when used with WithObservers"
);
}

let time_observer1 = TimeObserver::new("time1");
let time_observer2 = TimeObserver::new("time2");
let handle1 = time_observer1.handle();
let handle2 = time_observer2.handle();

// Verify observers start with no recorded runtime (pre_exec not yet called)
assert!(
time_observer1.last_runtime().is_none(),
"Observer 1 should have no runtime before execution"
);
assert!(
time_observer2.last_runtime().is_none(),
"Observer 2 should have no runtime before execution"
);

let observer_list = tuple_list!(time_observer1, time_observer2);
let mut executor = WithObservers::new(executor, observer_list);

let mut fuzzer: NopFuzzer = NopFuzzer::new();
let mut state = NopState::<NopInput>::new();
executor
.run_target(
&mut fuzzer,
&mut state,
&mut mgr,
&BytesInput::new(b"test".to_vec()),
)
.unwrap();

// Verify BOTH observers had pre_exec and post_exec called
let observers = executor.observers();

let time_obs1: &TimeObserver = observers.get(&handle1).as_ref().unwrap();
assert!(
time_obs1.last_runtime().is_some(),
"Observer 1: pre_exec and post_exec should have been called (last_runtime is set)"
);

let time_obs2: &TimeObserver = observers.get(&handle2).as_ref().unwrap();
assert!(
time_obs2.last_runtime().is_some(),
"Observer 2: pre_exec and post_exec should have been called (last_runtime is set)"
);

// Both observers should have recorded similar runtimes (same execution)
let runtime1 = time_obs1.last_runtime().unwrap();
let runtime2 = time_obs2.last_runtime().unwrap();
assert!(
runtime1.as_micros() > 0 || runtime2.as_micros() > 0,
"At least one observer should have recorded non-zero runtime"
);
Comment on lines +1059 to +1066
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion that at least one runtime has as_micros() > 0 can be flaky on fast systems/VMs where the echo process may complete within the same microsecond, yielding a Duration that rounds down to 0µs. Since the test already asserts both observers have last_runtime().is_some(), consider dropping the non-zero micros assertion or using a weaker check (e.g., as_nanos() > 0 or simply validating both are Some).

Suggested change
// Both observers should have recorded similar runtimes (same execution)
let runtime1 = time_obs1.last_runtime().unwrap();
let runtime2 = time_obs2.last_runtime().unwrap();
assert!(
runtime1.as_micros() > 0 || runtime2.as_micros() > 0,
"At least one observer should have recorded non-zero runtime"
);

Copilot uses AI. Check for mistakes.
}
}
6 changes: 5 additions & 1 deletion crates/libafl/src/executors/with_observers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub struct WithObservers<E, I, OT, S> {
impl<E, EM, I, OT, S, Z> Executor<EM, I, S, Z> for WithObservers<E, I, OT, S>
where
E: Executor<EM, I, S, Z>,
OT: ObserversTuple<I, S>,
{
fn run_target(
&mut self,
Expand All @@ -29,7 +30,10 @@ where
mgr: &mut EM,
input: &I,
) -> Result<ExitKind, Error> {
self.executor.run_target(fuzzer, state, mgr, input)
self.observers.pre_exec_all(state, input)?;
let exit_kind = self.executor.run_target(fuzzer, state, mgr, input)?;
self.observers.post_exec_all(state, input, &exit_kind)?;
Ok(exit_kind)
Comment on lines +33 to +36
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithObservers::run_target now calls pre_exec_all/post_exec_all on its observer tuple. StdFuzzer::execute_input already calls these for any Executor + HasObservers (see crates/libafl/src/fuzzer/mod.rs:1328-1338), so wrapping an executor with WithObservers will invoke observer lifecycle twice during normal fuzzing. This can skew observer state/metrics and adds overhead. Consider reverting WithObservers::run_target back to delegating only, and address the CommandExecutor/WithObservers mismatch in CommandExecutor (or via a CommandExecutor-specific wrapper) without duplicating the global observer lifecycle.

Suggested change
self.observers.pre_exec_all(state, input)?;
let exit_kind = self.executor.run_target(fuzzer, state, mgr, input)?;
self.observers.post_exec_all(state, input, &exit_kind)?;
Ok(exit_kind)
self.executor.run_target(fuzzer, state, mgr, input)

Copilot uses AI. Check for mistakes.
}
}

Expand Down
Loading