Fold MaybeInvalidModule into Config (bytecodealliance#1411)

* Fold `MaybeInvalidModule` into `Config`

This commit removes the top-level `MaybeInvalidModule` type and instead
folds it into a configuration option inside of `Config`. This has the
benefit of being able to still configure all the other `Config` options
during generating possibly-invalid modules at the cost that we can no
longer statically rule out success of `ensure_termination`. This error
is now propagated upwards and documented in a few places.

Closes bytecodealliance#1410

* Make anyhow non-optional for wasm-smith

* Update fuzzers

crates/wasm-smith/Cargo.toml

@@ -19,7 +19,7 @@ harness = false
 workspace = true
 
 [dependencies]
-anyhow = { workspace = true, optional = true }
+anyhow = { workspace = true }
 arbitrary = { workspace = true, features = ["derive"] }
 clap = { workspace = true, optional = true }
 flagset = "0.4"
@@ -42,5 +42,5 @@ wat = { path = "../wat" }
 libfuzzer-sys = { workspace = true }
 
 [features]
-_internal_cli = ["anyhow", "clap", "flagset/serde", "serde", "serde_derive", "wasmparser", "wat"]
+_internal_cli = ["clap", "flagset/serde", "serde", "serde_derive", "wasmparser", "wat"]
 wasmparser = ['dep:wasmparser', 'wasm-encoder/wasmparser']

crates/wasm-smith/src/config.rs

@@ -481,6 +481,18 @@ define_config! {
         ///
         /// Defaults to `false`.
         pub threads_enabled: bool = false,
+
+        /// Indicates whether wasm-smith is allowed to generate invalid function
+        /// bodies.
+        ///
+        /// When enabled this option will enable taking raw bytes from the input
+        /// byte stream and using them as a wasm function body. This means that
+        /// the output module is not guaranteed to be valid but can help tickle
+        /// various parts of validation/compilation in some circumstances as
+        /// well.
+        ///
+        /// Defaults to `false`.
+        pub allow_invalid_funcs: bool = false,
     }
 }
 
@@ -617,6 +629,7 @@ impl<'a> Arbitrary<'a> for Config {
             tail_call_enabled: false,
             gc_enabled: false,
             generate_custom_sections: false,
+            allow_invalid_funcs: false,
         })
     }
 }

crates/wasm-smith/src/core.rs

@@ -172,7 +172,7 @@ impl Module {
         duplicate_imports_behavior: DuplicateImportsBehavior,
     ) -> Result<Self> {
         let mut module = Module::empty(config, duplicate_imports_behavior);
-        module.build(u, false)?;
+        module.build(u)?;
         Ok(module)
     }
 
@@ -213,30 +213,6 @@ impl Module {
     }
 }
 
