[WIP?] QemuSugar Snapshot support (+ OptionalModule) #3341

jma-qb · 2025-07-16T15:36:56Z

Description

Trying to add an optional support for snapshot in Qemu Sugar. @domenukk suggested a IfModule like the IfStage to avoid duplicating a lot of code and pleasing Rust's type system.
I went with a simpler version taking a bool rather than a closure as we probably don't want to dynamically change the list of loaded modules.

I had to fix the SnapshotModule because it couldn't retrieve the module wrapped in the OptionalModule but:

it feels ugly
I want to also allow using the AddressFilter, it will require updating it with the same type of logic
I need to document the module and 2) if we want to include more modules in the future

if let Some(h) = emulator_modules.get_mut::<SnapshotModule>() {
    h.access(addr, SIZE);
} else {
    let snap = emulator_modules.get_mut::<OptionalModule<SnapshotModule>>().unwrap();
    let h = snap.get_inner_module_mut();
    h.access(addr, SIZE);
}

I'm open to any suggestion to improve this and how/where I should document it.

Checklist

[] I have run ./scripts/precommit.sh and addressed all comments

I ran the precommit script but couldn't address all comments.
In this example clippy wants me to merge the two if because it can't see the arm specific code.

        if let Some(h) = emulator_modules.get_mut::<Self>() {
            if !h.must_instrument(pc) {
                return None;
            }

            #[cfg(cpu_target = "arm")]
            h.cs.set_mode(if pc & 1 == 1 {
                capstone::arch::arm::ArchMode::Thumb.into()
            } else {
                capstone::arch::arm::ArchMode::Arm.into()
            })
            .unwrap();
        }

Also when targeting arm so it doesn't complain about this I got a lot of errors

error[E0432]: unresolved import `crate::SYS_execve`
  --> crates/libafl_qemu/src/modules/usermode/injections.rs:22:5
   |
22 | use crate::SYS_execve;
   |     ^^^^^^^^^^^^^^^^^ no `SYS_execve` in the root

error[E0432]: unresolved import `crate::SYS_fstatat64`
  --> crates/libafl_qemu/src/modules/usermode/snapshot.rs:10:5
   |
10 | use crate::SYS_fstatat64;
   |     ^^^^^^^^^^^^^^^^^^^^ no `SYS_fstatat64` in the root

error[E0432]: unresolved import `crate::SYS_mmap2`
  --> crates/libafl_qemu/src/modules/usermode/snapshot.rs:14:5
   |
14 | use crate::SYS_mmap2;
   |     ^^^^^^^^^^^^^^^^ no `SYS_mmap2` in the root

jma-qb · 2025-07-16T15:42:42Z

Forgot to note. It looks like it works when trying from the python bindings on the python_qemu fuzzer's target. The perfs are slightly decreased but I believe this is because the target is so small that the snapshot reset is heavy compared to just reseting a few registers and running again.

Also taking any feedback on how I should test it further.

tokatoka · 2025-07-16T15:49:26Z

imo this should be named as IfModule,
and in addition, can you make this OptionalModule to take an closure: CB instead of enabled: bool just like IfStage, then CB returns true or false to say if the module should be enabled ?

tokatoka · 2025-07-16T15:50:05Z

@domenukk what do you think?
I think making it consistent with IfStage is better

domenukk · 2025-07-16T15:51:34Z

Yes would be good if possible / easy to do

tokatoka · 2025-07-16T17:37:21Z

wait..
I start to see the problem.
yes, indeed it is ugly, and i don't this is good.
IfStage works because there's no code that looks up that stage, but for module it's a different story.

I have to understand what this optional is for, is it because you want to make it configurable at runtime to use the snapshot module in libafl_sugar?

tokatoka · 2025-07-16T17:44:39Z

if that's your goal, how about doing it like

(inside libafl_sugar)
if opt.use_snapshot {
 let module = tuple!(x, y, snapshot);
 // continue other things
}
else {
 let module = tuple!(x, y);
 // continue other things
}

