From cbc6f6071fae3868c750d3873db92501bd655623 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Thu, 27 Jan 2022 13:22:53 -0800 Subject: [PATCH 1/4] Fix a debug assertion in `externref` garbage collections When we GC, we assert the invariant that all `externref`s we find on the stack have a corresponding entry in the `VMExternRefActivationsTable`. However, we also might be in code that is in the process of fixing up this invariant and adding an entry to the table, but the table's bump chunk is full, and so we do a GC and then add the entry into the table. This will ultimately maintain our desired invariant, but there is a moment in time when we are doing the GC where the invariant is relaxed which is okay because the reference will be in the table before we return to Wasm or do anything else. This isn't a possible UAF, in other words. To make it so that the assertion won't trip, we explicitly insert the reference into the table *before* we GC, so that the invariant is not relaxed across a possibly-GCing operation (even though it would be safe in this particular case). --- crates/runtime/src/externref.rs | 6 ++++-- crates/runtime/src/libcalls.rs | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index 9677b39afa7e..573dd4b24f5a 100644 --- 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 index 2544e1673987..841dfaa1a2eb 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -402,6 +402,13 @@ 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. + activations_table.insert_without_gc(externref.clone()); + activations_table.insert_with_gc(externref, module_info_lookup); } From cc8d7778e2739dc7c5058122889853de141e8191 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Thu, 27 Jan 2022 13:59:05 -0800 Subject: [PATCH 2/4] Make the `table_ops` test case generator use globals as well This will make it generate `table.set`s that come from `global.get`s and `global.get`s that come from `table.set`s. Ultimately, it should give us much more fuzzing coverage of `externref` globals, their barriers, and passing `externref`s into and out of Wasm to get or set globals. --- crates/fuzzing/src/generators/table_ops.rs | 152 +++++++++++++++++---- 1 file changed, 127 insertions(+), 25 deletions(-) diff --git a/crates/fuzzing/src/generators/table_ops.rs b/crates/fuzzing/src/generators/table_ops.rs index 0bf3e9ce5c3b..f8104d6f1a2c 100644 --- a/crates/fuzzing/src/generators/table_ops.rs +++ b/crates/fuzzing/src/generators/table_ops.rs @@ -3,8 +3,8 @@ 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 @@ -12,11 +12,13 @@ use wasm_encoder::{ #[derive(Arbitrary, Debug)] pub struct TableOps { num_params: u8, + num_globals: u8, table_size: u32, ops: Vec, } const NUM_PARAMS_RANGE: Range = 1..10; +const NUM_GLOBALS_RANGE: Range = 1..10; const TABLE_SIZE_RANGE: Range = 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,6 +151,7 @@ impl TableOps { .section(&imports) .section(&functions) .section(&tables) + .section(&globals) .section(&exports) .section(&code); @@ -132,32 +159,56 @@ impl TableOps { } } -#[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); @@ -165,49 +216,53 @@ impl TableOp { // 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); From f292ff55cfb8306939fc2e03324007fe015d839e Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Thu, 27 Jan 2022 14:13:16 -0800 Subject: [PATCH 3/4] Do another GC after running Wasm in the `table_ops` test oracle --- crates/fuzzing/src/oracles.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/fuzzing/src/oracles.rs b/crates/fuzzing/src/oracles.rs index 3b443e5e1ded..c1feba550986 100644 --- 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)); From 19f8d94959a78d6251a51c74195232e56a02a865 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Fri, 28 Jan 2022 09:47:05 -0800 Subject: [PATCH 4/4] Expand on activations table invariants comment in `libcalls.rs` --- crates/runtime/src/libcalls.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index 841dfaa1a2eb..a6b29116727c 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -406,7 +406,10 @@ pub unsafe extern "C" fn wasmtime_activations_table_insert_with_gc( // 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. + // 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);