-/// Same as [`Module`], but may be invalid.
-///
-/// This module generates function bodies differnetly than `Module` to try to
-/// better explore wasm decoders and such.
-#[derive(Debug)]
-pub struct MaybeInvalidModule {
-    module: Module,
-}
-
-impl MaybeInvalidModule {
-    /// Encode this Wasm module into bytes.
-    pub fn to_bytes(&self) -> Vec<u8> {
-        self.module.to_bytes()
-    }
-}
-
-impl<'a> Arbitrary<'a> for MaybeInvalidModule {
-    fn arbitrary(u: &mut Unstructured<'a>) -> Result<Self> {
-        let mut module = Module::empty(Config::default(), DuplicateImportsBehavior::Allowed);
-        module.build(u, true)?;
-        Ok(MaybeInvalidModule { module })
-    }
-}
-
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub(crate) struct RecGroup {
     pub(crate) types: Vec<SubType>,
@@ -398,7 +374,7 @@ pub(crate) enum GlobalInitExpr {
 }
 
 impl Module {
-    fn build(&mut self, u: &mut Unstructured, allow_invalid: bool) -> Result<()> {
+    fn build(&mut self, u: &mut Unstructured) -> Result<()> {
         self.valtypes = configured_valtypes(&self.config);
 
         // We attempt to figure out our available imports *before* creating the types section here,
@@ -425,7 +401,7 @@ impl Module {
         self.arbitrary_start(u)?;
         self.arbitrary_elems(u)?;
         self.arbitrary_data(u)?;
-        self.arbitrary_code(u, allow_invalid)?;
+        self.arbitrary_code(u)?;
         Ok(())
     }
 
@@ -1795,11 +1771,11 @@ impl Module {
         )
     }
 
-    fn arbitrary_code(&mut self, u: &mut Unstructured, allow_invalid: bool) -> Result<()> {
+    fn arbitrary_code(&mut self, u: &mut Unstructured) -> Result<()> {
         self.code.reserve(self.num_defined_funcs);
         let mut allocs = CodeBuilderAllocations::new(self);
         for (_, ty) in self.funcs[self.funcs.len() - self.num_defined_funcs..].iter() {
-            let body = self.arbitrary_func_body(u, ty, &mut allocs, allow_invalid)?;
+            let body = self.arbitrary_func_body(u, ty, &mut allocs)?;
             self.code.push(body);
         }
         allocs.finish(u, self)?;
@@ -1811,11 +1787,10 @@ impl Module {
         u: &mut Unstructured,
         ty: &FuncType,
         allocs: &mut CodeBuilderAllocations,
-        allow_invalid: bool,
     ) -> Result<Code> {
         let mut locals = self.arbitrary_locals(u)?;
         let builder = allocs.builder(ty, &mut locals);
-        let instructions = if allow_invalid && u.arbitrary().unwrap_or(false) {
+        let instructions = if self.config.allow_invalid_funcs && u.arbitrary().unwrap_or(false) {
             Instructions::Arbitrary(arbitrary_vec_u8(u)?)
         } else {
             Instructions::Generated(builder.arbitrary(u, self)?)

crates/wasm-smith/src/core/terminate.rs

@@ -1,4 +1,5 @@
 use super::*;
+use anyhow::{bail, Result};
 use std::mem;
 use wasm_encoder::BlockType;
 
@@ -12,7 +13,14 @@ impl Module {
     ///
     /// The index of the fuel global is returned, so that you may control how
     /// much fuel the module is given.
-    pub fn ensure_termination(&mut self, default_fuel: u32) -> u32 {
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if any function body was generated with
+    /// possibly-invalid bytes rather than being generated by wasm-smith. In
+    /// such a situation this pass does not parse the input bytes and inject
+    /// instructions, instead it returns an error.
+    pub fn ensure_termination(&mut self, default_fuel: u32) -> Result<u32> {
         let fuel_global = self.globals.len() as u32;
         self.globals.push(GlobalType {
             val_type: ValType::I32,
@@ -41,10 +49,12 @@ impl Module {
 
             let instrs = match &mut code.instructions {
                 Instructions::Generated(list) => list,
-                // only present on modules contained within
-                // `MaybeInvalidModule`, which doesn't expose its internal
-                // `Module`.
-                Instructions::Arbitrary(_) => unreachable!(),
+                Instructions::Arbitrary(_) => {
+                    bail!(
+                        "failed to ensure that a function generated due to it \
+                         containing arbitrary instructions"
+                    )
+                }
             };
             let mut new_insts = Vec::with_capacity(instrs.len() * 2);
 
@@ -65,6 +75,6 @@ impl Module {
             *instrs = new_insts;
         }
 
-        fuel_global
+        Ok(fuel_global)
     }
 }

crates/wasm-smith/src/lib.rs

@@ -58,7 +58,7 @@ mod component;
 mod config;
 mod core;
 
-pub use crate::core::{InstructionKind, InstructionKinds, MaybeInvalidModule, Module};
+pub use crate::core::{InstructionKind, InstructionKinds, Module};
 use arbitrary::{Result, Unstructured};
 pub use component::Component;
 pub use config::{Config, MemoryOffsetChoices};

crates/wasm-smith/tests/core.rs

@@ -27,7 +27,7 @@ fn smoke_test_ensure_termination() {
         rng.fill_bytes(&mut buf);
         let u = Unstructured::new(&buf);
         if let Ok(mut module) = Module::arbitrary_take_rest(u) {
-            module.ensure_termination(10);
+            module.ensure_termination(10).unwrap();
             let wasm_bytes = module.to_bytes();
 
             let mut validator = Validator::new_with_features(wasm_features());

fuzz/src/lib.rs

@@ -42,7 +42,7 @@ pub fn generate_valid_module(
     // still produce a valid module.
     if u.ratio(1, 10)? {
         log::debug!("ensuring termination with 100 fuel");
-        module.ensure_termination(100);
+        let _ = module.ensure_termination(100);
     }
 
     log_wasm(&bytes, &config);
@@ -76,6 +76,27 @@ pub fn generate_valid_component(
     Ok((bytes, config))
 }
 
+pub fn validator_for_config(config: &Config) -> wasmparser::Validator {
+    wasmparser::Validator::new_with_features(wasmparser::WasmFeatures {
+        multi_value: config.multi_value_enabled,
+        multi_memory: config.max_memories > 1,
+        bulk_memory: config.bulk_memory_enabled,
+        reference_types: config.reference_types_enabled,
+        simd: config.simd_enabled,
+        relaxed_simd: config.relaxed_simd_enabled,
+        memory64: config.memory64_enabled,
+        threads: config.threads_enabled,
+        exceptions: config.exceptions_enabled,
+        // TODO: determine our larger story for function-references in
+        // wasm-tools and whether we should just have a Wasm GC flag since
+        // function-references is effectively part of the Wasm GC proposal at
+        // this point.
+        function_references: config.gc_enabled,
+        gc: config.gc_enabled,
+        ..wasmparser::WasmFeatures::default()
+    })
+}
+
 // Optionally log the module and its configuration if we've gotten this
 // far. Note that we don't do this unconditionally to avoid slowing down
 // fuzzing, but this is expected to be enabled when debugging a failing

fuzz/src/validate.rs

@@ -1,8 +1,33 @@
-use arbitrary::{Arbitrary, Result, Unstructured};
-use wasm_smith::MaybeInvalidModule;
+use arbitrary::{Result, Unstructured};
 use wasmparser::{Validator, WasmFeatures};
 
 pub fn run(u: &mut Unstructured<'_>) -> Result<()> {
+    // Either use `wasm-smith` to generate a module with possibly invalid
+    // functions or try validating raw bytes from the input itself.
+    if u.arbitrary()? {
+        validate_maybe_invalid_module(u)?;
+    } else {
+        validate_raw_bytes(u)?;
+    }
+    Ok(())
+}
+
+pub fn validate_maybe_invalid_module(u: &mut Unstructured<'_>) -> Result<()> {
+    // Generate a "valid" module but specifically allow invalid functions which
+    // means that some functions may be defined from the input bytes raw. This
+    // means that most of the module is valid but only some functions may be
+    // invalid which can help stress various bits and pieces of validation.
+    let (wasm, config) = crate::generate_valid_module(u, |config, _| {
+        config.allow_invalid_funcs = true;
+        Ok(())
+    })?;
+    let mut validator = crate::validator_for_config(&config);
+    drop(validator.validate_all(&wasm));
+    Ok(())
+}
+
+pub fn validate_raw_bytes(u: &mut Unstructured<'_>) -> Result<()> {
+    // Enable arbitrary combinations of features to validate the input bytes.
     let mut validator = Validator::new_with_features(WasmFeatures {
         reference_types: u.arbitrary()?,
         multi_value: u.arbitrary()?,
@@ -26,18 +51,8 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> {
         component_model_values: u.arbitrary()?,
         component_model_nested_names: u.arbitrary()?,
     });
-    let use_maybe_invalid = u.arbitrary()?;
-
-    if use_maybe_invalid {
-        if let Ok(module) = MaybeInvalidModule::arbitrary(u) {
-            let wasm = module.to_bytes();
-            crate::log_wasm(&wasm, "");
-            drop(validator.validate_all(&wasm));
-        }
-    } else {
-        let wasm = u.bytes(u.len())?;
-        crate::log_wasm(wasm, "");
-        drop(validator.validate_all(wasm));
-    }
+    let wasm = u.bytes(u.len())?;
+    crate::log_wasm(wasm, "");
+    drop(validator.validate_all(wasm));
     Ok(())
 }

fuzz/src/validate_valid_module.rs

@@ -25,25 +25,7 @@ pub fn run(u: &mut Unstructured<'_>) -> Result<()> {
     };
 
     // Validate the module or component and assert that it passes validation.
-    let mut validator = wasmparser::Validator::new_with_features(wasmparser::WasmFeatures {
-        component_model: generate_component,
-        multi_value: config.multi_value_enabled,
-        multi_memory: config.max_memories > 1,
-        bulk_memory: config.bulk_memory_enabled,
-        reference_types: config.reference_types_enabled,
-        simd: config.simd_enabled,
-        relaxed_simd: config.relaxed_simd_enabled,
-        memory64: config.memory64_enabled,
-        threads: config.threads_enabled,
-        exceptions: config.exceptions_enabled,
-        // TODO: determine our larger story for function-references in
-        // wasm-tools and whether we should just have a Wasm GC flag since
-        // function-references is effectively part of the Wasm GC proposal at
-        // this point.
-        function_references: config.gc_enabled,
-        gc: config.gc_enabled,
-        ..wasmparser::WasmFeatures::default()
-    });
+    let mut validator = crate::validator_for_config(&config);
     if let Err(e) = validator.validate_all(&wasm_bytes) {
         let component_or_module = if generate_component {
             "component"