Refactor of Qemu configuration #2707

rmalmain · 2024-11-19T15:29:45Z

No description provided.

libafl_qemu/src/qemu/config.rs

domenukk · 2024-11-19T17:23:52Z

libafl_qemu/src/qemu/config.rs


    #[test]
    #[cfg(feature = "usermode")]
    fn usermode() {
        let program = "/bin/pwd";
-        let qemu = Qemu::builder().program("/bin/pwd").build().unwrap();


Doens't the old API look better here? Can't we just keep Qemu::builder() and .build() then calls Qemu::init_whatever?

That's true, but the thing is that we need to separate the configuration step from the init step, and the builder generated by typed-builder is not made for this, it's a pain to move around

IMHO it's more important to have a good API than to have nice code "on the inside".
This is already what you're using (right?) idanarye/rust-typed-builder#80

from the user's point of view, i think separating configuration from actual initialization is not so bad as well.
it makes more clear what the configuration part is and where qemu is actually getting initialized.

hiding something like qemu initialization in a From implementation doesn't sound good imho

No it's really bad for users. We have builders everywhere in the codebase and they all work the same. There's no reason this shoudl work differently. You can still pass around an un-built builder

Why not migrate all uses of Qemu to Emulator directly? I (as a user) don't get the split between those two

I mean that's probably beyond the scope of this PR...

@domenukk the idea of Qemu is to have a 0-sized object giving a slightly higher-level interface to QEMU (basically making it more convenient to call to QEMU functions and providing a more rusty api), that can only be obtained when QEMU is correctly initialized.
i use this trick a lot to give a Qemu as parameter for 0 cost.
also (i may be wrong), i think it's nice to let the possibility to use QEMU without all the things in Emulator and still provide a usable API.

in practice, i think you're right and most people will not want to use Qemu alone at all. the separation mostly exists to make it simpler to separate things and make it more clear what is specific to QEMU and what is higher-level stuff we introduce. A possible thing we can try is to move Qemu and QemuHooks to libafl_qemu_sys? at least it would remove the confusion between Emulator and Qemu for users.

@Marcondiro i thought about this, but i think it will be quite heavy to do in practice because rust-typed-builder adds up many generics internally that would be difficult to annotate explicitly.

Why not make a builder for emulator instead, and remove qemu from the user-facing APIs then?

Also, you're not married to typed_builder if it dropping it makes the API cleaner

QemuConfig is quite big, not using something like typed_builder would be a lot of work and make it less maintainable. idk of any other typed builder like this one.
EmulatorBuilder already exists btw, it's used to set things like the modules, snapshot engine, etc...
QemuConfig is just a programmatic way to set the cli of QEMU. if we continue to use the derive macro for getting qemu cli from QemuConfig, we need all the fields of the builder to be an option of the QEMU cli, hence the current split.

libafl_qemu/src/qemu/config.rs

* back to one QEMU init function * other small things

* qemu is now a parameter to EmulatorModule callbacks and most function hooks. * EmulatorModules is initialized before QEMU is initialized.

rmalmain · 2024-11-26T15:50:34Z

some things we should do at some point:

i think we should make Emulator an argument to EmulatorModule and remove the self reference. otherwise (it's an old problem) each EmulatorModule has a double mut reference to itself (one through &mut self and one through EmulatorModules).
we should find a better way to type hooks callbacks. for now they are declared 2 times (both in rust and c side) and can easily get messed up. also, it's quite verbose to create new hooks. imho we should create some kind of builder on c side and simplify the rust side with more macros.

* continue to propagate qemu argument as hook first parameter * use pre_syscall* and post_syscall* everywhere * fix some clippy stuff

…d use the module interface instead. * adapt qemu_launcher example to fully work with emulator, since qemu must now be initialized by emulator.

rmalmain · 2024-11-27T18:38:35Z

this pr has an important breaking change: qemu cannot be initialized independently from emulator. if emulator is needed, qemu must be initialized by emulator (before it was possible to initialize qemu and give it to emulator).
this is because we now have callbacks for things happening before qemu gets initialized, so obviously we cannot initialize it before.
i changed the way asan stuff gets initialized, it should be more natural (i removed the custom init of qemu for asan and moved it to module callbacks).

Marcondiro · 2024-12-05T08:54:16Z

@rmalmain can you merge main? thanks

tokatoka · 2024-12-05T13:49:09Z

Can you write into MIGRATION.md about what you changed in this PR right before you merge this?

rmalmain · 2024-12-05T14:35:34Z

i'm still editing stuff, i'll write when the pr is over

rmalmain · 2024-12-05T16:36:42Z

new proposal for the emulator builder:

do not use TypedBuilder anymore, and replace it with a 'runtime' builder. otherwise i cannot really use the builder around because of the generics problem.
add qemu_config method to EmulatorBuilder that takes a closure as argument with the current qemu configurator as argument and must return the new configurator. that way, ppl can configure qemu from emulator without too much trouble. not sure it's the best solution, but i think it's a bit more usable like this.

opinion @domenukk @Marcondiro ?

Marcondiro · 2024-12-06T08:54:07Z

Hey, probably a stupid question, wasn't it enough to add an empty pub(crate) new() -> Self for Qemu so that the QemuConfig could have remained (almost) unchanged and the init fn called at the right moment by emu?

(Also, should we make Qemu::init less pub anyway now?)

rmalmain · 2024-12-06T09:29:54Z

Hey, probably a stupid question, wasn't it enough to add an empty pub(crate) new() -> Self for Qemu so that the QemuConfig could have remained (almost) unchanged and the init fn called at the right moment by emu?

i'm not sure to understand. do you mean QemuBuilder in the original implementation should have returned a Qemu from a new (empty) function, and init called later on by EmulatorBuilder?

(Also, should we make Qemu::init less pub anyway now?)

if Qemu was not pub, it would be much more simple indeed. i'm trying to keep it pub because i think there could be usecases for Qemu without an Emulator. what i think we can do though is to move it (with QemuHooks) to libafl_qemu_sys to make it less confusing for users what should be used in most cases.

Marcondiro · 2024-12-06T09:34:08Z

i'm not sure to understand. do you mean QemuBuilder in the original implementation should have returned a Qemu from a new (empty) function, and init called later on by EmulatorBuilder?

yeah exactly, in the From that converts QemuConfig -> Qemu we can simply call a dummy Qemu::new() and then (a modified) Qemu::init() -> Result<(), ...> in Emu

rmalmain · 2024-12-06T09:37:35Z

yeah exactly, in the From that converts QemuConfig -> Qemu we can simply call a dummy Qemu::new() and then (a modified) Qemu::init() -> Result<(), ...> in Emu

hum i don't think we want to do that. it gives access to all the Qemu functions that only make sense after init is called. i think we really want to give Qemu when it's ready to be fully usable.

Marcondiro · 2024-12-06T10:28:05Z

Ok got it thanks!

If the Qemu "direct" initialization is gonna be a more "advanced", hidden and less used API, I would care more about the EmulatorBuilder API than the Qemu's init/builder/build/init_with_config. Instead to me it looks like we are doing the opposite, the closure thing is not very handy imho...

I think as a user of EmulatorBuilder I'd like to just use .qemu_config(my_qemu_config), and that's it.
Would this be doable?
Like for instance making .qemu_config take T: Into<QemuParam> and implementing From<QemuConfig> (or From<QemuConfigBuilder>) and From<Vec[String]> for QemuParam

Sorry if I'm missing something :D

qemu config refactoring

af20343

Marcondiro reviewed Nov 19, 2024

View reviewed changes

libafl_qemu/src/qemu/config.rs Outdated Show resolved Hide resolved

domenukk reviewed Nov 19, 2024

View reviewed changes

Marcondiro reviewed Nov 20, 2024

View reviewed changes

libafl_qemu/src/qemu/config.rs Outdated Show resolved Hide resolved

Marcondiro reviewed Nov 20, 2024

View reviewed changes

libafl_qemu/src/qemu/config.rs Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

Merge branch 'main' into qemu_builder_update

5fa831a

This comment was marked as resolved.

Sign in to view

rmalmain added 3 commits November 25, 2024 11:58

* QEMU error refactoring*

28655d8

* back to one QEMU init function * other small things

update test

b85ab06

Light refactor of EmulatorModules

47d2741

* qemu is now a parameter to EmulatorModule callbacks and most function hooks. * EmulatorModules is initialized before QEMU is initialized.

rmalmain added 10 commits November 26, 2024 17:02

Merge branch 'main' into qemu_builder_update

0d37b78

fmt

1c61b38

* asan fixed size accesses working with generics

525fd11

* continue to propagate qemu argument as hook first parameter * use pre_syscall* and post_syscall* everywhere * fix some clippy stuff

fmt

5346f83

fix fuzzers

17059d7

* refactor asan and asanguest modules to avoid custom init of QEMU an…

c50ba05

…d use the module interface instead. * adapt qemu_launcher example to fully work with emulator, since qemu must now be initialized by emulator.

fmt

8d8631e

clippy

f2491c1

Merge branch 'main' into qemu_builder_update

a12dc91

fix qemu_coverage

76480c1

rmalmain added 5 commits November 27, 2024 19:39

fmt

46f0da7

forgot qemu args in launcher

3464e44

use correct args

005475b

adapt api

ea840cf

testing stuff

2998445

Merge branch 'main' into qemu_builder_update

c5b3e19

new builder?

0fe2977

rmalmain added 2 commits December 5, 2024 16:44

use closure for qemu config from emulator builder.

b7f5240

better format

413767f

clippy + fmt

8b04e3f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of Qemu configuration #2707

Refactor of Qemu configuration #2707

rmalmain commented Nov 19, 2024

domenukk Nov 19, 2024

Marcondiro Nov 20, 2024

domenukk Nov 20, 2024

rmalmain Nov 22, 2024

domenukk Nov 22, 2024 •

edited

Loading

domenukk Dec 3, 2024 •

edited

Loading

rmalmain Dec 3, 2024

domenukk Dec 3, 2024

domenukk Dec 3, 2024

rmalmain Dec 3, 2024

This comment was marked as outdated.

This comment was marked as resolved.

rmalmain commented Nov 26, 2024

rmalmain commented Nov 27, 2024

Marcondiro commented Dec 5, 2024

tokatoka commented Dec 5, 2024

rmalmain commented Dec 5, 2024

rmalmain commented Dec 5, 2024

Marcondiro commented Dec 6, 2024 •

edited

Loading

rmalmain commented Dec 6, 2024

Marcondiro commented Dec 6, 2024

rmalmain commented Dec 6, 2024

Marcondiro commented Dec 6, 2024 •

edited

Loading

Refactor of Qemu configuration #2707

Are you sure you want to change the base?

Refactor of Qemu configuration #2707

Conversation

rmalmain commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

domenukk Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as resolved.

rmalmain commented Nov 26, 2024

rmalmain commented Nov 27, 2024

Marcondiro commented Dec 5, 2024

tokatoka commented Dec 5, 2024

rmalmain commented Dec 5, 2024

rmalmain commented Dec 5, 2024

Marcondiro commented Dec 6, 2024 • edited Loading

rmalmain commented Dec 6, 2024

Marcondiro commented Dec 6, 2024

rmalmain commented Dec 6, 2024

Marcondiro commented Dec 6, 2024 • edited Loading

domenukk Nov 22, 2024 •

edited

Loading

domenukk Dec 3, 2024 •

edited

Loading

Marcondiro commented Dec 6, 2024 •

edited

Loading

Marcondiro commented Dec 6, 2024 •

edited

Loading