Merge pull request #3734 from fitzgen/externref-debug-assertion-fix

fitzgen · web-flow · commit 34537a39cda9 · 2022-01-28T11:03:03.000-08:00
Fix a debug assertion in `externref` garbage collections
diff --git a/crates/fuzzing/src/generators/table_ops.rs b/crates/fuzzing/src/generators/table_ops.rs
@@ -3,20 +3,22 @@
 use arbitrary::Arbitrary;
 use std::ops::Range;
 use wasm_encoder::{
-    CodeSection, EntityType, Export, ExportSection, Function, FunctionSection, ImportSection,
-    Instruction, Module, TableSection, TableType, TypeSection, ValType,
+    CodeSection, EntityType, Export, ExportSection, Function, FunctionSection, GlobalSection,
+    ImportSection, Instruction, Module, TableSection, TableType, TypeSection, ValType,
 };
 
 /// A description of a Wasm module that makes a series of `externref` table
 /// operations.
 #[derive(Arbitrary, Debug)]
 pub struct TableOps {
     num_params: u8,
+    num_globals: u8,
     table_size: u32,
     ops: Vec<TableOp>,
 }
 
 const NUM_PARAMS_RANGE: Range<u8> = 1..10;
+const NUM_GLOBALS_RANGE: Range<u8> = 1..10;
 const TABLE_SIZE_RANGE: Range<u32> = 1..100;
 const MAX_OPS: usize = 100;
 
@@ -28,6 +30,13 @@ impl TableOps {
         num_params
     }
 
+    /// Get the number of globals this module has.
+    pub fn num_globals(&self) -> u8 {
+        let num_globals = std::cmp::max(self.num_globals, NUM_GLOBALS_RANGE.start);
+        let num_globals = std::cmp::min(num_globals, NUM_GLOBALS_RANGE.end);
+        num_globals
+    }
+
     /// 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);
@@ -98,6 +107,18 @@ impl TableOps {
             maximum: None,
         });
 
+        // Define our globals.
+        let mut globals = GlobalSection::new();
+        for _ in 0..self.num_globals() {
+            globals.global(
+                wasm_encoder::GlobalType {
+                    val_type: wasm_encoder::ValType::ExternRef,
+                    mutable: true,
+                },
+                Instruction::RefNull(wasm_encoder::ValType::ExternRef),
+            );
+        }
+
         // Define the "run" function export.
         let mut functions = FunctionSection::new();
         functions.function(1);
@@ -111,7 +132,12 @@ impl TableOps {
 
         func.instruction(Instruction::Loop(wasm_encoder::BlockType::Empty));
         for op in self.ops.iter().take(MAX_OPS) {
-            op.insert(&mut func, self.num_params() as u32, self.table_size());
+            op.insert(
+                &mut func,
+                self.num_params() as u32,
+                self.table_size(),
+                self.num_globals() as u32,
+            );
         }
         func.instruction(Instruction::Br(0));
         func.instruction(Instruction::End);
@@ -125,89 +151,118 @@ impl TableOps {
             .section(&imports)
             .section(&functions)
             .section(&tables)
+            .section(&globals)
             .section(&exports)
             .section(&code);
 
         module.finish()
     }
 }
 
-#[derive(Arbitrary, Debug)]
+#[derive(Arbitrary, Copy, Clone, Debug)]
 pub(crate) enum TableOp {
     // `call $gc; drop; drop; drop;`
     Gc,
+
     // `(drop (table.get x))`
     Get(i32),
+
+    // `(drop (global.get i))`
+    GetGlobal(u32),
+
     // `(table.set x (local.get y))`
     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),
+
+    // `(global.set x (local.get y))`
+    SetGlobalFromParam(u32, u32),
+
+    // `(global.set x (table.get y))`
+    SetGlobalFromGet(u32, i32),
+
+    // `call $make_refs; global.set x; global.set y; global.set z`
+    SetGlobalFromMake(u32, u32, u32),
+
     // `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),
+
+    // `global.get x; global.get y; global.get z; call $take_refs`
+    TakeFromGlobalGet(u32, u32, u32),
+
     // `call $make_refs; call $take_refs`
     TakeFromMake,