yes, you a lot of duplicate code in this way, but to me it's more reasonable to handle all this in the user's code (libafl_sugar's code)
i don't think complicating the library side code (libafl_qemu) is a good idea.

jma-qb · 2025-07-16T19:23:41Z

if that's your goal, how about doing it like
(inside libafl_sugar)
if opt.use_snapshot {
 let module = tuple!(x, y, snapshot);
 // continue other things
}
else {
 let module = tuple!(x, y);
 // continue other things
}
yes, you a lot of duplicate code in this way, but to me it's more reasonable to handle all this in the user's code (libafl_sugar's code) i don't think complicating the library side code (libafl_qemu) is a good idea.

You're right that's the goal.
The thing is that if you stack more options it becomes a huge mess. There's already duplicated code because of the cmplog feature, I'd like to add Snapshot right now and probably AddressFilter. Later we could add ASAN or other things. It will go exponential.

jma-qb · 2025-07-16T19:32:42Z

imo this should be named as IfModule, and in addition, can you make this OptionalModule to take an closure: CB instead of enabled: bool just like IfStage, then CB returns true or false to say if the module should be enabled ?

Regarding the name, I chose OptionalModule to differentiate with the IfStage which uses a closure.
I didn't use a closure because:

you probably don't want to change the list of modules while fuzzing
there's probably an overhead to call a closure every time you use the module. Especially with modules like the Asan one.

I can do it but it'll require a few changes. Like dropping the Debug bound for the EmulatorModule Trait (doesn't seem important) and might complicate things a bit but it's doable.

Anyway, if you feel it adds too much complexity to the qemu code I'm happy to drop this.

domenukk · 2025-07-16T22:35:09Z

crates/libafl_qemu/src/modules/utils/logic.rs

+use crate::modules::{EmulatorModule, EmulatorModuleTuple};
+
+#[derive(Debug)]
+pub struct OptionalModule<MD> {


If we don't use a closure, this should probably be an Enum

i agree with that.
the way it's written, it looks like the module could be enabled after it has been initialized, which would definitely not work in most cases.

with an enum like

pub enum OptionalModule<MD> { Disabled, Enabled(MD), }

it would make the intention clear.

domenukk · 2025-07-16T22:35:16Z

A closure that checks a single flag should optimize reasonably well, don't you think? Probably emits the same code in the end

rmalmain · 2025-07-25T06:00:17Z

crates/libafl_qemu/src/modules/usermode/snapshot.rs

@@ -842,8 +845,15 @@ pub fn trace_write_snapshot<ET, I, S, const SIZE: usize>(
    I: Unpin,
    S: Unpin,
 {
-    let h = emulator_modules.get_mut::<SnapshotModule>().unwrap();
-    h.access(addr, SIZE);
+    if let Some(h) = emulator_modules.get_mut::<SnapshotModule>() {


i don't think we should remove the unwrap here.
if for some reason we end up there and there is no snapshot module, it's actually a bug.
the comment applies to all the other similar changes.

If the SnapshotModule is wrapped in an OptionalModule this call will fail, so this is done so we have a chance to retrieve the module from inside the OptionalModule.
So the unwrap is kept in the else branch.

yeah i see, i'm not sure the current solution is good.
it forces to do code duplication.
maybe we can replace get_mut with a new method, calling to a method implemented by the trait EmulatorModule, which can return an Option<&mut Self>? it's a bit heavy but at least we can keep the same semantic and avoid duplicating code in callbacks

rmalmain · 2025-07-25T06:04:34Z

crates/libafl_sugar/src/forkserver.rs

@@ -81,6 +81,9 @@ pub struct ForkserverBytesCoverageSugar<'a> {
    /// Fuzz `iterations` number of times, instead of indefinitely; implies use of `fuzz_loop_for`
    #[builder(default = None)]
    iterations: Option<u64>,
+    /// Disable redirection of stdout to /dev/null on unix build targets


why not just use a bool here?

rmalmain · 2025-07-25T06:10:40Z

crates/libafl_qemu/src/modules/utils/logic.rs

+use crate::modules::{EmulatorModule, EmulatorModuleTuple};
+
+#[derive(Debug)]
+pub struct OptionalModule<MD> {


i agree with that.
the way it's written, it looks like the module could be enabled after it has been initialized, which would definitely not work in most cases.

with an enum like

pub enum OptionalModule<MD> { Disabled, Enabled(MD), }

it would make the intention clear.

rmalmain · 2025-07-25T06:14:37Z

crates/libafl_sugar/src/qemu.rs

@@ -88,6 +91,8 @@ where
    /// Disable redirection of stdout to /dev/null on unix build targets
    #[builder(default = None)]


i guess this is why you used Option<bool>.
we can just use bool here as well instead, same for next field

Most of it uses Option<bool> so I mimicked it. Maybe it is done this way because of the python bindings, allowing to have optional parameters. I'm not sure this is the explanation 🤷

we should be able to default to false or smth like that.
i don't think it would break anything, we can try

rmalmain · 2025-07-25T06:25:48Z

if that's your goal, how about doing it like
(inside libafl_sugar)
if opt.use_snapshot {
 let module = tuple!(x, y, snapshot);
 // continue other things
}
else {
 let module = tuple!(x, y);
 // continue other things
}
yes, you a lot of duplicate code in this way, but to me it's more reasonable to handle all this in the user's code (libafl_sugar's code) i don't think complicating the library side code (libafl_qemu) is a good idea.

it's fine the way @jma-qb did it, it's adding a file without touching the internal logic.
cf the code of qemu_launcher, we tried to use fully generic code and it's unreadable:

LibAFL/fuzzers/binary_only/qemu_launcher/src/client.rs

Line 128 in dd3ef2a

if self.options.rerun_input.is_some() {

in term of performance, the extra check would only be noticeable in pre_exec / post_exec, which should be fine in most cases.

tokatoka · 2025-07-25T11:08:26Z

in term of performance, the extra check would only be noticeable in pre_exec / post_exec, which should be fine in most cases.

but for example in trace_write_snapshot doesn't this bring you additional hashmap lookup everytime qemu sees a write? you sure it really is ok?

tokatoka · 2025-07-25T11:09:57Z

https://github.com/AFLplusplus/LibAFL/blob/poc/crates/libafl_sugar/src/qemu.rs

I have a solution using macros 😜

rmalmain · 2025-07-25T21:40:16Z

in term of performance, the extra check would only be noticeable in pre_exec / post_exec, which should be fine in most cases.

but for example in trace_write_snapshot doesn't this bring you additional hashmap lookup everytime qemu sees a write? you sure it really is ok?

which hashmap lookup? if you use snapshot in the same way as before, it will replace let h = ... .unwrap() by if let Some(h) = ..., so it should be equivalent in term of control flow / memory access. in release mode, this should be optimized out since the compiler knows exactly the content of the tuple list, so if the compiler is not too stupid this remove the dead code and just give h without any condition check when snapshot is present.

rmalmain · 2025-07-25T21:45:32Z

https://github.com/AFLplusplus/LibAFL/blob/poc/crates/libafl_sugar/src/qemu.rs

I have a solution using macros 😜

yeah i saw, i don't want to maintain this kind of macro, they tend to become unreadable.
this part (https://github.com/AFLplusplus/LibAFL/blob/poc/crates/libafl_sugar/src/qemu.rs#L356) grows exponentially with the number of booleans to check btw. so if you have let's say 4 optional modules in the future, you have to check for 16 couples of booleans in the match

jma-qb added 3 commits July 16, 2025 14:58

add OptionalModule and sugar support for snapshots

ac8a04a

apply precommit.sh

0e83bab

add enable_stdout to sugar forkserver python bindings

e8c757a

domenukk reviewed Jul 16, 2025

View reviewed changes

Merge branch 'main' into sugar_snapshot

c96d4d1

rmalmain reviewed Jul 25, 2025

View reviewed changes

		@@ -88,6 +91,8 @@ where
		/// Disable redirection of stdout to /dev/null on unix build targets
		#[builder(default = None)]

Uh oh!

[WIP?] QemuSugar Snapshot support (+ OptionalModule) #3341

Are you sure you want to change the base?

[WIP?] QemuSugar Snapshot support (+ OptionalModule) #3341

Uh oh!

Conversation

jma-qb commented Jul 16, 2025

Description

Checklist

Uh oh!

jma-qb commented Jul 16, 2025

Uh oh!

tokatoka commented Jul 16, 2025

Uh oh!

tokatoka commented Jul 16, 2025

Uh oh!

domenukk commented Jul 16, 2025

Uh oh!

tokatoka commented Jul 16, 2025

Uh oh!

tokatoka commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jma-qb commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jma-qb commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenukk commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jma-qb Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmalmain Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmalmain commented Jul 25, 2025

Uh oh!

tokatoka commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tokatoka commented Jul 25, 2025

Uh oh!

rmalmain commented Jul 25, 2025

Uh oh!

rmalmain commented Jul 25, 2025

Uh oh!

Uh oh!

tokatoka commented Jul 16, 2025 •

edited

Loading

jma-qb commented Jul 16, 2025 •

edited

Loading

domenukk commented Jul 16, 2025 •

edited

Loading

jma-qb Jul 25, 2025 •

edited

Loading

rmalmain Jul 25, 2025 •

edited

Loading

tokatoka commented Jul 25, 2025 •

edited

Loading