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

@@ -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;
+            // Force this pooling allocator to always be able to accommodate the
-            cfg.max_memories = limits.memories as usize;
+            // module that may be generated.
-            cfg.max_tables = limits.tables as usize;
+            limits.memories = cfg.max_memories as u32;
-            cfg.max_memory_pages = limits.memory_pages;
+            limits.tables = cfg.max_tables as u32;
-
-            // Force no aliases in any generated modules as they might count against the
-            // import limits above.
-            cfg.max_aliases = 0;
         }
 
         Ok(config)

crates/fuzzing/src/generators/module.rs

@@ -17,21 +17,27 @@ impl<'a> Arbitrary<'a> for ModuleConfig {
     fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<ModuleConfig> {
         let mut config = SwarmConfig::arbitrary(u)?;
 
-        // Allow multi-memory by default.
+        // Allow multi-memory but make it unlikely
-        config.max_memories = config.max_memories.max(2);
+        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 })
     }

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<StoreLimits>, 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<bool> {
     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
+        // Both sides failed. If either one hits a stack overflow then that's an
-        // (since the `lhs` and `rhs` could be failing for different reasons)
+        // engine defined limit which means we can no longer compare the state
-        // but this seems good enough for now.
+        // of the two instances, so `false` is returned and nothing else is
-        (Err(_), Err(_)) => {}
+        // 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::<Trap>().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.

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 {

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 {

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) {
+    fn assert_error_match(&self, trap: &Trap, err: &Error) {
-        // TODO: should implement this for `wasmi`
+        // Acquire a `wasmi::Trap` from the wasmi error which we'll use to
-        drop((trap, err));
+        // assert that it has the same kind of trap as the wasmtime-based trap.
+        let wasmi = match err.downcast_ref::<wasmi::Error>() {
+            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::<wasmi::Trap>()
+                .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::<wasmi::Error>() {
+            Some(wasmi::Error::Trap(trap)) => trap,
+            Some(_) => return false,
+            None => match err.downcast_ref::<wasmi::Trap>() {
+                Some(trap) => trap,
+                None => return false,
+            },
+        };
+        match trap.kind() {
+            wasmi::TrapKind::StackOverflow => true,
+            _ => false,
+        }
     }
 }
 

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) {
+    fn assert_error_match(&self, trap: &Trap, err: &Error) {
-        let trap2 = err.downcast::<Trap>().unwrap();
+        let trap2 = err
+            .downcast_ref::<Trap>()
+            .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::<Trap>() {
+            Some(trap) => trap.trap_code() == Some(TrapCode::StackOverflow),
+            None => false,
+        }
+    }
 }
 
 /// A wrapper around a Wasmtime instance.

crates/fuzzing/src/oracles/engine.rs

@@ -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![];
@@ -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

fuzz/fuzz_targets/differential.rs

@@ -86,8 +86,13 @@ fn run(data: &[u8]) -> Result<()> {
     };
     log_wasm(&wasm);
 
-    // Choose a left-hand side Wasm engine.
+    // Choose a left-hand side Wasm engine. If no engine could be chosen then
-    let mut lhs = engine::choose(&mut u, &config, unsafe { &ALLOWED_ENGINES })?;
+    // 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::<Trap>().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::<Vec<_>>();
-            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;
             }