+
     // `call $gc; call $take_refs`
     TakeFromGc,
 }
 
 impl TableOp {
-    fn insert(&self, func: &mut Function, num_params: u32, table_size: u32) {
+    fn insert(self, func: &mut Function, num_params: u32, table_size: u32, num_globals: 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;
 
+        let gc_func_idx = 0;
+        let take_refs_func_idx = 1;
+        let make_refs_func_idx = 2;
+
         match self {
             Self::Gc => {
-                func.instruction(Instruction::Call(0));
+                func.instruction(Instruction::Call(gc_func_idx));
                 func.instruction(Instruction::Drop);
                 func.instruction(Instruction::Drop);
                 func.instruction(Instruction::Drop);
             }
             Self::Get(x) => {
-                func.instruction(Instruction::I32Const(*x % table_mod));
+                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 % table_mod));
-                func.instruction(Instruction::LocalGet(*y % num_params));
+                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 % table_mod));
-                func.instruction(Instruction::I32Const(*y % table_mod));
+                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::Call(make_refs_func_idx));
 
                 func.instruction(Instruction::LocalSet(num_params));
-                func.instruction(Instruction::I32Const(*x % table_mod));
+                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::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::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::Call(make_refs_func_idx));
                 func.instruction(Instruction::Drop);
                 func.instruction(Instruction::Drop);
                 func.instruction(Instruction::Drop);
@@ -216,27 +271,52 @@ impl TableOp {
                 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));
+                func.instruction(Instruction::Call(take_refs_func_idx));
             }
             TableOp::TakeFromGet(x, y, z) => {
-                func.instruction(Instruction::I32Const(*x % table_mod));
+                func.instruction(Instruction::I32Const(x % table_mod));
                 func.instruction(Instruction::TableGet { table: 0 });
 
-                func.instruction(Instruction::I32Const(*y % table_mod));
+                func.instruction(Instruction::I32Const(y % table_mod));
                 func.instruction(Instruction::TableGet { table: 0 });
 
-                func.instruction(Instruction::I32Const(*z % table_mod));
+                func.instruction(Instruction::I32Const(z % table_mod));
                 func.instruction(Instruction::TableGet { table: 0 });
 
-                func.instruction(Instruction::Call(1));
+                func.instruction(Instruction::Call(take_refs_func_idx));
             }
             TableOp::TakeFromMake => {
-                func.instruction(Instruction::Call(2));
-                func.instruction(Instruction::Call(1));
+                func.instruction(Instruction::Call(make_refs_func_idx));
+                func.instruction(Instruction::Call(take_refs_func_idx));
             }
             Self::TakeFromGc => {
-                func.instruction(Instruction::Call(0));
-                func.instruction(Instruction::Call(1));
+                func.instruction(Instruction::Call(gc_func_idx));
+                func.instruction(Instruction::Call(take_refs_func_idx));
+            }
+            TableOp::GetGlobal(x) => {
+                func.instruction(Instruction::GlobalGet(x % num_globals));
+                func.instruction(Instruction::Drop);
+            }
+            TableOp::SetGlobalFromParam(global, param) => {
+                func.instruction(Instruction::LocalGet(param % num_params));
+                func.instruction(Instruction::GlobalSet(global % num_globals));
+            }
+            TableOp::SetGlobalFromGet(global, x) => {
+                func.instruction(Instruction::I32Const(x));
+                func.instruction(Instruction::TableGet { table: 0 });
+                func.instruction(Instruction::GlobalSet(global % num_globals));
+            }
+            TableOp::SetGlobalFromMake(x, y, z) => {
+                func.instruction(Instruction::Call(make_refs_func_idx));
+                func.instruction(Instruction::GlobalSet(x % num_globals));
+                func.instruction(Instruction::GlobalSet(y % num_globals));
+                func.instruction(Instruction::GlobalSet(z % num_globals));
+            }
+            TableOp::TakeFromGlobalGet(x, y, z) => {
+                func.instruction(Instruction::GlobalGet(x % num_globals));
+                func.instruction(Instruction::GlobalGet(y % num_globals));
+                func.instruction(Instruction::GlobalGet(z % num_globals));
+                func.instruction(Instruction::Call(take_refs_func_idx));
             }
         }
     }
