diff --git a/crates/fuzzing/src/generators/config.rs b/crates/fuzzing/src/generators/config.rs index 6ee459a7af..6da42fb786 100644 --- a/crates/fuzzing/src/generators/config.rs +++ b/crates/fuzzing/src/generators/config.rs @@ -38,11 +38,11 @@ impl Config { // Make it more likely that there are types available to generate a // function with. - config.min_types = 1; + config.min_types = config.min_types.max(1); config.max_types = config.max_types.max(1); // Generate at least one function - config.min_funcs = 1; + config.min_funcs = config.min_funcs.max(1); config.max_funcs = config.max_funcs.max(1); // Allow a memory to be generated, but don't let it get too large. @@ -102,7 +102,7 @@ impl Config { /// to ensure termination; as doing so will add an additional global to the module, /// the pooling allocator, if configured, will also have its globals limit updated. pub fn generate( - &mut self, + &self, input: &mut Unstructured<'_>, default_fuel: Option, ) -> arbitrary::Result { diff --git a/crates/fuzzing/src/generators/single_inst_module.rs b/crates/fuzzing/src/generators/single_inst_module.rs index ee156610d9..8f77d24f40 100644 --- a/crates/fuzzing/src/generators/single_inst_module.rs +++ b/crates/fuzzing/src/generators/single_inst_module.rs @@ -24,17 +24,7 @@ pub struct SingleInstModule<'a> { impl<'a> SingleInstModule<'a> { /// Choose a single-instruction module that matches `config`. - pub fn new(u: &mut Unstructured<'a>, config: &mut ModuleConfig) -> arbitrary::Result<&'a Self> { - // To avoid skipping modules unnecessarily during fuzzing, fix up the - // `ModuleConfig` to match the inherent limits of a single-instruction - // module. - config.config.min_funcs = 1; - config.config.max_funcs = 1; - config.config.min_tables = 0; - config.config.max_tables = 0; - config.config.min_memories = 0; - config.config.max_memories = 0; - + pub fn new(u: &mut Unstructured<'a>, config: &ModuleConfig) -> arbitrary::Result<&'a Self> { // Only select instructions that match the `ModuleConfig`. let instructions = &INSTRUCTIONS .iter() diff --git a/crates/fuzzing/src/oracles/diff_spec.rs b/crates/fuzzing/src/oracles/diff_spec.rs index d90b8d32bd..42e9fe9d41 100644 --- a/crates/fuzzing/src/oracles/diff_spec.rs +++ b/crates/fuzzing/src/oracles/diff_spec.rs @@ -1,9 +1,9 @@ //! Evaluate an exported Wasm function using the WebAssembly specification //! reference interpreter. -use crate::generators::{DiffValue, DiffValueType, ModuleConfig}; +use crate::generators::{Config, DiffValue, DiffValueType}; use crate::oracles::engine::{DiffEngine, DiffInstance}; -use anyhow::{anyhow, bail, Error, Result}; +use anyhow::{anyhow, Error, Result}; use wasm_spec_interpreter::Value; use wasmtime::Trap; @@ -11,20 +11,15 @@ use wasmtime::Trap; pub struct SpecInterpreter; impl SpecInterpreter { - /// Build a new [`SpecInterpreter`] but only if the configuration does not - /// rely on features that the current bindings (i.e., - /// `wasm-spec-interpreter`) do not support. - pub fn new(config: &ModuleConfig) -> Result { - if config.config.reference_types_enabled { - bail!("the spec interpreter bindings do not support reference types") - } + pub(crate) fn new(config: &mut Config) -> Self { + let config = &mut config.module_config.config; + // TODO: right now the interpreter bindings only execute the first // function in the module so if there's possibly more than one function // it's not possible to run the other function. This should be fixed // with improvements to the ocaml bindings to the interpreter. - if config.config.max_funcs > 1 { - bail!("the spec interpreter bindings can only support one function for now") - } + config.min_funcs = 1; + config.max_funcs = 1; // TODO: right now the instantiation step for the interpreter does // nothing and the evaluation step performs an instantiation followed by @@ -32,18 +27,18 @@ impl SpecInterpreter { // engines will "succeed" in the interpreter because the error is // delayed to the execution. This should be fixed by making // instantiation a first-class primitive in our interpreter bindings. - if config.config.max_tables > 0 { - bail!("the spec interpreter bindings do not fail as they should with out-of-bounds table accesses") - } + config.min_tables = 0; + config.max_tables = 0; - if config.config.memory64_enabled { - bail!("memory64 not implemented in spec interpreter"); - } + config.min_memories = config.min_memories.min(1); + config.max_memories = config.max_memories.min(1); - if config.config.threads_enabled { - bail!("spec interpreter does not support the threading proposal"); - } - Ok(Self) + config.memory64_enabled = false; + config.threads_enabled = false; + config.bulk_memory_enabled = false; + config.reference_types_enabled = false; + + Self } } @@ -160,5 +155,5 @@ fn smoke() { if !wasm_spec_interpreter::support_compiled_in() { return; } - crate::oracles::engine::smoke_test_engine(|config| SpecInterpreter::new(&config.module_config)) + crate::oracles::engine::smoke_test_engine(|_, config| Ok(SpecInterpreter::new(config))) } diff --git a/crates/fuzzing/src/oracles/diff_v8.rs b/crates/fuzzing/src/oracles/diff_v8.rs index d5d7ac0cb8..8b8efd79d2 100644 --- a/crates/fuzzing/src/oracles/diff_v8.rs +++ b/crates/fuzzing/src/oracles/diff_v8.rs @@ -1,4 +1,4 @@ -use crate::generators::{DiffValue, DiffValueType, ModuleConfig}; +use crate::generators::{Config, DiffValue, DiffValueType}; use crate::oracles::engine::{DiffEngine, DiffInstance}; use anyhow::{bail, Error, Result}; use std::cell::RefCell; @@ -12,7 +12,7 @@ pub struct V8Engine { } impl V8Engine { - pub fn new(config: &ModuleConfig) -> Result { + pub fn new(config: &mut Config) -> V8Engine { static INIT: Once = Once::new(); INIT.call_once(|| { @@ -21,27 +21,22 @@ impl V8Engine { v8::V8::initialize(); }); + let config = &mut config.module_config.config; // FIXME: reference types are disabled for now as we seemingly keep finding // a segfault in v8. This is found relatively quickly locally and keeps // getting found by oss-fuzz and currently we don't think that there's // really much we can do about it. For the time being disable reference // types entirely. An example bug is // https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=45662 - if config.config.reference_types_enabled { - bail!("reference types are buggy in v8"); - } + config.reference_types_enabled = false; - if config.config.memory64_enabled { - bail!("memory64 not enabled by default in v8"); - } + config.min_memories = config.min_memories.min(1); + config.max_memories = config.max_memories.min(1); + config.memory64_enabled = false; - if config.config.max_memories > 1 { - bail!("multi-memory not enabled by default in v8"); - } - - Ok(Self { + Self { isolate: Rc::new(RefCell::new(v8::Isolate::new(Default::default()))), - }) + } } } @@ -325,5 +320,5 @@ fn get_diff_value( #[test] fn smoke() { - crate::oracles::engine::smoke_test_engine(|config| V8Engine::new(&config.module_config)) + crate::oracles::engine::smoke_test_engine(|_, config| Ok(V8Engine::new(config))) } diff --git a/crates/fuzzing/src/oracles/diff_wasmi.rs b/crates/fuzzing/src/oracles/diff_wasmi.rs index 2a34b6f379..457d5e0570 100644 --- a/crates/fuzzing/src/oracles/diff_wasmi.rs +++ b/crates/fuzzing/src/oracles/diff_wasmi.rs @@ -1,45 +1,30 @@ //! Evaluate an exported Wasm function using the wasmi interpreter. -use crate::generators::{DiffValue, DiffValueType, ModuleConfig}; +use crate::generators::{Config, DiffValue, DiffValueType}; use crate::oracles::engine::{DiffEngine, DiffInstance}; -use anyhow::{bail, Context, Error, Result}; +use anyhow::{Context, Error, Result}; use wasmtime::{Trap, TrapCode}; /// A wrapper for `wasmi` as a [`DiffEngine`]. pub struct WasmiEngine; impl WasmiEngine { - /// Build a new [`WasmiEngine`] but only if the configuration does not rely - /// on features that `wasmi` does not support. - pub fn new(config: &ModuleConfig) -> Result { - if config.config.reference_types_enabled { - bail!("wasmi does not support reference types") - } - if config.config.simd_enabled { - bail!("wasmi does not support SIMD") - } - if config.config.multi_value_enabled { - bail!("wasmi does not support multi-value") - } - if config.config.saturating_float_to_int_enabled { - bail!("wasmi does not support saturating float-to-int conversions") - } - if config.config.sign_extension_enabled { - bail!("wasmi does not support sign-extension") - } - if config.config.memory64_enabled { - bail!("wasmi does not support memory64"); - } - if config.config.bulk_memory_enabled { - bail!("wasmi does not support bulk memory"); - } - 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) + pub(crate) fn new(config: &mut Config) -> Self { + let config = &mut config.module_config.config; + config.reference_types_enabled = false; + config.simd_enabled = false; + config.multi_value_enabled = false; + config.saturating_float_to_int_enabled = false; + config.sign_extension_enabled = false; + config.memory64_enabled = false; + config.bulk_memory_enabled = false; + config.threads_enabled = false; + config.max_memories = config.max_memories.min(1); + config.min_memories = config.min_memories.min(1); + config.max_tables = config.max_tables.min(1); + config.min_tables = config.min_tables.min(1); + + Self } } @@ -223,5 +208,5 @@ impl Into for wasmi::RuntimeValue { #[test] fn smoke() { - crate::oracles::engine::smoke_test_engine(|config| WasmiEngine::new(&config.module_config)) + crate::oracles::engine::smoke_test_engine(|_, config| Ok(WasmiEngine::new(config))) } diff --git a/crates/fuzzing/src/oracles/diff_wasmtime.rs b/crates/fuzzing/src/oracles/diff_wasmtime.rs index b3fa796d9d..29d0e08475 100644 --- a/crates/fuzzing/src/oracles/diff_wasmtime.rs +++ b/crates/fuzzing/src/oracles/diff_wasmtime.rs @@ -1,15 +1,16 @@ //! Evaluate an exported Wasm function using Wasmtime. -use crate::generators::{self, DiffValue, DiffValueType}; +use crate::generators::{self, DiffValue, DiffValueType, WasmtimeConfig}; use crate::oracles::dummy; use crate::oracles::engine::DiffInstance; use crate::oracles::{compile_module, engine::DiffEngine, StoreLimits}; use anyhow::{Context, Error, Result}; +use arbitrary::Unstructured; use wasmtime::{Extern, FuncType, Instance, Module, Store, Trap, TrapCode, Val}; /// A wrapper for using Wasmtime as a [`DiffEngine`]. pub struct WasmtimeEngine { - pub(crate) config: generators::Config, + config: generators::Config, } impl WasmtimeEngine { @@ -17,7 +18,13 @@ impl WasmtimeEngine { /// later. Ideally the store and engine could be built here but /// `compile_module` takes a [`generators::Config`]; TODO re-factor this if /// that ever changes. - pub fn new(config: generators::Config) -> Result { + pub fn new(u: &mut Unstructured<'_>, config: &generators::Config) -> arbitrary::Result { + let mut new_config = u.arbitrary::()?; + new_config.make_compatible_with(&config.wasmtime); + let config = generators::Config { + wasmtime: new_config, + module_config: config.module_config.clone(), + }; Ok(Self { config }) } } @@ -220,5 +227,5 @@ impl Into for Val { #[test] fn smoke() { - crate::oracles::engine::smoke_test_engine(|config| WasmtimeEngine::new(config)) + crate::oracles::engine::smoke_test_engine(|u, config| WasmtimeEngine::new(u, config)) } diff --git a/crates/fuzzing/src/oracles/engine.rs b/crates/fuzzing/src/oracles/engine.rs index b69ac532a6..f791dee024 100644 --- a/crates/fuzzing/src/oracles/engine.rs +++ b/crates/fuzzing/src/oracles/engine.rs @@ -1,68 +1,37 @@ //! Define the interface for differential evaluation of Wasm functions. -use crate::generators::{Config, DiffValue, DiffValueType, WasmtimeConfig}; +use crate::generators::{Config, DiffValue, DiffValueType}; use crate::oracles::{diff_wasmi::WasmiEngine, diff_wasmtime::WasmtimeEngine}; use anyhow::Error; use arbitrary::Unstructured; use wasmtime::Trap; -/// Pick one of the engines implemented in this module that is: -/// - in the list of `allowed` engines -/// - can evaluate Wasm modules compatible with `existing_config`. -pub fn choose( +/// Returns a function which can be used to build the engine name specified. +/// +/// `None` is returned if the named engine does not have support compiled into +/// this crate. +pub fn build( u: &mut Unstructured<'_>, - existing_config: &Config, - allowed: &[&str], + name: &str, + config: &mut Config, ) -> arbitrary::Result>> { - // Filter out any engines that cannot match the `existing_config` or are not - // `allowed`. - let mut engines: Vec> = vec![]; + let engine: Box = match name { + "wasmtime" => Box::new(WasmtimeEngine::new(u, config)?), + "wasmi" => Box::new(WasmiEngine::new(config)), - if allowed.contains(&"wasmtime") { - let mut new_wasmtime_config: WasmtimeConfig = u.arbitrary()?; - new_wasmtime_config.make_compatible_with(&existing_config.wasmtime); - let new_config = Config { - wasmtime: new_wasmtime_config, - module_config: existing_config.module_config.clone(), - }; - if let Result::Ok(e) = WasmtimeEngine::new(new_config) { - engines.push(Box::new(e)) - } - } + #[cfg(feature = "fuzz-spec-interpreter")] + "spec" => Box::new(crate::oracles::diff_spec::SpecInterpreter::new(config)), + #[cfg(not(feature = "fuzz-spec-interpreter"))] + "spec" => return Ok(None), - if allowed.contains(&"wasmi") { - if let Result::Ok(e) = WasmiEngine::new(&existing_config.module_config) { - engines.push(Box::new(e)) - } - } + #[cfg(not(any(windows, target_arch = "s390x")))] + "v8" => Box::new(crate::oracles::diff_v8::V8Engine::new(config)), + #[cfg(any(windows, target_arch = "s390x"))] + "v8" => return Ok(None), - #[cfg(feature = "fuzz-spec-interpreter")] - if allowed.contains(&"spec") { - if let Result::Ok(e) = - crate::oracles::diff_spec::SpecInterpreter::new(&existing_config.module_config) - { - engines.push(Box::new(e)) - } - } + _ => panic!("unknown engine {name}"), + }; - #[cfg(not(any(windows, target_arch = "s390x")))] - if allowed.contains(&"v8") { - if let Result::Ok(e) = - crate::oracles::diff_v8::V8Engine::new(&existing_config.module_config) - { - engines.push(Box::new(e)) - } - } - - if engines.is_empty() { - return Ok(None); - } - - // Use the input of the fuzzer to pick an engine that we'll be fuzzing - // Wasmtime against. - let index: usize = u.int_in_range(0..=engines.len() - 1)?; - let engine = engines.swap_remove(index); - log::debug!("selected engine: {}", engine.name()); Ok(Some(engine)) } @@ -187,11 +156,11 @@ pub fn parse_env_list(env_variable: &str) -> Option> { } #[cfg(test)] -pub fn smoke_test_engine(mk_engine: impl Fn(Config) -> anyhow::Result) -where +pub fn smoke_test_engine( + mk_engine: impl Fn(&mut arbitrary::Unstructured<'_>, &mut Config) -> arbitrary::Result, +) where T: DiffEngine, { - use arbitrary::Arbitrary; use rand::prelude::*; let mut rng = SmallRng::seed_from_u64(0); @@ -199,8 +168,8 @@ where let n = 100; for _ in 0..n { rng.fill_bytes(&mut buf); - let u = Unstructured::new(&buf); - let mut config = match Config::arbitrary_take_rest(u) { + let mut u = Unstructured::new(&buf); + let mut config = match u.arbitrary::() { Ok(config) => config, Err(_) => continue, }; @@ -208,19 +177,7 @@ where // settings, can guaranteed instantiate a module. config.set_differential_config(); - // Configure settings to ensure that any filters in engine constructors - // try not to filter out this `Config`. - config.module_config.config.reference_types_enabled = false; - config.module_config.config.bulk_memory_enabled = false; - config.module_config.config.memory64_enabled = false; - config.module_config.config.threads_enabled = false; - config.module_config.config.simd_enabled = false; - config.module_config.config.min_funcs = 1; - config.module_config.config.max_funcs = 1; - config.module_config.config.min_tables = 0; - config.module_config.config.max_tables = 0; - - let mut engine = match mk_engine(config) { + let mut engine = match mk_engine(&mut u, &mut config) { Ok(engine) => engine, Err(e) => { println!("skip {:?}", e); diff --git a/fuzz/fuzz_targets/differential.rs b/fuzz/fuzz_targets/differential.rs index bb726e3236..53c376b967 100644 --- a/fuzz/fuzz_targets/differential.rs +++ b/fuzz/fuzz_targets/differential.rs @@ -60,60 +60,84 @@ fn run(data: &[u8]) -> Result<()> { STATS.bump_attempts(); let mut u = Unstructured::new(data); + + // Generate a Wasmtime and module configuration and update its settings + // initially to be suitable for differential execution where the generated + // wasm will behave the same in two different engines. This will get further + // refined below. let mut config: Config = u.arbitrary()?; config.set_differential_config(); - // Generate the Wasm module; this is specified by either the ALLOWED_MODULES - // environment variable or a random selection between wasm-smith and - // single-inst. - let build_wasm_smith_module = |u: &mut Unstructured, config: &mut Config| -> Result<_> { + // Choose an engine that Wasmtime will be differentially executed against. + // The chosen engine is then created, which might update `config`, and + // returned as a trait object. + let lhs = u.choose(unsafe { &ALLOWED_ENGINES })?; + let mut lhs = match engine::build(&mut u, lhs, &mut config)? { + Some(engine) => engine, + // The chosen engine does not have support compiled into the fuzzer, + // discard this test case. + None => return Ok(()), + }; + + // Using the now-legalized module configuration generate the Wasm module; + // this is specified by either the ALLOWED_MODULES environment variable or a + // random selection between wasm-smith and single-inst. + let build_wasm_smith_module = |u: &mut Unstructured, config: &Config| -> Result<_> { STATS.wasm_smith_modules.fetch_add(1, SeqCst); let module = config.generate(u, Some(1000))?; Ok(module.to_bytes()) }; - let build_single_inst_module = |u: &mut Unstructured, config: &mut Config| -> Result<_> { + let build_single_inst_module = |u: &mut Unstructured, config: &Config| -> Result<_> { STATS.single_instruction_modules.fetch_add(1, SeqCst); - let module = SingleInstModule::new(u, &mut config.module_config)?; + let module = SingleInstModule::new(u, &config.module_config)?; Ok(module.to_bytes()) }; if unsafe { ALLOWED_MODULES.is_empty() } { panic!("unable to generate a module to fuzz against; check `ALLOWED_MODULES`") } let wasm = match *u.choose(unsafe { ALLOWED_MODULES.as_slice() })? { - "wasm-smith" => build_wasm_smith_module(&mut u, &mut config)?, - "single-inst" => build_single_inst_module(&mut u, &mut config)?, + "wasm-smith" => build_wasm_smith_module(&mut u, &config)?, + "single-inst" => build_single_inst_module(&mut u, &config)?, _ => unreachable!(), }; log_wasm(&wasm); - // 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(()), - }; + // Instantiate the generated wasm file in the chosen differential engine. let lhs_instance = lhs.instantiate(&wasm); STATS.bump_engine(lhs.name()); - // Choose a right-hand side Wasm engine--this will always be Wasmtime. + // Always use Wasmtime as the second engine to instantiate within. let rhs_store = config.to_store(); let rhs_module = wasmtime::Module::new(rhs_store.engine(), &wasm).unwrap(); let rhs_instance = WasmtimeInstance::new(rhs_store, rhs_module); - // If we fail to instantiate, check that both sides do. let (mut lhs_instance, mut rhs_instance) = match (lhs_instance, rhs_instance) { + // Both sides successful, continue below to invoking exports. (Ok(l), Ok(r)) => (l, r), + + // Both sides failed, make sure they failed for the same reason but then + // we're done with this fuzz test case. (Err(l), Err(r)) => { let err = r.downcast::().expect("not a trap"); lhs.assert_error_match(&err, &l); return Ok(()); } - (l, r) => panic!( - "failed to instantiate only one side: {:?} != {:?}", - l.err(), - r.err() - ), + + // One side succeeded and one side failed, that means a bug happened! + (l, r) => { + // FIXME(#4852): the spec interpreter doesn't instantiate as part of + // the instantiate step so if wasmtime failed and the spec succeeded + // that's ok. This clause should be removed once that issue is + // fixed. + if l.is_ok() && lhs.name() == "spec" { + return Ok(()); + } + panic!( + "failed to instantiate only one side: {:?} != {:?}", + l.err(), + r.err() + ) + } }; // Call each exported function with different sets of arguments. @@ -148,6 +172,13 @@ fn run(data: &[u8]) -> Result<()> { break 'outer; } + // FIXME(#4852): the spec interpreter only supports one execution + // right now because each execution re-instantiates the module in + // its bindings. This should be removed once that issue is fixed. + if lhs.name() == "spec" { + 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