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).
This commit is contained in:
Nick Fitzgerald
2022-01-27 13:22:53 -08:00
parent 7928a3ffb4
commit cbc6f6071f
2 changed files with 11 additions and 2 deletions

View File

@@ -463,7 +463,7 @@ impl Deref for VMExternRef {
/// ///
/// We use this so that we can morally put `VMExternRef`s inside of `HashSet`s /// 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. /// even though they don't implement `Eq` and `Hash` to avoid foot guns.
#[derive(Clone)] #[derive(Clone, Debug)]
struct VMExternRefWithTraits(VMExternRef); struct VMExternRefWithTraits(VMExternRef);
impl Hash for VMExternRefWithTraits { impl Hash for VMExternRefWithTraits {
@@ -940,7 +940,9 @@ pub unsafe fn gc(
debug_assert!( debug_assert!(
r.is_null() || activations_table_set.contains(&r), r.is_null() || activations_table_set.contains(&r),
"every on-stack externref inside a Wasm frame should \ "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) { if let Some(r) = NonNull::new(r) {
VMExternRefActivationsTable::insert_precise_stack_root( VMExternRefActivationsTable::insert_precise_stack_root(

View File

@@ -402,6 +402,13 @@ pub unsafe extern "C" fn wasmtime_activations_table_insert_with_gc(
let externref = VMExternRef::clone_from_raw(externref); let externref = VMExternRef::clone_from_raw(externref);
let instance = (*vmctx).instance(); let instance = (*vmctx).instance();
let (activations_table, module_info_lookup) = (*instance.store()).externref_activations_table(); 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); activations_table.insert_with_gc(externref, module_info_lookup);
} }