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 16 commits into
base: main
Choose a base branch
from
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
Collaborator Author

Choose a reason for hiding this comment

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

but it works like any other builder, right? it's just that the builder doesn't build directly qemu, but qemu's config instead.
i think QemuConfig::builder makes it quite clear what we are building is not QEMU, right?

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