@@ -250,6 +330,7 @@ mod tests {
     fn test_wat_string() {
         let ops = TableOps {
             num_params: 5,
+            num_globals: 1,
             table_size: 20,
             ops: vec![
                 TableOp::Gc,
@@ -261,6 +342,11 @@ mod tests {
                 TableOp::TakeFromParams(8, 9, 10),
                 TableOp::TakeFromGet(11, 12, 13),
                 TableOp::TakeFromMake,
+                TableOp::GetGlobal(14),
+                TableOp::SetGlobalFromParam(15, 16),
+                TableOp::SetGlobalFromGet(17, 18),
+                TableOp::SetGlobalFromMake(19, 20, 21),
+                TableOp::TakeFromGlobalGet(22, 23, 24),
             ],
         };
 
@@ -320,9 +406,25 @@ mod tests {
       call 1
       call 2
       call 1
+      global.get 0
+      drop
+      local.get 1
+      global.set 0
+      i32.const 18
+      table.get 0
+      global.set 0
+      call 2
+      global.set 0
+      global.set 0
+      global.set 0
+      global.get 0
+      global.get 0
+      global.get 0
+      call 1
       br 0 (;@1;)
     end)
   (table (;0;) 20 externref)
+  (global (;0;) (mut externref) (ref.null extern))
   (export "run" (func 3)))
 "#;
         eprintln!("expected WAT = {}", expected);
diff --git a/crates/fuzzing/src/oracles.rs b/crates/fuzzing/src/oracles.rs
@@ -540,6 +540,9 @@ pub fn table_ops(mut fuzz_config: generators::Config, ops: generators::table_ops
             .map(|_| Val::ExternRef(Some(ExternRef::new(CountDrops(num_dropped.clone())))))
             .collect();
         let _ = run.call(&mut store, &args, &mut []);
+
+        // Do a final GC after running the Wasm.
+        store.gc();
     }
 
     assert_eq!(num_dropped.load(SeqCst), expected_drops.load(SeqCst));
diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs
@@ -463,7 +463,7 @@ impl Deref for VMExternRef {
 ///
 /// We use this so that we can morally put `VMExternRef`s inside of `HashSet`s
 /// even though they don't implement `Eq` and `Hash` to avoid foot guns.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 struct VMExternRefWithTraits(VMExternRef);
 
 impl Hash for VMExternRefWithTraits {
@@ -940,7 +940,9 @@ pub unsafe fn gc(
                         debug_assert!(
                             r.is_null() || activations_table_set.contains(&r),
                             "every on-stack externref inside a Wasm frame should \
-                            have an entry in the VMExternRefActivationsTable"
+                            have an entry in the VMExternRefActivationsTable; \
+                            {:?} is not in the table",
+                            r
                         );
                         if let Some(r) = NonNull::new(r) {
                             VMExternRefActivationsTable::insert_precise_stack_root(
diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs
@@ -402,6 +402,16 @@ pub unsafe extern "C" fn wasmtime_activations_table_insert_with_gc(
     let externref = VMExternRef::clone_from_raw(externref);
     let instance = (*vmctx).instance();
     let (activations_table, module_info_lookup) = (*instance.store()).externref_activations_table();
+
+    // Invariant: all `externref`s on the stack have an entry in the activations
+    // table. So we need to ensure that this `externref` is in the table
+    // *before* we GC, even though `insert_with_gc` will ensure that it is in
+    // the table *after* the GC. This technically results in one more hash table
+    // look up than is strictly necessary -- which we could avoid by having an
+    // additional GC method that is aware of these GC-triggering references --
+    // but it isn't really a concern because this is already a slow path.
+    activations_table.insert_without_gc(externref.clone());
+
     activations_table.insert_with_gc(externref, module_info_lookup);
 }
 

Original file line number	Diff line number	Diff line change
`@@ -540,6 +540,9 @@ pub fn table_ops(mut fuzz_config: generators::Config, ops: generators::table_ops`
`540`	`540`	`.map(\|_\| Val::ExternRef(Some(ExternRef::new(CountDrops(num_dropped.clone())))))`
`541`	`541`	`.collect();`
`542`	`542`	`let _ = run.call(&mut store, &args, &mut []);`
	`543`	`+`
	`544`	`+ // Do a final GC after running the Wasm.`
	`545`	`+ store.gc();`
`543`	`546`	`}`
`544`	`547`
`545`	`548`	`assert_eq!(num_dropped.load(SeqCst), expected_drops.load(SeqCst));`