fuzz: Add a fuzz target for table.{get,set} operations

This new fuzz target exercises sequences of `table.get`s, `table.set`s, and
GCs.

It already found a couple bugs:

* Some leaks due to ref count cycles between stores and host-defined functions
  closing over those stores.

* If there are no live references for a PC, Cranelift can avoid emiting an
  associated stack map. This was running afoul of a debug assertion.

crates/fuzzing/src/generators.rs

@@ -11,6 +11,8 @@
 #[cfg(feature = "binaryen")]
 pub mod api;
 
+pub mod table_ops;
+
 use arbitrary::{Arbitrary, Unstructured};
 
 /// A Wasm test case generator that is powered by Binaryen's `wasm-opt -ttf`.

crates/fuzzing/src/generators/table_ops.rs

@@ -0,0 +1,148 @@
+//! Generating series of `table.get` and `table.set` operations.
+
+use arbitrary::Arbitrary;
+use std::fmt::Write;
+use std::ops::Range;
+
+/// A description of a Wasm module that makes a series of `externref` table
+/// operations.
+#[derive(Arbitrary, Debug)]
+pub struct TableOps {
+    num_params: u8,
+    table_size: u32,
+    ops: Vec<TableOp>,
+}
+
+const NUM_PARAMS_RANGE: Range<u8> = 1..10;
+const TABLE_SIZE_RANGE: Range<u32> = 1..100;
+const MAX_OPS: usize = 1000;
+
+impl TableOps {
+    /// Get the number of parameters this module's "run" function takes.
+    pub fn num_params(&self) -> u8 {
+        let num_params = std::cmp::max(self.num_params, NUM_PARAMS_RANGE.start);
+        let num_params = std::cmp::min(num_params, NUM_PARAMS_RANGE.end);
+        num_params
+    }
+
+    /// Get the size of the table that this module uses.
+    pub fn table_size(&self) -> u32 {
+        let table_size = std::cmp::max(self.table_size, TABLE_SIZE_RANGE.start);
+        let table_size = std::cmp::min(table_size, TABLE_SIZE_RANGE.end);
+        table_size
+    }
+
+    /// Convert this into a WAT string.
+    ///
+    /// The module requires a single import: `(import "" "gc" (func))`. This
+    /// should be a function to trigger GC.
+    ///
+    /// The single export of the module is a function "run" that takes
+    /// `self.num_params()` parameters of type `externref`.
+    ///
+    /// The "run" function is guaranteed to terminate (no loops or recursive
+    /// calls), but is not guaranteed to avoid traps (might access out-of-bounds
+    /// of the table).
+    pub fn to_wat_string(&self) -> String {
+        let mut wat = "(module\n".to_string();
+
+        // Import the GC function.
+        wat.push_str("  (import \"\" \"gc\" (func))\n");
+
+        // Define our table.
+        wat.push_str("  (table $table ");
+        write!(&mut wat, "{}", self.table_size()).unwrap();
+        wat.push_str(" externref)\n");
+
+        // Define the "run" function export.
+        wat.push_str(r#"  (func (export "run") (param"#);
+        for _ in 0..self.num_params() {
+            wat.push_str(" externref");
+        }
+        wat.push_str(")\n");
+        for op in self.ops.iter().take(MAX_OPS) {
+            wat.push_str("    ");
+            op.to_wat_string(&mut wat);
+            wat.push('\n');
+        }
+        wat.push_str("  )\n");
+
+        wat.push_str(")\n");
+        wat
+    }
+}
+
+#[derive(Arbitrary, Debug)]
+pub(crate) enum TableOp {
+    // `(call 0)`
+    Gc,
+    // `(drop (table.get x))`
+    Get(u32),
+    // `(table.set x (local.get y))`
+    SetFromParam(u32, u8),
+    // `(table.set x (table.get y))`
+    SetFromGet(u32, u32),
+}
+
+impl TableOp {
+    fn to_wat_string(&self, wat: &mut String) {
+        match self {
+            Self::Gc => {
+                wat.push_str("(call 0)");
+            }
+            Self::Get(x) => {
+                wat.push_str("(drop (table.get $table (i32.const ");
+                write!(wat, "{}", x).unwrap();
+                wat.push_str(")))");
+            }
+            Self::SetFromParam(x, y) => {
+                wat.push_str("(table.set $table (i32.const ");
+                write!(wat, "{}", x).unwrap();
+                wat.push_str(") (local.get ");
+                write!(wat, "{}", y).unwrap();
+                wat.push_str("))");
+            }
+            Self::SetFromGet(x, y) => {
+                wat.push_str("(table.set $table (i32.const ");
+                write!(wat, "{}", x).unwrap();
+                wat.push_str(") (table.get $table (i32.const ");
+                write!(wat, "{}", y).unwrap();
+                wat.push_str(")))");
+            }
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_wat_string() {
+        let ops = TableOps {
+            num_params: 2,
+            table_size: 10,
+            ops: vec![
+                TableOp::Gc,
+                TableOp::Get(0),
+                TableOp::SetFromParam(1, 2),
+                TableOp::SetFromGet(3, 4),
+            ],
+        };
+
+        let expected = r#"
+(module
+  (import "" "gc" (func))
+  (table $table 10 externref)
+  (func (export "run") (param externref externref)
+    (call 0)
+    (drop (table.get $table (i32.const 0)))
+    (table.set $table (i32.const 1) (local.get 2))
+    (table.set $table (i32.const 3) (table.get $table (i32.const 4)))
+  )
+)
+"#;
+        let actual = ops.to_wat_string();
+        assert_eq!(actual.trim(), expected.trim());
+    }
+}

crates/fuzzing/src/oracles.rs

@@ -13,12 +13,15 @@
 pub mod dummy;
 
 use dummy::dummy_imports;
+use std::cell::Cell;
+use std::rc::Rc;
 use std::sync::atomic::{AtomicUsize, Ordering::SeqCst};
 use wasmtime::*;
 use wasmtime_wast::WastContext;
 
+static CNT: AtomicUsize = AtomicUsize::new(0);
+
 fn log_wasm(wasm: &[u8]) {
-    static CNT: AtomicUsize = AtomicUsize::new(0);
     if !log::log_enabled!(log::Level::Debug) {
         return;
     }
@@ -33,6 +36,16 @@ fn log_wasm(wasm: &[u8]) {
     }
 }
 
+fn log_wat(wat: &str) {
+    if !log::log_enabled!(log::Level::Debug) {
+        return;
+    }
+
+    let i = CNT.fetch_add(1, SeqCst);
+    let name = format!("testcase{}.wat", i);
+    std::fs::write(&name, wat).expect("failed to write wat file");
+}
+
 /// Instantiate the Wasm buffer, and implicitly fail if we have an unexpected
 /// panic or segfault or anything else that can be detected "passively".
 ///
@@ -400,3 +413,55 @@ pub fn spectest(config: crate::generators::Config, test: crate::generators::Spec
         .run_buffer(test.file, test.contents.as_bytes())
         .unwrap();
 }
+
+/// Execute a series of `table.get` and `table.set` operations.
+pub fn table_ops(config: crate::generators::Config, ops: crate::generators::table_ops::TableOps) {
+    let _ = env_logger::try_init();
+
+    let num_dropped = Rc::new(Cell::new(0));
+
+    {
+        let mut config = config.to_wasmtime();
+        config.wasm_reference_types(true);
+        let engine = Engine::new(&config);
+        let store = Store::new(&engine);
+
+        let wat = ops.to_wat_string();
+        log_wat(&wat);
+        let module = match Module::new(&engine, &wat) {
+            Ok(m) => m,
+            Err(_) => return,
+        };
+
+        // To avoid timeouts, limit the number of explicit GCs we perform per
+        // test case.
+        const MAX_GCS: usize = 5;
+
+        let num_gcs = Cell::new(0);
+        let gc = Func::wrap(&store, move |caller: Caller| {
+            if num_gcs.get() < MAX_GCS {
+                caller.store().gc();
+                num_gcs.set(num_gcs.get() + 1);
+            }
+        });
+
+        let instance = Instance::new(&store, &module, &[gc.into()]).unwrap();
+        let run = instance.get_func("run").unwrap();
+
+        let args: Vec<_> = (0..ops.num_params())
+            .map(|_| Val::ExternRef(Some(ExternRef::new(CountDrops(num_dropped.clone())))))
+            .collect();
+        let _ = run.call(&args);
+    }
+
+    assert_eq!(num_dropped.get(), ops.num_params());
+    return;
+
+    struct CountDrops(Rc<Cell<u8>>);
+
+    impl Drop for CountDrops {
+        fn drop(&mut self) {
+            self.0.set(self.0.get().checked_add(1).unwrap());
+        }
+    }
+}

crates/runtime/src/externref.rs

@@ -901,18 +901,14 @@ impl StackMapRegistry {
             // Exact hit.
             Ok(i) => i,
 
-            Err(n) => {
-                // `Err(0)` means that the associated stack map would have been
-                // the first element in the array if this pc had an associated
-                // stack map, but this pc does not have an associated stack
-                // map. That doesn't make sense since every call and trap inside
-                // Wasm is a GC safepoint and should have a stack map, and the
-                // only way to have Wasm frames under this native frame is if we
-                // are at a call or a trap.
-                debug_assert!(n != 0);
-
-                n - 1
-            }
+            // `Err(0)` means that the associated stack map would have been the
+            // first element in the array if this pc had an associated stack
+            // map, but this pc does not have an associated stack map. This can
+            // only happen inside a Wasm frame if there are no live refs at this
+            // pc.
+            Err(0) => return None,
+
+            Err(n) => n - 1,
         };
 
         let stack_map = stack_maps.pc_to_stack_map[index].1.clone();

crates/wasmtime/src/instance.rs

@@ -52,8 +52,8 @@ fn instantiate(
             config.memory_creator.as_ref().map(|a| a as _),
             store.interrupts().clone(),
             host,
-            &**store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _,
-            &**store.stack_map_registry() as *const StackMapRegistry as *mut _,
+            store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _,
+            store.stack_map_registry() as *const StackMapRegistry as *mut _,
         )?;
 
         // After we've created the `InstanceHandle` we still need to run

crates/wasmtime/src/runtime.rs

@@ -812,8 +812,8 @@ pub(crate) struct StoreInner {
     instances: RefCell<Vec<InstanceHandle>>,
     signal_handler: RefCell<Option<Box<SignalHandler<'static>>>>,
     jit_code_ranges: RefCell<Vec<(usize, usize)>>,
-    externref_activations_table: Rc<VMExternRefActivationsTable>,
-    stack_map_registry: Rc<StackMapRegistry>,
+    externref_activations_table: VMExternRefActivationsTable,
+    stack_map_registry: StackMapRegistry,
 }
 
 struct HostInfoKey(VMExternRef);
@@ -853,8 +853,8 @@ impl Store {
                 instances: RefCell::new(Vec::new()),
                 signal_handler: RefCell::new(None),
                 jit_code_ranges: RefCell::new(Vec::new()),
-                externref_activations_table: Rc::new(VMExternRefActivationsTable::new()),
-                stack_map_registry: Rc::new(StackMapRegistry::default()),
+                externref_activations_table: VMExternRefActivationsTable::new(),
+                stack_map_registry: StackMapRegistry::default(),
             }),
         }
     }
