Fix a use-after-free bug when passing ExternRefs to Wasm

We _must not_ trigger a GC when moving refs from host code into
Wasm (e.g. returned from a host function or passed as arguments to a Wasm
function). After insertion into the table, this reference is no longer
rooted. If multiple references are being sent from the host into Wasm and we
allowed GCs during insertion, then the following events could happen:

* Reference A is inserted into the activations table. This does not trigger a
  GC, but does fill the table to capacity.

* The caller's reference to A is removed. Now the only reference to A is from
  the activations table.

* Reference B is inserted into the activations table. Because the table is at
  capacity, a GC is triggered.

* A is reclaimed because the only reference keeping it alive was the activation
  table's reference (it isn't inside any Wasm frames on the stack yet, so stack
  scanning and stack maps don't increment its reference count).

* We transfer control to Wasm, giving it A and B. Wasm uses A. That's a use
  after free.

To prevent uses after free, we cannot GC when moving refs into the
`VMExternRefActivationsTable` because we are passing them from the host to Wasm.

On the other hand, when we are *cloning* -- as opposed to moving -- refs from
the host to Wasm, then it is fine to GC while inserting into the activations
table, because the original referent that we are cloning from is still alive and
rooting the ref.

crates/fuzzing/src/generators/table_ops.rs

