Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor of Qemu configuration #2707

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
60 changes: 22 additions & 38 deletions libafl_qemu/src/emu/builder.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use std::{fmt::Debug, marker::PhantomData};
use std::{
fmt::{Debug, Formatter},
marker::PhantomData,
};

use libafl::{
inputs::{HasTargetBytes, UsesInput},
Expand All @@ -17,10 +20,9 @@ use crate::{
};

#[derive(Clone, Debug)]
enum QemuBuilder {
Qemu(Qemu),
QemuConfig(QemuConfig),
QemuString(Vec<String>),
pub enum QemuParams {
Config(QemuConfig),
Cli(Vec<String>),
}

#[derive(Clone, Debug)]
Expand All @@ -32,7 +34,7 @@ where
driver: ED,
snapshot_manager: SM,
command_manager: CM,
qemu_builder: Option<QemuBuilder>,
qemu_parameters: Option<QemuParams>,
phantom: PhantomData<S>,
}

Expand All @@ -47,7 +49,7 @@ where
driver: NopEmulatorDriver,
snapshot_manager: NopSnapshotManager,
command_manager: NopCommandManager,
qemu_builder: None,
qemu_parameters: None,
phantom: PhantomData,
}
}
Expand All @@ -67,7 +69,7 @@ where
command_manager: StdCommandManager::default(),
snapshot_manager: StdSnapshotManager::default(),
driver: StdEmulatorDriver::builder().build(),
qemu_builder: None,
qemu_parameters: None,
phantom: PhantomData,
}
}
Expand All @@ -85,7 +87,7 @@ where
command_manager: StdCommandManager::default(),
snapshot_manager: FastSnapshotManager::default(),
driver: StdEmulatorDriver::builder().build(),
qemu_builder: None,
qemu_parameters: None,
phantom: PhantomData,
}
}
Expand All @@ -99,14 +101,14 @@ where
driver: ED,
command_manager: CM,
snapshot_manager: SM,
qemu_builder: Option<QemuBuilder>,
qemu_parameters: Option<QemuParams>,
) -> Self {
Self {
modules,
command_manager,
driver,
snapshot_manager,
qemu_builder,
qemu_parameters,
phantom: PhantomData,
}
}
Expand All @@ -116,20 +118,13 @@ where
CM: CommandManager<ED, ET, S, SM>,
ET: EmulatorModuleTuple<S>,
{
let qemu_builder = self.qemu_builder.ok_or(QemuInitError::EmptyArgs)?;
let qemu_parameters = self.qemu_parameters.ok_or(QemuInitError::EmptyArgs)?;

let mut emulator_hooks = unsafe { EmulatorHooks::new(QemuHooks::get_unchecked()) };

self.modules.pre_qemu_init_all(&mut emulator_hooks);

let qemu: Qemu = match qemu_builder {
QemuBuilder::Qemu(qemu) => qemu,
QemuBuilder::QemuConfig(qemu_config) => {
let res: Result<Qemu, QemuInitError> = qemu_config.into();
res?
}
QemuBuilder::QemuString(qemu_string) => Qemu::init(&qemu_string)?,
};
let qemu = Qemu::init_with_params(&qemu_parameters)?;

unsafe {
Ok(Emulator::new_with_qemu(
Expand All @@ -156,7 +151,7 @@ where
self.driver,
self.command_manager,
self.snapshot_manager,
Some(QemuBuilder::QemuConfig(qemu_config)),
Some(QemuParams::Config(qemu_config)),
)
}

Expand All @@ -167,18 +162,7 @@ where
self.driver,
self.command_manager,
self.snapshot_manager,
Some(QemuBuilder::QemuString(qemu_cli)),
)
}

#[must_use]
pub fn qemu(self, qemu: Qemu) -> EmulatorBuilder<CM, ED, ET, S, SM> {
EmulatorBuilder::new(
self.modules,
self.driver,
self.command_manager,
self.snapshot_manager,
Some(QemuBuilder::Qemu(qemu)),
Some(QemuParams::Cli(qemu_cli)),
)
}

Expand All @@ -192,7 +176,7 @@ where
self.driver,
self.command_manager,
self.snapshot_manager,
self.qemu_builder,
self.qemu_parameters,
)
}

Expand All @@ -202,7 +186,7 @@ where
driver,
self.command_manager,
self.snapshot_manager,
self.qemu_builder,
self.qemu_parameters,
)
}

Expand All @@ -215,7 +199,7 @@ where
self.driver,
command_manager,
self.snapshot_manager,
self.qemu_builder,
self.qemu_parameters,
)
}

Expand All @@ -225,7 +209,7 @@ where
self.driver,
self.command_manager,
self.snapshot_manager,
self.qemu_builder,
self.qemu_parameters,
)
}

Expand All @@ -238,7 +222,7 @@ where
self.driver,
self.command_manager,
snapshot_manager,
self.qemu_builder,
self.qemu_parameters,
)
}
}
43 changes: 13 additions & 30 deletions libafl_qemu/src/qemu/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,13 @@ use core::{
fmt,
fmt::{Display, Formatter},
};
use std::{
path::{Path, PathBuf},
sync::OnceLock,
};
use std::path::{Path, PathBuf};

use getset::Getters;
use libafl_derive;
use strum_macros;
use typed_builder::TypedBuilder;

use crate::{Qemu, QemuInitError};

pub(super) static QEMU_CONFIG: OnceLock<QemuConfig> = OnceLock::new();

