From cbc6f6071fae3868c750d3873db92501bd655623 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Thu, 27 Jan 2022 13:22:53 -0800 Subject: [PATCH] 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 9677b39afa..573dd4b24f 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 2544e16739..841dfaa1a2 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); }