From 99b739fb3cb0b885f6ab1298a8eaa67150d82e58 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Fri, 12 Jul 2024 15:18:19 -0700 Subject: [PATCH] Remove `unchecked_*` methods on `Rooted`s for getting `VMGcRef`s (#8948) Because they take a shared borrow of the store and return a shared borrow of the `VMGcRef` with the same lifetime, and because performing a GC requires a mutable borrow of the store, there actually was no reason to differentiate between checked and unchecked methods or require `AutoAssertNoGc` arguments. Fixes https://github.com/bytecodealliance/wasmtime/issues/8940 --- .../wasmtime/src/runtime/gc/enabled/anyref.rs | 14 ++----- .../src/runtime/gc/enabled/externref.rs | 7 +++- .../src/runtime/gc/enabled/rooting.rs | 40 +++++++++---------- .../src/runtime/gc/enabled/structref.rs | 2 +- crates/wasmtime/src/runtime/store.rs | 2 +- 5 files changed, 29 insertions(+), 36 deletions(-) diff --git a/crates/wasmtime/src/runtime/gc/enabled/anyref.rs b/crates/wasmtime/src/runtime/gc/enabled/anyref.rs index cea735678a..0ba936b976 100644 --- a/crates/wasmtime/src/runtime/gc/enabled/anyref.rs +++ b/crates/wasmtime/src/runtime/gc/enabled/anyref.rs @@ -253,7 +253,7 @@ impl AnyRef { } pub(crate) fn _ty(&self, store: &StoreOpaque) -> Result { - let gc_ref = self.inner.unchecked_try_gc_ref(store)?; + let gc_ref = self.inner.try_gc_ref(store)?; if gc_ref.is_i31() { return Ok(HeapType::I31); } @@ -323,9 +323,7 @@ impl AnyRef { pub(crate) fn _is_i31(&self, store: &StoreOpaque) -> Result { assert!(self.comes_from_same_store(store)); - // NB: Can't use `AutoAssertNoGc` here because we only have a shared - // context, not a mutable context. - let gc_ref = self.inner.unchecked_try_gc_ref(store)?; + let gc_ref = self.inner.try_gc_ref(store)?; Ok(gc_ref.is_i31()) } @@ -348,9 +346,7 @@ impl AnyRef { pub(crate) fn _as_i31(&self, store: &StoreOpaque) -> Result> { assert!(self.comes_from_same_store(store)); - // NB: Can't use `AutoAssertNoGc` here because we only have a shared - // context, not a mutable context. - let gc_ref = self.inner.unchecked_try_gc_ref(store)?; + let gc_ref = self.inner.try_gc_ref(store)?; Ok(gc_ref.as_i31().map(Into::into)) } @@ -383,9 +379,7 @@ impl AnyRef { } pub(crate) fn _is_struct(&self, store: &StoreOpaque) -> Result { - // NB: Can't use `AutoAssertNoGc` here because we only have a shared - // context, not a mutable context. - let gc_ref = self.inner.unchecked_try_gc_ref(store)?; + let gc_ref = self.inner.try_gc_ref(store)?; Ok(!gc_ref.is_i31() && store.gc_store()?.kind(gc_ref).matches(VMGcKind::StructRef)) } diff --git a/crates/wasmtime/src/runtime/gc/enabled/externref.rs b/crates/wasmtime/src/runtime/gc/enabled/externref.rs index 5a8033f284..1cbdeac0a9 100644 --- a/crates/wasmtime/src/runtime/gc/enabled/externref.rs +++ b/crates/wasmtime/src/runtime/gc/enabled/externref.rs @@ -319,7 +319,7 @@ impl ExternRef { T: 'a, { let store = store.into().0; - let gc_ref = self.inner.unchecked_try_gc_ref(&store)?; + let gc_ref = self.inner.try_gc_ref(&store)?; let externref = gc_ref.as_externref_unchecked(); Ok(store.gc_store()?.externref_host_data(externref)) } @@ -359,7 +359,10 @@ impl ExternRef { T: 'a, { let store = store.into().0; - let gc_ref = self.inner.unchecked_try_gc_ref(store)?.unchecked_copy(); + // NB: need to do an unchecked copy to release the borrow on the store + // so that we can get the store's GC store. But importantly we cannot + // trigger a GC while we are working with `gc_ref` here. + let gc_ref = self.inner.try_gc_ref(store)?.unchecked_copy(); let externref = gc_ref.as_externref_unchecked(); Ok(store.gc_store_mut()?.externref_host_data_mut(externref)) } diff --git a/crates/wasmtime/src/runtime/gc/enabled/rooting.rs b/crates/wasmtime/src/runtime/gc/enabled/rooting.rs index 380539fe31..7f218df6d5 100644 --- a/crates/wasmtime/src/runtime/gc/enabled/rooting.rs +++ b/crates/wasmtime/src/runtime/gc/enabled/rooting.rs @@ -209,13 +209,23 @@ impl GcRootIndex { self.store_id == store.id() } - /// Same as `RootedGcRefImpl::get_gc_ref` but doesn't check that the raw GC - /// ref is only used during the scope of an `AutoAssertNoGc`. + /// Same as `RootedGcRefImpl::get_gc_ref` but not associated with any + /// particular `T: GcRef`. /// - /// It is up to callers to avoid triggering a GC while holding onto the - /// resulting raw `VMGcRef`. Failure to uphold this invariant is memory safe - /// but will lead to general incorrectness such as panics and wrong results. - pub(crate) fn unchecked_get_gc_ref<'a>(&self, store: &'a StoreOpaque) -> Option<&'a VMGcRef> { + /// We must avoid triggering a GC while holding onto the resulting raw + /// `VMGcRef` to avoid use-after-free bugs and similar. The `'a` lifetime + /// threaded from the `store` to the result will normally prevent GCs + /// statically, at compile time, since performing a GC requires a mutable + /// borrow of the store. However, if you call `VMGcRef::unchecked_copy` on + /// the resulting GC reference, then all bets are off and this invariant is + /// up to you to manually uphold. Failure to uphold this invariant is memory + /// safe but will lead to general incorrectness such as panics and wrong + /// results. + /// + /// # Panics + /// + /// Panics if `self` is not associated with the given store. + pub(crate) fn get_gc_ref<'a>(&self, store: &'a StoreOpaque) -> Option<&'a VMGcRef> { assert!( self.comes_from_same_store(store), "object used with wrong store" @@ -236,27 +246,13 @@ impl GcRootIndex { } } - /// Same as `RootedGcRefImpl::get_gc_ref` but not associated with any - /// particular `T: GcRef`. - pub(crate) fn get_gc_ref<'a>(&self, store: &'a AutoAssertNoGc<'_>) -> Option<&'a VMGcRef> { - self.unchecked_get_gc_ref(store) - } - - /// Same as `unchecked_get_gc_ref` but returns an error instead of `None` if + /// Same as `get_gc_ref` but returns an error instead of `None` if /// the GC reference has been unrooted. /// /// # Panics /// /// Panics if `self` is not associated with the given store. - pub(crate) fn unchecked_try_gc_ref<'a>(&self, store: &'a StoreOpaque) -> Result<&'a VMGcRef> { - self.unchecked_get_gc_ref(store).ok_or_else(|| { - anyhow!("attempted to use a garbage-collected object that has been unrooted") - }) - } - - /// Same as `get_gc_ref` but returns an error instead of `None` if the GC - /// reference has been unrooted. - pub(crate) fn try_gc_ref<'a>(&self, store: &'a AutoAssertNoGc<'_>) -> Result<&'a VMGcRef> { + pub(crate) fn try_gc_ref<'a>(&self, store: &'a StoreOpaque) -> Result<&'a VMGcRef> { self.get_gc_ref(store).ok_or_else(|| { anyhow!("attempted to use a garbage-collected object that has been unrooted") }) diff --git a/crates/wasmtime/src/runtime/gc/enabled/structref.rs b/crates/wasmtime/src/runtime/gc/enabled/structref.rs index 12cdc797a2..d0ae4aeb77 100644 --- a/crates/wasmtime/src/runtime/gc/enabled/structref.rs +++ b/crates/wasmtime/src/runtime/gc/enabled/structref.rs @@ -534,7 +534,7 @@ impl StructRef { } pub(crate) fn type_index(&self, store: &StoreOpaque) -> Result { - let gc_ref = self.inner.unchecked_try_gc_ref(store)?; + let gc_ref = self.inner.try_gc_ref(store)?; let header = store.gc_store()?.header(gc_ref); debug_assert!(header.kind().matches(VMGcKind::StructRef)); Ok(header.ty().expect("structrefs should have concrete types")) diff --git a/crates/wasmtime/src/runtime/store.rs b/crates/wasmtime/src/runtime/store.rs index 71436b12f6..7eab04580f 100644 --- a/crates/wasmtime/src/runtime/store.rs +++ b/crates/wasmtime/src/runtime/store.rs @@ -2614,7 +2614,7 @@ unsafe impl crate::runtime::vm::Store for StoreInner { None => None, Some(r) => { let r = r - .unchecked_get_gc_ref(store) + .get_gc_ref(store) .expect("still in scope") .unchecked_copy(); Some(store.gc_store_mut()?.clone_gc_ref(&r))