From 10dbb19983cf77b5d8c92ca93fa9b4ede3c94fc9 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 2 Sep 2022 14:16:02 -0500 Subject: [PATCH] Various improvements to differential fuzzing (#4845) * Improve wasmi differential fuzzer * Support modules with a `start` function * Implement trap-matching to ensure that wasmi and Wasmtime both report the same flavor of trap. * Support differential fuzzing where no engines match Locally I was attempting to run against just one wasm engine with `ALLOWED_ENGINES=wasmi` but the fuzzer quickly panicked because the generated test case didn't match wasmi's configuration. This commit updates engine-selection in the differential fuzzer to return `None` if no engine is applicable, throwing out the test case. This won't be hit at all with oss-fuzz-based runs but for local runs it'll be useful to have. * Improve proposal support in differential fuzzer * De-prioritize unstable wasm proposals such as multi-memory and memory64 by making them more unlikely with `Unstructured::ratio`. * Allow fuzzing multi-table (reference types) and multi-memory by avoiding setting their maximums to 1 in `set_differential_config`. * Update selection of the pooling strategy to unconditionally support the selected module config rather than the other way around. * Improve handling of traps in differential fuzzing This commit fixes an issue found via local fuzzing where engines were reporting different results but the underlying reason for this was that one engine was hitting stack overflow before the other. To fix the underlying issue I updated the execution to check for stack overflow and, if hit, it discards the entire fuzz test case from then on. The rationale behind this is that each engine can have unique limits for stack overflow. One test case I was looking at for example would stack overflow at less than 1000 frames with epoch interruption enabled but would stack overflow at more than 1000 frames with it disabled. This means that the state after the trap started to diverge and it looked like the engines produced different results. While I was at it I also improved the "function call returned a trap" case to compare traps to make sure the same trap reason popped out. * Fix fuzzer tests --- crates/fuzzing/src/generators/config.rs | 34 ++++---- crates/fuzzing/src/generators/module.rs | 16 ++-- crates/fuzzing/src/oracles.rs | 35 ++++++--- crates/fuzzing/src/oracles/diff_spec.rs | 8 +- crates/fuzzing/src/oracles/diff_v8.rs | 10 ++- crates/fuzzing/src/oracles/diff_wasmi.rs | 86 +++++++++++++++++++-- crates/fuzzing/src/oracles/diff_wasmtime.rs | 15 +++- crates/fuzzing/src/oracles/engine.rs | 15 +++- fuzz/fuzz_targets/differential.rs | 28 +++++-- 9 files changed, 192 insertions(+), 55 deletions(-) diff --git a/crates/fuzzing/src/generators/config.rs b/crates/fuzzing/src/generators/config.rs index 14463dfa78..6ee459a7af 100644 --- a/crates/fuzzing/src/generators/config.rs +++ b/crates/fuzzing/src/generators/config.rs @@ -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; @@ -54,7 +48,6 @@ impl Config { // Allow a memory to be generated, but don't let it get too large. // Additionally require the maximum size to guarantee that the growth // behavior is consistent across engines. - config.max_memories = 1; config.max_memory_pages = 10; config.memory_max_size_required = true; @@ -65,7 +58,6 @@ impl Config { // // Note that while reference types are disabled below, only allow one // table. - config.max_tables = 1; config.max_table_elements = 1_000; config.table_max_size_required = true; @@ -86,10 +78,10 @@ impl Config { } = &mut self.wasmtime.strategy { // One single-page memory - limits.memories = 1; + limits.memories = config.max_memories as u32; limits.memory_pages = 10; - limits.tables = 1; + limits.tables = config.max_tables as u32; limits.table_elements = 1_000; limits.size = 1_000_000; @@ -329,16 +321,22 @@ impl<'a> Arbitrary<'a> for Config { if let InstanceAllocationStrategy::Pooling { instance_limits: limits, .. - } = &config.wasmtime.strategy + } = &mut config.wasmtime.strategy { + let cfg = &mut config.module_config.config; // If the pooling allocator is used, do not allow shared memory to // be created. FIXME: see // https://github.com/bytecodealliance/wasmtime/issues/4244. - config.module_config.config.threads_enabled = false; + cfg.threads_enabled = false; // Force the use of a normal memory config when using the pooling allocator and // limit the static memory maximum to be the same as the pooling allocator's memory // page limit. + if cfg.max_memory_pages < limits.memory_pages { + limits.memory_pages = cfg.max_memory_pages; + } else { + cfg.max_memory_pages = limits.memory_pages; + } config.wasmtime.memory_config = match config.wasmtime.memory_config { MemoryConfig::Normal(mut config) => { config.static_memory_maximum_size = Some(limits.memory_pages * 0x10000); @@ -351,14 +349,10 @@ impl<'a> Arbitrary<'a> for Config { } }; - let cfg = &mut config.module_config.config; - cfg.max_memories = limits.memories as usize; - cfg.max_tables = limits.tables as usize; - cfg.max_memory_pages = limits.memory_pages; - - // Force no aliases in any generated modules as they might count against the - // import limits above. - cfg.max_aliases = 0; + // Force this pooling allocator to always be able to accommodate the + // module that may be generated. + limits.memories = cfg.max_memories as u32; + limits.tables = cfg.max_tables as u32; } Ok(config) diff --git a/crates/fuzzing/src/generators/module.rs b/crates/fuzzing/src/generators/module.rs index 2c608acb48..a33feb1db3 100644 --- a/crates/fuzzing/src/generators/module.rs +++ b/crates/fuzzing/src/generators/module.rs @@ -17,21 +17,27 @@ impl<'a> Arbitrary<'a> for ModuleConfig { fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result { let mut config = SwarmConfig::arbitrary(u)?; - // Allow multi-memory by default. - config.max_memories = config.max_memories.max(2); + // Allow multi-memory but make it unlikely + if u.ratio(1, 20)? { + config.max_memories = config.max_memories.max(2); + } else { + config.max_memories = 1; + } // Allow multi-table by default. - config.max_tables = config.max_tables.max(4); + if config.reference_types_enabled { + config.max_tables = config.max_tables.max(4); + } // Allow enabling some various wasm proposals by default. Note that // these are all unconditionally turned off even with // `SwarmConfig::arbitrary`. - config.memory64_enabled = u.arbitrary()?; + config.memory64_enabled = u.ratio(1, 20)?; // Allow the threads proposal if memory64 is not already enabled. FIXME: // to allow threads and memory64 to coexist, see // https://github.com/bytecodealliance/wasmtime/issues/4267. - config.threads_enabled = !config.memory64_enabled && u.arbitrary()?; + config.threads_enabled = !config.memory64_enabled && u.ratio(1, 20)?; Ok(ModuleConfig { config }) } diff --git a/crates/fuzzing/src/oracles.rs b/crates/fuzzing/src/oracles.rs index 45c05858c9..98845a19c4 100644 --- a/crates/fuzzing/src/oracles.rs +++ b/crates/fuzzing/src/oracles.rs @@ -19,7 +19,7 @@ pub mod engine; mod stacks; use self::diff_wasmtime::WasmtimeInstance; -use self::engine::DiffInstance; +use self::engine::{DiffEngine, DiffInstance}; use crate::generators::{self, DiffValue, DiffValueType}; use arbitrary::Arbitrary; pub use stacks::check_stacks; @@ -330,24 +330,28 @@ pub fn instantiate_with_dummy(store: &mut Store, module: &Module) - /// Evaluate the function identified by `name` in two different engine /// instances--`lhs` and `rhs`. /// +/// Returns `Ok(true)` if more evaluations can happen or `Ok(false)` if the +/// instances may have drifted apart and no more evaluations can happen. +/// /// # Panics /// /// This will panic if the evaluation is different between engines (e.g., /// results are different, hashed instance is different, one side traps, etc.). pub fn differential( lhs: &mut dyn DiffInstance, + lhs_engine: &dyn DiffEngine, rhs: &mut WasmtimeInstance, name: &str, args: &[DiffValue], result_tys: &[DiffValueType], -) -> anyhow::Result<()> { +) -> anyhow::Result { log::debug!("Evaluating: `{}` with {:?}", name, args); let lhs_results = match lhs.evaluate(name, args, result_tys) { Ok(Some(results)) => Ok(results), Err(e) => Err(e), // this engine couldn't execute this type signature, so discard this // execution by returning success. - Ok(None) => return Ok(()), + Ok(None) => return Ok(true), }; log::debug!(" -> results on {}: {:?}", lhs.name(), &lhs_results); @@ -360,11 +364,24 @@ pub fn differential( match (lhs_results, rhs_results) { // If the evaluation succeeds, we compare the results. (Ok(lhs_results), Ok(rhs_results)) => assert_eq!(lhs_results, rhs_results), - // Both sides failed--this is an acceptable result (e.g., both sides - // trap at a divide by zero). We could compare the error strings perhaps - // (since the `lhs` and `rhs` could be failing for different reasons) - // but this seems good enough for now. - (Err(_), Err(_)) => {} + + // Both sides failed. If either one hits a stack overflow then that's an + // engine defined limit which means we can no longer compare the state + // of the two instances, so `false` is returned and nothing else is + // compared. + // + // Otherwise, though, the same error should have popped out and this + // falls through to checking the intermediate state otherwise. + (Err(lhs), Err(rhs)) => { + let err = rhs.downcast::().expect("not a trap"); + let poisoned = err.trap_code() == Some(TrapCode::StackOverflow) + || lhs_engine.is_stack_overflow(&lhs); + + if poisoned { + return Ok(false); + } + lhs_engine.assert_error_match(&err, &lhs); + } // A real bug is found if only one side fails. (Ok(_), Err(_)) => panic!("only the `rhs` ({}) failed for this input", rhs.name()), (Err(_), Ok(_)) => panic!("only the `lhs` ({}) failed for this input", lhs.name()), @@ -392,7 +409,7 @@ pub fn differential( panic!("memories have differing values"); } - Ok(()) + Ok(true) } /// Invoke the given API calls. diff --git a/crates/fuzzing/src/oracles/diff_spec.rs b/crates/fuzzing/src/oracles/diff_spec.rs index d545d24ce0..d90b8d32bd 100644 --- a/crates/fuzzing/src/oracles/diff_spec.rs +++ b/crates/fuzzing/src/oracles/diff_spec.rs @@ -59,10 +59,16 @@ impl DiffEngine for SpecInterpreter { })) } - fn assert_error_match(&self, trap: &Trap, err: Error) { + fn assert_error_match(&self, trap: &Trap, err: &Error) { // TODO: implement this for the spec interpreter drop((trap, err)); } + + fn is_stack_overflow(&self, err: &Error) -> bool { + // TODO: implement this for the spec interpreter + drop(err); + false + } } struct SpecInstance { diff --git a/crates/fuzzing/src/oracles/diff_v8.rs b/crates/fuzzing/src/oracles/diff_v8.rs index f0e1a1a38c..d5d7ac0cb8 100644 --- a/crates/fuzzing/src/oracles/diff_v8.rs +++ b/crates/fuzzing/src/oracles/diff_v8.rs @@ -35,6 +35,10 @@ impl V8Engine { bail!("memory64 not enabled by default in v8"); } + if config.config.max_memories > 1 { + bail!("multi-memory not enabled by default in v8"); + } + Ok(Self { isolate: Rc::new(RefCell::new(v8::Isolate::new(Default::default()))), }) @@ -78,7 +82,7 @@ impl DiffEngine for V8Engine { })) } - fn assert_error_match(&self, wasmtime: &Trap, err: Error) { + fn assert_error_match(&self, wasmtime: &Trap, err: &Error) { let v8 = err.to_string(); let wasmtime_msg = wasmtime.to_string(); let verify_wasmtime = |msg: &str| { @@ -148,6 +152,10 @@ impl DiffEngine for V8Engine { verify_wasmtime("not possibly present in an error, just panic please"); } + + fn is_stack_overflow(&self, err: &Error) -> bool { + err.to_string().contains("Maximum call stack size exceeded") + } } struct V8Instance { diff --git a/crates/fuzzing/src/oracles/diff_wasmi.rs b/crates/fuzzing/src/oracles/diff_wasmi.rs index efc9160c0b..2a34b6f379 100644 --- a/crates/fuzzing/src/oracles/diff_wasmi.rs +++ b/crates/fuzzing/src/oracles/diff_wasmi.rs @@ -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; @@ -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"); + } Ok(Self) } } @@ -49,13 +52,86 @@ 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)); + fn assert_error_match(&self, trap: &Trap, err: &Error) { + // 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_ref::() { + Some(wasmi::Error::Trap(trap)) => trap, + + // Out-of-bounds data segments turn into this category which + // Wasmtime reports as a `MemoryOutOfBounds`. + Some(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. + Some(wasmi::Error::Instantiation(msg)) => { + log::debug!("ignoring wasmi instantiation error: {msg}"); + return; + } + + Some(other) => panic!("unexpected wasmi error: {}", other), + + None => err + .downcast_ref::() + .expect(&format!("not a trap: {:?}", err)), + }; + 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!(), + } + } + + fn is_stack_overflow(&self, err: &Error) -> bool { + let trap = match err.downcast_ref::() { + Some(wasmi::Error::Trap(trap)) => trap, + Some(_) => return false, + None => match err.downcast_ref::() { + Some(trap) => trap, + None => return false, + }, + }; + match trap.kind() { + wasmi::TrapKind::StackOverflow => true, + _ => false, + } } } diff --git a/crates/fuzzing/src/oracles/diff_wasmtime.rs b/crates/fuzzing/src/oracles/diff_wasmtime.rs index 3c86138f9e..b3fa796d9d 100644 --- a/crates/fuzzing/src/oracles/diff_wasmtime.rs +++ b/crates/fuzzing/src/oracles/diff_wasmtime.rs @@ -5,7 +5,7 @@ use crate::oracles::dummy; use crate::oracles::engine::DiffInstance; use crate::oracles::{compile_module, engine::DiffEngine, StoreLimits}; use anyhow::{Context, Error, Result}; -use wasmtime::{Extern, FuncType, Instance, Module, Store, Trap, Val}; +use wasmtime::{Extern, FuncType, Instance, Module, Store, Trap, TrapCode, Val}; /// A wrapper for using Wasmtime as a [`DiffEngine`]. pub struct WasmtimeEngine { @@ -34,8 +34,10 @@ impl DiffEngine for WasmtimeEngine { Ok(Box::new(instance)) } - fn assert_error_match(&self, trap: &Trap, err: Error) { - let trap2 = err.downcast::().unwrap(); + fn assert_error_match(&self, trap: &Trap, err: &Error) { + let trap2 = err + .downcast_ref::() + .expect(&format!("not a trap: {:?}", err)); assert_eq!( trap.trap_code(), trap2.trap_code(), @@ -44,6 +46,13 @@ impl DiffEngine for WasmtimeEngine { trap2 ); } + + fn is_stack_overflow(&self, err: &Error) -> bool { + match err.downcast_ref::() { + Some(trap) => trap.trap_code() == Some(TrapCode::StackOverflow), + None => false, + } + } } /// A wrapper around a Wasmtime instance. diff --git a/crates/fuzzing/src/oracles/engine.rs b/crates/fuzzing/src/oracles/engine.rs index bcb76b4940..b69ac532a6 100644 --- a/crates/fuzzing/src/oracles/engine.rs +++ b/crates/fuzzing/src/oracles/engine.rs @@ -13,7 +13,7 @@ pub fn choose( u: &mut Unstructured<'_>, existing_config: &Config, allowed: &[&str], -) -> arbitrary::Result> { +) -> arbitrary::Result>> { // Filter out any engines that cannot match the `existing_config` or are not // `allowed`. let mut engines: Vec> = vec![]; @@ -54,13 +54,16 @@ pub fn choose( } } + if engines.is_empty() { + return Ok(None); + } + // 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. @@ -73,7 +76,11 @@ pub trait DiffEngine { /// Tests that the wasmtime-originating `trap` matches the error this engine /// generated. - fn assert_error_match(&self, trap: &Trap, err: Error); + fn assert_error_match(&self, trap: &Trap, err: &Error); + + /// Returns whether the error specified from this engine might be stack + /// overflow. + fn is_stack_overflow(&self, err: &Error) -> bool; } /// Provide a way to evaluate Wasm functions--a Wasm instance implemented by a diff --git a/fuzz/fuzz_targets/differential.rs b/fuzz/fuzz_targets/differential.rs index 0d24eb94b0..bb726e3236 100644 --- a/fuzz/fuzz_targets/differential.rs +++ b/fuzz/fuzz_targets/differential.rs @@ -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()); @@ -101,7 +106,7 @@ fn run(data: &[u8]) -> Result<()> { (Ok(l), Ok(r)) => (l, r), (Err(l), Err(r)) => { let err = r.downcast::().expect("not a trap"); - lhs.assert_error_match(&err, l); + lhs.assert_error_match(&err, &l); return Ok(()); } (l, r) => panic!( @@ -112,7 +117,7 @@ fn run(data: &[u8]) -> Result<()> { }; // Call each exported function with different sets of arguments. - for (name, signature) in rhs_instance.exported_functions() { + 'outer: for (name, signature) in rhs_instance.exported_functions() { let mut invocations = 0; loop { let arguments = signature @@ -123,8 +128,9 @@ fn run(data: &[u8]) -> Result<()> { .results() .map(|t| DiffValueType::try_from(t).unwrap()) .collect::>(); - differential( + let ok = differential( lhs_instance.as_mut(), + lhs.as_ref(), &mut rhs_instance, &name, &arguments, @@ -132,12 +138,20 @@ fn run(data: &[u8]) -> Result<()> { ) .expect("failed to run differential evaluation"); + invocations += 1; + STATS.total_invocations.fetch_add(1, SeqCst); + + // If this differential execution has resulted in the two instances + // diverging in state we can't keep executing so don't execute any + // more functions. + if !ok { + break 'outer; + } + // We evaluate the same function with different arguments until we // hit a predetermined limit or we run out of unstructured data--it // does not make sense to re-evaluate the same arguments over and // over. - invocations += 1; - STATS.total_invocations.fetch_add(1, SeqCst); if invocations > NUM_INVOCATIONS || u.is_empty() { break; }