@@ -1090,11 +1090,11 @@ impl Store {
         }
     }
 
-    pub(crate) fn externref_activations_table(&self) -> &Rc<VMExternRefActivationsTable> {
+    pub(crate) fn externref_activations_table(&self) -> &VMExternRefActivationsTable {
         &self.inner.externref_activations_table
     }
 
-    pub(crate) fn stack_map_registry(&self) -> &Rc<StackMapRegistry> {
+    pub(crate) fn stack_map_registry(&self) -> &StackMapRegistry {
         &self.inner.stack_map_registry
     }
 
@@ -1105,8 +1105,8 @@ impl Store {
         // used with this store in `self.inner.stack_map_registry`.
         unsafe {
             wasmtime_runtime::gc(
-                &*self.inner.stack_map_registry,
-                &*self.inner.externref_activations_table,
+                &self.inner.stack_map_registry,
+                &self.inner.externref_activations_table,
             );
         }
     }

crates/wasmtime/src/trampoline/create_handle.rs

@@ -47,8 +47,8 @@ pub(crate) fn create_handle(
             signatures.into_boxed_slice(),
             state,
             store.interrupts().clone(),
-            &**store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _,
-            &**store.stack_map_registry() as *const StackMapRegistry as *mut _,
+            store.externref_activations_table() as *const VMExternRefActivationsTable as *mut _,
+            store.stack_map_registry() as *const StackMapRegistry as *mut _,
         )?;
         Ok(store.add_instance(handle))
     }

fuzz/Cargo.toml

@@ -57,6 +57,12 @@ path = "fuzz_targets/spectests.rs"
 test = false
 doc = false
 
+[[bin]]
+name = "table_ops"
+path = "fuzz_targets/table_ops.rs"
+test = false
+doc = false
+
 [[bin]]
 name = "peepmatic_simple_automata"
 path = "fuzz_targets/peepmatic_simple_automata.rs"

fuzz/fuzz_targets/table_ops.rs

@@ -0,0 +1,9 @@
+#![no_main]
+
+use libfuzzer_sys::fuzz_target;
+use wasmtime_fuzzing::generators::{table_ops::TableOps, Config};
+
+fuzz_target!(|pair: (Config, TableOps)| {
+    let (config, ops) = pair;
+    wasmtime_fuzzing::oracles::table_ops(config, ops);
+});

tests/all/gc.rs

@@ -47,12 +47,9 @@ fn smoke_test_gc() -> anyhow::Result<()> {
         "#,
     )?;
 
-    let do_gc = Func::wrap(&store, {
-        let store = store.clone();
-        move || {
-            // Do a GC with `externref`s on the stack in Wasm frames.
-            store.gc();
-        }
+    let do_gc = Func::wrap(&store, |caller: Caller| {
+        // Do a GC with `externref`s on the stack in Wasm frames.
+        caller.store().gc();
     });
     let instance = Instance::new(&store, &module, &[do_gc.into()])?;
     let func = instance.get_func("func").unwrap();