@@ -18,7 +18,7 @@ pub struct TableOps {
 
 const NUM_PARAMS_RANGE: Range<u8> = 1..10;
 const TABLE_SIZE_RANGE: Range<u32> = 1..100;
-const MAX_OPS: usize = 1000;
+const MAX_OPS: usize = 100;
 
 impl TableOps {
     /// Get the number of parameters this module's "run" function takes.
@@ -49,9 +49,46 @@ impl TableOps {
     pub fn to_wasm_binary(&self) -> Vec<u8> {
         let mut module = Module::new();
 
+        // Encode the types for all functions that we are using.
+        let mut types = TypeSection::new();
+
+        // 0: "gc"
+        types.function(
+            vec![],
+            // Return a bunch of stuff from `gc` so that we exercise GCing when
+            // there is return pointer space allocated on the stack. This is
+            // especially important because the x64 backend currently
+            // dynamically adjusts the stack pointer for each call that uses
+            // return pointers rather than statically allocating space in the
+            // stack frame.
+            vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
+        );
+
+        // 1: "run"
+        let mut params: Vec<ValType> = Vec::with_capacity(self.num_params() as usize);
+        for _i in 0..self.num_params() {
+            params.push(ValType::ExternRef);
+        }
+        let results = vec![];
+        types.function(params, results);
+
+        // 2: `take_refs`
+        types.function(
+            vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
+            vec![],
+        );
+
+        // 3: `make_refs`
+        types.function(
+            vec![],
+            vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
+        );
+
         // Import the GC function.
         let mut imports = ImportSection::new();
         imports.import("", Some("gc"), EntityType::Function(0));
+        imports.import("", Some("take_refs"), EntityType::Function(2));
+        imports.import("", Some("make_refs"), EntityType::Function(3));
 
         // Define our table.
         let mut tables = TableSection::new();
@@ -61,32 +98,24 @@ impl TableOps {
             maximum: None,
         });
 
-        // Encode the types for all functions that we are using.
-        let mut types = TypeSection::new();
-        types.function(vec![], vec![]); // 0: "gc"
-        let mut params: Vec<ValType> = Vec::with_capacity(self.num_params() as usize);
-        for _i in 0..self.num_params() {
-            params.push(ValType::ExternRef);
-        }
-        let results = vec![];
-        types.function(params, results); // 1: "run"
-
         // Define the "run" function export.
         let mut functions = FunctionSection::new();
         functions.function(1);
 
         let mut exports = ExportSection::new();
-        exports.export("run", Export::Function(1));
+        exports.export("run", Export::Function(3));
 
-        let mut params: Vec<(u32, ValType)> = Vec::with_capacity(self.num_params() as usize);
-        for _i in 0..self.num_params() {
-            params.push((0, ValType::ExternRef));
-        }
-        let mut func = Function::new(params);
+        // Give ourselves one scratch local that we can use in various `TableOp`
+        // implementations.
+        let mut func = Function::new(vec![(1, ValType::ExternRef)]);
 
+        func.instruction(Instruction::Loop(wasm_encoder::BlockType::Empty));
         for op in self.ops.iter().take(MAX_OPS) {
-            op.insert(&mut func);
+            op.insert(&mut func, self.num_params() as u32, self.table_size());
         }
+        func.instruction(Instruction::Br(0));
+        func.instruction(Instruction::End);
+        func.instruction(Instruction::End);
 
         let mut code = CodeSection::new();
         code.function(&func);
@@ -105,7 +134,7 @@ impl TableOps {
 
 #[derive(Arbitrary, Debug)]
 pub(crate) enum TableOp {
-    // `(call 0)`
+    // `call $gc; drop; drop; drop;`
     Gc,
     // `(drop (table.get x))`
     Get(i32),
@@ -113,30 +142,102 @@ pub(crate) enum TableOp {
     SetFromParam(i32, u32),
     // `(table.set x (table.get y))`
     SetFromGet(i32, i32),
+    // `call $make_refs; table.set x; table.set y; table.set z`
+    SetFromMake(i32, i32, i32),
+    // `call $make_refs; drop; drop; drop;`
+    Make,
+    // `local.get x; local.get y; local.get z; call $take_refs`
+    TakeFromParams(u32, u32, u32),
+    // `table.get x; table.get y; table.get z; call $take_refs`
+    TakeFromGet(i32, i32, i32),
+    // `call $make_refs; call $take_refs`
+    TakeFromMake,
+    // `call $gc; call $take_refs`
+    TakeFromGc,
 }
 
 impl TableOp {
-    fn insert(&self, func: &mut Function) {
+    fn insert(&self, func: &mut Function, num_params: u32, table_size: u32) {
+        assert!(num_params > 0);
+        assert!(table_size > 0);
+
+        // Add one to make sure that out of bounds table accesses are possible,
+        // but still rare.
+        let table_mod = table_size as i32 + 1;
+
         match self {
             Self::Gc => {
                 func.instruction(Instruction::Call(0));
+                func.instruction(Instruction::Drop);
+                func.instruction(Instruction::Drop);
+                func.instruction(Instruction::Drop);
             }
             Self::Get(x) => {
-                func.instruction(Instruction::I32Const(*x));
+                func.instruction(Instruction::I32Const(*x % table_mod));
                 func.instruction(Instruction::TableGet { table: 0 });
                 func.instruction(Instruction::Drop);
             }
             Self::SetFromParam(x, y) => {
-                func.instruction(Instruction::I32Const(*x));
-                func.instruction(Instruction::LocalGet(*y));
+                func.instruction(Instruction::I32Const(*x % table_mod));
+                func.instruction(Instruction::LocalGet(*y % num_params));
                 func.instruction(Instruction::TableSet { table: 0 });
             }
             Self::SetFromGet(x, y) => {
-                func.instruction(Instruction::I32Const(*x));
-                func.instruction(Instruction::I32Const(*y));
+                func.instruction(Instruction::I32Const(*x % table_mod));
+                func.instruction(Instruction::I32Const(*y % table_mod));
                 func.instruction(Instruction::TableGet { table: 0 });
                 func.instruction(Instruction::TableSet { table: 0 });
             }
+            Self::SetFromMake(x, y, z) => {
+                func.instruction(Instruction::Call(2));
+
+                func.instruction(Instruction::LocalSet(num_params));
+                func.instruction(Instruction::I32Const(*x % table_mod));
+                func.instruction(Instruction::LocalGet(num_params));
+                func.instruction(Instruction::TableSet { table: 0 });
+
+                func.instruction(Instruction::LocalSet(num_params));
+                func.instruction(Instruction::I32Const(*y % table_mod));
+                func.instruction(Instruction::LocalGet(num_params));
+                func.instruction(Instruction::TableSet { table: 0 });
+
+                func.instruction(Instruction::LocalSet(num_params));
+                func.instruction(Instruction::I32Const(*z % table_mod));
+                func.instruction(Instruction::LocalGet(num_params));
+                func.instruction(Instruction::TableSet { table: 0 });
+            }
+            TableOp::Make => {
+                func.instruction(Instruction::Call(2));
+                func.instruction(Instruction::Drop);
+                func.instruction(Instruction::Drop);
+                func.instruction(Instruction::Drop);
+            }
+            TableOp::TakeFromParams(x, y, z) => {
+                func.instruction(Instruction::LocalGet(x % num_params));
+                func.instruction(Instruction::LocalGet(y % num_params));
+                func.instruction(Instruction::LocalGet(z % num_params));
+                func.instruction(Instruction::Call(1));
+            }
+            TableOp::TakeFromGet(x, y, z) => {
+                func.instruction(Instruction::I32Const(*x % table_mod));
+                func.instruction(Instruction::TableGet { table: 0 });
+
+                func.instruction(Instruction::I32Const(*y % table_mod));
+                func.instruction(Instruction::TableGet { table: 0 });
+
+                func.instruction(Instruction::I32Const(*z % table_mod));
+                func.instruction(Instruction::TableGet { table: 0 });
+
+                func.instruction(Instruction::Call(1));
+            }
+            TableOp::TakeFromMake => {
+                func.instruction(Instruction::Call(2));
+                func.instruction(Instruction::Call(1));
+            }
+            Self::TakeFromGc => {
+                func.instruction(Instruction::Call(0));
+                func.instruction(Instruction::Call(1));
+            }
         }
     }
 }
@@ -148,38 +249,95 @@ mod tests {
     #[test]
     fn test_wat_string() {
         let ops = TableOps {
-            num_params: 2,
-            table_size: 10,
+            num_params: 5,
+            table_size: 20,
             ops: vec![
                 TableOp::Gc,
                 TableOp::Get(0),
                 TableOp::SetFromParam(1, 2),
                 TableOp::SetFromGet(3, 4),
+                TableOp::SetFromMake(5, 6, 7),
+                TableOp::Make,
+                TableOp::TakeFromParams(8, 9, 10),
+                TableOp::TakeFromGet(11, 12, 13),
+                TableOp::TakeFromMake,
             ],
         };
 
         let expected = r#"
 (module
   (type (;0;) (func))
-  (type (;1;) (func (param externref externref)))
+  (type (;1;) (func (param externref externref externref externref externref)))
+  (type (;2;) (func (param externref externref externref)))
+  (type (;3;) (func (result externref externref externref)))
   (import "" "gc" (func (;0;) (type 0)))
-  (func (;1;) (type 1) (param externref externref)
-    call 0
-    i32.const 0
-    table.get 0
-    drop
-    i32.const 1
-    local.get 2
-    table.set 0
-    i32.const 3
-    i32.const 4
-    table.get 0
-    table.set 0)
-  (table (;0;) 10 externref)
-  (export "run" (func 1)))
+  (import "" "take_refs" (func (;1;) (type 2)))
+  (import "" "make_refs" (func (;2;) (type 3)))
+  (func (;3;) (type 1) (param externref externref externref externref externref)
+    (local externref i32)
+    i32.const 100
+    local.set 6
+    loop  ;; label = @1
+      call 0
+      i32.const 0
+      table.get 0
+      drop
+      i32.const 1
+      local.get 2
+      table.set 0
+      i32.const 3
+      i32.const 4
+      table.get 0
+      table.set 0
+      call 2
+      local.set 5
+      i32.const 5
+      local.get 5
+      table.set 0
+      local.set 5
+      i32.const 6
+      local.get 5
+      table.set 0
+      local.set 5
+      i32.const 7
+      local.get 5
+      table.set 0
+      call 2
+      drop
+      drop
+      drop
+      local.get 3
+      local.get 4
+      local.get 0
+      call 1
+      i32.const 11
+      table.get 0
+      i32.const 12
+      table.get 0
+      i32.const 13
+      table.get 0
+      call 1
+      call 2
+      call 1
+      local.get 6
+      i32.const -1
+      i32.add
+      local.tee 6
+      br_if 0 (;@1;)
+    end)
+  (table (;0;) 20 externref)
+  (export "run" (func 3)))
 "#;
+        eprintln!("expected WAT = {}", expected);
+
         let actual = ops.to_wasm_binary();
+        if let Err(e) = wasmparser::validate(&actual) {
+            panic!("TableOps should generate valid Wasm; got error: {}", e);
+        }
+
         let actual = wasmprinter::print_bytes(&actual).unwrap();
+        eprintln!("actual WAT = {}", actual);
+
         assert_eq!(actual.trim(), expected.trim());
     }
 }

crates/fuzzing/src/oracles.rs

@@ -514,16 +514,17 @@ pub fn table_ops(
 ) {
     let _ = env_logger::try_init();
 
+    let expected_drops = Arc::new(AtomicUsize::new(ops.num_params() as usize));
     let num_dropped = Arc::new(AtomicUsize::new(0));
 
     {
         let mut config = fuzz_config.to_wasmtime();
         config.wasm_reference_types(true);
+        config.consume_fuel(true);
+
         let engine = Engine::new(&config).unwrap();
         let mut store = create_store(&engine);
-        if fuzz_config.consume_fuel {
-            store.add_fuel(u64::max_value()).unwrap();
-        }
+        store.add_fuel(100).unwrap();
 
         let wasm = ops.to_wasm_binary();
         log_wasm(&wasm);
@@ -532,18 +533,104 @@ pub fn table_ops(
             Err(_) => return,
         };
 
+        let mut linker = Linker::new(&engine);
+
         // To avoid timeouts, limit the number of explicit GCs we perform per
         // test case.
         const MAX_GCS: usize = 5;
 
         let num_gcs = AtomicUsize::new(0);
-        let gc = Func::wrap(&mut store, move |mut caller: Caller<'_, StoreLimits>| {
-            if num_gcs.fetch_add(1, SeqCst) < MAX_GCS {
-                caller.gc();
-            }
-        });
+        linker
+            .define(
+                "",
+                "gc",
+                // NB: use `Func::new` so that this can still compile on the old x86
+                // backend, where `IntoFunc` isn't implemented for multi-value
+                // returns.
+                Func::new(
+                    &mut store,
+                    FuncType::new(
+                        vec![],
+                        vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
+                    ),
+                    {
+                        let num_dropped = num_dropped.clone();
+                        let expected_drops = expected_drops.clone();
+                        move |mut caller: Caller<'_, StoreLimits>, _params, results| {
+                            if num_gcs.fetch_add(1, SeqCst) < MAX_GCS {
+                                caller.gc();
+                            }
 
-        let instance = Instance::new(&mut store, &module, &[gc.into()]).unwrap();
+                            expected_drops.fetch_add(3, SeqCst);
+                            results[0] =
+                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                            results[1] =
+                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                            results[2] =
+                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                            Ok(())
+                        }
+                    },
+                ),
+            )
+            .unwrap();
+
+        linker
+            .func_wrap("", "take_refs", {
+                let expected_drops = expected_drops.clone();
+                move |a: Option<ExternRef>, b: Option<ExternRef>, c: Option<ExternRef>| {
+                    // Do the assertion on each ref's inner data, even though it
+                    // all points to the same atomic, so that if we happen to
+                    // run into a use-after-free bug with one of these refs we
+                    // are more likely to trigger a segfault.
+                    if let Some(a) = a {
+                        let a = a.data().downcast_ref::<CountDrops>().unwrap();
+                        assert!(a.0.load(SeqCst) <= expected_drops.load(SeqCst));
+                    }
+                    if let Some(b) = b {
+                        let b = b.data().downcast_ref::<CountDrops>().unwrap();
+                        assert!(b.0.load(SeqCst) <= expected_drops.load(SeqCst));
+                    }
+                    if let Some(c) = c {
+                        let c = c.data().downcast_ref::<CountDrops>().unwrap();
+                        assert!(c.0.load(SeqCst) <= expected_drops.load(SeqCst));
+                    }
+                }
+            })
+            .unwrap();
+
+        linker
+            .define(
+                "",
+                "make_refs",
+                // NB: use `Func::new` so that this can still compile on the old
+                // x86 backend, where `IntoFunc` isn't implemented for
+                // multi-value returns.
+                Func::new(
+                    &mut store,
+                    FuncType::new(
+                        vec![],
+                        vec![ValType::ExternRef, ValType::ExternRef, ValType::ExternRef],
+                    ),
+                    {
+                        let num_dropped = num_dropped.clone();
+                        let expected_drops = expected_drops.clone();
+                        move |_caller, _params, results| {
+                            expected_drops.fetch_add(3, SeqCst);
+                            results[0] =
+                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                            results[1] =
+                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                            results[2] =
+                                Some(ExternRef::new(CountDrops(num_dropped.clone()))).into();
+                            Ok(())
+                        }
+                    },
+                ),
+            )
+            .unwrap();
+
+        let instance = linker.instantiate(&mut store, &module).unwrap();
         let run = instance.get_func(&mut store, "run").unwrap();
 
         let args: Vec<_> = (0..ops.num_params())
@@ -552,7 +639,7 @@ pub fn table_ops(
         let _ = run.call(&mut store, &args);
     }
 
-    assert_eq!(num_dropped.load(SeqCst), ops.num_params() as usize);
+    assert_eq!(num_dropped.load(SeqCst), expected_drops.load(SeqCst));
     return;
 
     struct CountDrops(Arc<AtomicUsize>);