Make executor state available to the harness

[rmalmain]

First attempt to let the possibility to get a mutable reference to some executor-related state in the harness. It seems to work nicely on QEMU fuzzers for now. libafl_sugar seems to cause issues though, I still have to investigate it. Only QEMU-related executors will concretely see a difference for now, but it should be easy to extend this system to use it for other executors if the need arises.

[rmalmain]

The main issue there is for now is that it requires changing harnesses' signatures. I am thinking of a way to wrap this so it does not change the signature for fuzzers unrelated to QEMU.

I pushed the PR to show the current state of this thing, but it's clearly not finalized.

[andreafioraldi]

On discord I described an alternative design with option to pass the executor itself to the closure, i think we should opt for it instead of increasing the type complexity

Il ven 16 feb 2024, 17:18 Dominik Maier @.***> ha scritto:

@.**** commented on this pull request.

In fuzzers/frida_gdiplus/src/fuzzer.rs https://github.com/AFLplusplus/LibAFL/pull/1847#discussion_r1492770973:

@@ -86,7 +86,7 @@ unsafe fn fuzz(options: &FuzzerOptions) -> Result<(), Error> { unsafe extern "C" fn(data: *const u8, size: usize) -> i32, > = lib.get(options.harness_function.as_bytes()).unwrap();

•    let mut frida_harness = |input: &BytesInput| {
  

•    let mut frida_harness = |input: &BytesInput, _executor_state: &mut ()| {
  

I'd add a new method to executors that's ::with_state or something, and takes this new harness (since most of the time we don't care) Alternatively, we could implement From<FnMut<...>> that adapts the closures, maybe?

— Reply to this email directly, view it on GitHub https://github.com/AFLplusplus/LibAFL/pull/1847#pullrequestreview-1885634574, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD3LJ6QGESZZ5SVFIDII7UDYT6IG3AVCNFSM6AAAAABDBVSSVKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOBVGYZTINJXGQ . You are receiving this because your review was requested.Message ID: @.***>

[rmalmain]

@andreafioraldi, a minimized version of the problem here: Playground (It's also on Discord). The core problem is that the harness does not necessarily take a &mut Self as the second argument but may take a super structure containing Self (typically the case with QemuExecutor containing GenericInProcessExecutor containing the harness. In that case, the second argument should be generic if we follow this architecture. Otherwise, it would take GenericInProcessExecutor as the argument). Another solution may be to restructure this, but I'm not sure it would be less involving than modifying the Executor trait.

I agree we should avoid increasing the type complexity if possible. But for now, I couldn't find a satisfying solution different from this one.

[rmalmain]

It should be in good shape now, there should only be minor things to clean up. In the end, I didn't exactly follow your idea @domenukk. Instead, I created a struct for harnesses with state and kept the original one without state in the harness signature. I made the internal state implement the common behavior to avoid code copy, it should be reasonable. This solution keeps the original harnesses unchanged and allows for the possibility of using harnesses with the executor's state mutable reference when necessary. I am not sure the names I chose are the best (...ExecutorWithState seems confusing, ideas welcome). The most annoying part of this proposition is that it changes the signature of run_target and adds (a bit of) complexity to the Executor trait. Most of the time, this method is not called directly by high-level code, so I think it's not a big problem in practice. This solution has no performance overhead and allows to extend existing Executors that may want to access some internal state at runtime easily.

[tokatoka]

i think i asked this before but i forgot. what was the reason that you had to change the run_target's signature?

[tokatoka]

like if you need it can you not store it as a "state" and fetch it at the beginning of run_target? then you don't have to change run_target's signature. only the internal of state-having-executor

[tokatoka]

it's strange to have stage as a argument and then executor_state as another argument

[rmalmain]

To pass the reference of the state to expose to the internal Executor. Example: QemuExecutor contains an InProcessExecutor and calls its run_target function in its own run_target definition. There are 2 common ways to give the internal state of QemuExecutor to the InProcessExecutor: store a reference in InProcessExecutor to the state (not a good solution IMHO) or give it as an argument of the function (implemented solution).

[tokatoka]

Is it possible to make QemuExecutor contains another InProcessWithStateExecutor, which, in his custom run_target() implementation, fetches the metadata from State and pass to harness?

[rmalmain]

If you do this, there will be an overhead no? Or you need to store some kind of reference to the state to expose, and then have a self-referencing struct and use pin, right?

[tokatoka]

accessing the metadata is hashmap lookup O(1) so i think it's fine..

[tokatoka]

i don't know what you save into executor_state, but if you want preserve this executor_state across restarts (i.e. if you don't want to lose it after crashes/timeouts), you need to store it into state anyways.

[rmalmain]

If we use metadata the executor state is then owned by the map, so even when we need to access the executor state for other things, we need to perform a fetch. It's O(1), but if we need to fetch the state a lot, it can still have some impact. Idk how much we need to perform fetches to the state though, so maybe it's negligible?

[tokatoka]

If you are talking about the "fetch the state (a lot during one single execution)" no i think you can just pass the reference &mut YourMetadataTrait to the harness then, after that your later access to it will not have lookup.

[tokatoka]

also is it not possible to simply add another member into InProcessWithStateExecutor and make this guy have some member like self.my_executor_state: EM?

[rmalmain]

also is it not possible to simply add another member into InProcessWithStateExecutor and make this guy have some member like self.my_executor_state: EM?

Hum, then in that case the state of QemuExecutor would be held by InProcessExecutorWithState? It sounds strange but I think it would work out. It's better than putting it in metadata IMHO. It also means that QemuExecutor would need to access its state through InProcessExecutorWithState all the time, but I think it's not a problem.

[tokatoka]

actually depends on you. if you think your state needs to be saved across restarts (in the future), then you have to make it into a metadata

[rmalmain]

I don't think we need this, QemuExecutorState is just a wrapper around the old members of QemuExecutor and they were not stored as metadata afaik.

[rmalmain]

Closed in favor of #1900.