Remove unchecked_* methods on Rooteds for getting VMGcRefs (byt…

…ecodealliance#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 bytecodealliance#8940
ahaoboy · Jul 12, 2024 · 99b739f · 99b739f
1 parent 9459cf5
commit 99b739f
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 36 deletions.
diff --git 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<HeapType> {
-        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<bool> {
         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<Option<I31>> {
         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<bool> {
-        // 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
@@ -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
@@ -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
@@ -534,7 +534,7 @@ impl StructRef {
     }
 
     pub(crate) fn type_index(&self, store: &StoreOpaque) -> Result<VMSharedTypeIndex> {
-        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
@@ -2614,7 +2614,7 @@ unsafe impl<T> crate::runtime::vm::Store for StoreInner<T> {
             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))