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

Various improvements to differential fuzzing #4845

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
6 changes: 0 additions & 6 deletions crates/fuzzing/src/generators/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,6 @@ impl Config {
pub fn set_differential_config(&mut self) {
let config = &mut self.module_config.config;

// Disable the start function for now.
//
// TODO: should probably allow this after testing it works with the new
// differential setup in all engines.
config.allow_start_export = false;

// Make it more likely that there are types available to generate a
// function with.
config.min_types = 1;
Expand Down
67 changes: 63 additions & 4 deletions crates/fuzzing/src/oracles/diff_wasmi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use crate::generators::{DiffValue, DiffValueType, ModuleConfig};
use crate::oracles::engine::{DiffEngine, DiffInstance};
use anyhow::{bail, Context, Error, Result};
use wasmtime::Trap;
use wasmtime::{Trap, TrapCode};

/// A wrapper for `wasmi` as a [`DiffEngine`].
pub struct WasmiEngine;
Expand Down Expand Up @@ -36,6 +36,9 @@ impl WasmiEngine {
if config.config.threads_enabled {
bail!("wasmi does not support threads");
}
if config.config.max_memories > 1 {
bail!("wasmi does not support multi-memory");
}
Comment on lines +39 to +41
Copy link
Member

Choose a reason for hiding this comment

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

Instead of bailing out when an engine doesn't support a config, can we have a pre-pass where the engine is given mutable access to the config to turn off anything that it doesn't support? This just seems like better use of fuzzing time than rejecting iterations and bailing out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I've come around to feeling this way as well, I'll work on refactoring to enable this

Ok(Self)
}
}
Expand All @@ -49,13 +52,69 @@ impl DiffEngine for WasmiEngine {
let module = wasmi::Module::from_buffer(wasm).context("unable to validate Wasm module")?;
let instance = wasmi::ModuleInstance::new(&module, &wasmi::ImportsBuilder::default())
.context("unable to instantiate module in wasmi")?;
let instance = instance.assert_no_start();
let instance = instance.run_start(&mut wasmi::NopExternals)?;
Ok(Box::new(WasmiInstance { module, instance }))
}

fn assert_error_match(&self, trap: &Trap, err: Error) {
// TODO: should implement this for `wasmi`
drop((trap, err));
// Acquire a `wasmi::Trap` from the wasmi error which we'll use to
// assert that it has the same kind of trap as the wasmtime-based trap.
let wasmi = match err.downcast::<wasmi::Error>() {
Ok(wasmi::Error::Trap(trap)) => trap,

// Out-of-bounds data segments turn into this category which
// Wasmtime reports as a `MemoryOutOfBounds`.
Ok(wasmi::Error::Memory(msg)) => {
assert_eq!(
trap.trap_code(),
Some(TrapCode::MemoryOutOfBounds),
"wasmtime error did not match wasmi: {msg}"
);
return;
}

// Ignore this for now, looks like "elements segment does not fit"
// falls into this category and to avoid doing string matching this
// is just ignored.
Ok(wasmi::Error::Instantiation(msg)) => {
log::debug!("ignoring wasmi instantiation error: {msg}");
return;
}

Ok(other) => panic!("unexpected wasmi error: {}", other),

Err(err) => err.downcast::<wasmi::Trap>().unwrap(),
};
match wasmi.kind() {
wasmi::TrapKind::StackOverflow => {
assert_eq!(trap.trap_code(), Some(TrapCode::StackOverflow))
}
wasmi::TrapKind::MemoryAccessOutOfBounds => {
assert_eq!(trap.trap_code(), Some(TrapCode::MemoryOutOfBounds))
}
wasmi::TrapKind::Unreachable => {
assert_eq!(trap.trap_code(), Some(TrapCode::UnreachableCodeReached))
}
wasmi::TrapKind::TableAccessOutOfBounds => {
assert_eq!(trap.trap_code(), Some(TrapCode::TableOutOfBounds))
}
wasmi::TrapKind::ElemUninitialized => {
assert_eq!(trap.trap_code(), Some(TrapCode::IndirectCallToNull))
}
wasmi::TrapKind::DivisionByZero => {
assert_eq!(trap.trap_code(), Some(TrapCode::IntegerDivisionByZero))
}
wasmi::TrapKind::IntegerOverflow => {
assert_eq!(trap.trap_code(), Some(TrapCode::IntegerOverflow))
}
wasmi::TrapKind::InvalidConversionToInt => {
assert_eq!(trap.trap_code(), Some(TrapCode::BadConversionToInteger))
}
wasmi::TrapKind::UnexpectedSignature => {
assert_eq!(trap.trap_code(), Some(TrapCode::BadSignature))
}
wasmi::TrapKind::Host(_) => unreachable!(),
}
}
}

Expand Down
9 changes: 6 additions & 3 deletions crates/fuzzing/src/oracles/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub fn choose(
u: &mut Unstructured<'_>,
existing_config: &Config,
allowed: &[&str],
) -> arbitrary::Result<Box<dyn DiffEngine>> {
) -> arbitrary::Result<Option<Box<dyn DiffEngine>>> {
// Filter out any engines that cannot match the `existing_config` or are not
// `allowed`.
let mut engines: Vec<Box<dyn DiffEngine>> = vec![];
Expand Down Expand Up @@ -54,13 +54,16 @@ pub fn choose(
}
}

if engines.is_empty() {
return Ok(None);
}
Comment on lines +57 to +59
Copy link
Member

Choose a reason for hiding this comment

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

Slightly surprised we would ever have a case where we can't run the test case in Wasmtime. What is even the point at that time?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue this is addressing is if testing Wasmtime against Wasmtime has been disabled using the ALLOWED_ENGINES environment variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah this is only applicable when you're doing something like ALLOWED_ENGINES=wasmi locally where I was trying to only differentially-execute against wasmi


// Use the input of the fuzzer to pick an engine that we'll be fuzzing
// Wasmtime against.
assert!(!engines.is_empty());
let index: usize = u.int_in_range(0..=engines.len() - 1)?;
let engine = engines.swap_remove(index);
log::debug!("selected engine: {}", engine.name());
Ok(engine)
Ok(Some(engine))
}

/// Provide a way to instantiate Wasm modules.
Expand Down
9 changes: 7 additions & 2 deletions fuzz/fuzz_targets/differential.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,13 @@ fn run(data: &[u8]) -> Result<()> {
};
log_wasm(&wasm);

// Choose a left-hand side Wasm engine.
let mut lhs = engine::choose(&mut u, &config, unsafe { &ALLOWED_ENGINES })?;
// Choose a left-hand side Wasm engine. If no engine could be chosen then
// that means the configuration selected above doesn't match any allowed
// engine (configured via an env var) so the test case is thrown out.
let mut lhs = match engine::choose(&mut u, &config, unsafe { &ALLOWED_ENGINES })? {
Some(engine) => engine,
None => return Ok(()),
};
let lhs_instance = lhs.instantiate(&wasm);
STATS.bump_engine(lhs.name());

Expand Down