#[cfg(feature = "systemmode")]
#[derive(Debug, strum_macros::Display, Clone)]
#[strum(prefix = "-accel ", serialize_all = "lowercase")]
Expand Down Expand Up @@ -304,14 +297,16 @@ impl<R: AsRef<Path>> From<R> for Program {
}

#[derive(Debug, Clone, libafl_derive::Display, TypedBuilder, Getters)]
#[builder(build_method(into = Result<Qemu, QemuInitError>), builder_method(vis = "pub(crate)",
#[builder(builder_method(
vis = "pub(crate)",
rmalmain marked this conversation as resolved.
Show resolved Hide resolved
doc = "Since Qemu is a zero sized struct, this is not a completely standard builder pattern. \
The Qemu configuration is not stored in the Qemu struct after build() but in QEMU_CONFIG \
Therefore, to use the derived builder and avoid boilerplate a builder for QemuConfig is \
derived. \
The QemuConfig::builder is called in Qemu::builder() which is the only place where it should \
rmalmain marked this conversation as resolved.
Show resolved Hide resolved
be called, in this way the one to one matching of Qemu and QemuConfig is enforced. Therefore \
its visibility is pub(crate)"))]
its visibility is pub(crate)"
))]
#[getset(get = "pub")]
pub struct QemuConfig {
#[cfg(feature = "systemmode")]
Expand Down Expand Up @@ -350,40 +345,28 @@ pub struct QemuConfig {
program: Program,
} // Adding something here? Please leave Program as the last field

impl From<QemuConfig> for Result<Qemu, QemuInitError> {
/// This method is necessary to make the API resemble a typical builder pattern, i.e.
/// `Qemu::builder().foo(bar).build()`, while still leveraging `TypedBuilder` for this
/// non-standard use case where `Qemu` doesn't store the configuration.
/// Internally, `TypedBuilder` is used to generate a builder for `QemuConfig`.
/// This `QemuConfig.into()` method is used by the derived `QemuConfigBuilder.build()`
/// to go from `QemuConfigBuilder` to `QemuConfig`, and finally to `Qemu` in one fn.
///
/// # Errors
/// returns `QemuInitError` if the Qemu initialization fails, including cases where Qemu has
/// already been initialized.
fn from(config: QemuConfig) -> Self {
let args = config
impl From<&QemuConfig> for Vec<String> {
/// Generate the QEMU-compatible initialization cli string from the QEMU config.
rmalmain marked this conversation as resolved.
Show resolved Hide resolved
fn from(config: &QemuConfig) -> Self {
config
.to_string()
.split(' ')
.map(ToString::to_string)
.collect::<Vec<String>>();
let qemu = Qemu::init(&args)?;
QEMU_CONFIG
.set(config)
.map_err(|_| unreachable!("BUG: QEMU_CONFIG was already set but Qemu was not init!"))?;
Ok(qemu)
.collect::<Vec<String>>()
}
}

#[cfg(test)]
mod test {
use super::*;
use crate::Qemu;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@domenukk domenukk Nov 22, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@domenukk domenukk Dec 3, 2024

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

let qemu_config = QemuConfig::builder().program("/bin/pwd").build();
let qemu = Qemu::init_with_config(&qemu_config).unwrap();
let config = qemu.get_config().unwrap();
assert_eq!(config.to_string().trim(), program.trim());
}
Expand Down
26 changes: 20 additions & 6 deletions libafl_qemu/src/qemu/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use std::{
mem::MaybeUninit,
ops::Range,
pin::Pin,
sync::OnceLock,
};

use libafl_bolts::os::unix_signals::Signal;
Expand All @@ -28,10 +29,10 @@ use libafl_qemu_sys::{
use num_traits::Num;
use strum::IntoEnumIterator;

use crate::{GuestAddrKind, GuestReg, Regs};
use crate::{GuestAddrKind, GuestReg, QemuParams, Regs};

pub mod config;
use config::{QemuConfig, QemuConfigBuilder, QEMU_CONFIG};
use config::QemuConfig;

#[cfg(feature = "usermode")]
mod usermode;
Expand All @@ -49,6 +50,8 @@ pub use hooks::*;

static mut QEMU_IS_INITIALIZED: bool = false;

pub(super) static QEMU_CONFIG: OnceLock<QemuConfig> = OnceLock::new();

#[derive(Debug)]
pub enum QemuError {
Init(QemuInitError),
Expand Down Expand Up @@ -574,10 +577,21 @@ impl From<u8> for HookData {

#[allow(clippy::unused_self)]
impl Qemu {
/// For more details about the parameters check
/// [the QEMU documentation](https://www.qemu.org/docs/master/about/).
pub fn builder() -> QemuConfigBuilder {
QemuConfig::builder()
pub fn init_with_params(params: &QemuParams) -> Result<Self, QemuInitError> {
match params {
QemuParams::Config(config) => Self::init_with_config(config),
QemuParams::Cli(cli) => Self::init(cli.as_ref()),
}
}

pub fn init_with_config(config: &QemuConfig) -> Result<Self, QemuInitError> {
let qemu_args: Vec<String> = config.into();

QEMU_CONFIG
.set(config.clone())
.map_err(|_| unreachable!("BUG: QEMU_CONFIG was already set but Qemu was not init!"))?;

Self::init(qemu_args.as_ref())
}

#[allow(clippy::must_use_candidate, clippy::similar_names)]
Expand Down
Loading