Make Val::to_raw a safe function (#11319)

This commit updates Wasmtime's core `Val::to_raw` function a safe
function. This was previously marked as `unsafe` with documentation that
the raw pointer could be invalid, but that's not a reason for the
function itself to be `unsafe`. Usage of the returned value is still
`unsafe`, but simply acquiring the value is not itself an unsafe
operation.

This additionally marks a number of GC-related `from_raw` functions as
safe. Wasmtime's GC is safe in the face of heap corruption, so it's
memory safe to pass in any 32-bit value. Documentation still indicates
that panics are possible, however.

Author: alexcrichton

crates/wasmtime/src/runtime/func.rs

@@ -1015,7 +1015,8 @@ impl Func {
     /// Converts the raw representation of a `funcref` into an `Option<Func>`
     ///
     /// This is intended to be used in conjunction with [`Func::new_unchecked`],
-    /// [`Func::call_unchecked`], and [`ValRaw`] with its `funcref` field.
+    /// [`Func::call_unchecked`], and [`ValRaw`] with its `funcref` field. This
+    /// is the dual of [`Func::to_raw`].
     ///
     /// # Unsafety
     ///
@@ -1038,12 +1039,12 @@ impl Func {
     /// This function returns a value that's suitable for writing into the
     /// `funcref` field of the [`ValRaw`] structure.
     ///
-    /// # Unsafety
+    /// # Safety
     ///
-    /// The returned value is only valid for as long as the store is alive and
-    /// this function is properly rooted within it. Additionally this function
-    /// should not be liberally used since it's a very low-level knob.
-    pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> *mut c_void {
+    /// The returned value is only valid for as long as the store is alive.
+    /// This value is safe to pass to [`Func::from_raw`] so long as the same
+    /// `store` is provided.
+    pub fn to_raw(&self, mut store: impl AsContextMut) -> *mut c_void {
         self.vm_func_ref(store.as_context_mut().0).as_ptr().cast()
     }
 
@@ -1155,9 +1156,7 @@ impl Func {
         debug_assert!(values_vec.is_empty());
         values_vec.resize_with(values_vec_size, || ValRaw::v128(0));
         for (arg, slot) in params.iter().cloned().zip(&mut values_vec) {
-            unsafe {
-                *slot = arg.to_raw(&mut *store)?;
-            }
+            *slot = arg.to_raw(&mut *store)?;
         }
 
         unsafe {
@@ -1255,9 +1254,7 @@ impl Func {
         for (i, (ret, ty)) in results.iter().zip(ty.results()).enumerate() {
             ret.ensure_matches_ty(caller.store.0, &ty)
                 .context("function attempted to return an incompatible value")?;
-            unsafe {
-                values_vec[i] = ret.to_raw(&mut caller.store)?;
-            }
+            values_vec[i] = ret.to_raw(&mut caller.store)?;
         }
 
         // Restore our `val_vec` back into the store so it's usable for the next

crates/wasmtime/src/runtime/gc/disabled/anyref.rs

@@ -61,17 +61,17 @@ impl AnyRef {
         unreachable!()
     }
 
-    pub unsafe fn from_raw(_store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
+    pub fn from_raw(_store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
         assert_eq!(raw, 0);
         None
     }
 
-    pub unsafe fn _from_raw(_store: &mut AutoAssertNoGc<'_>, raw: u32) -> Option<Rooted<Self>> {
+    pub fn _from_raw(_store: &mut AutoAssertNoGc<'_>, raw: u32) -> Option<Rooted<Self>> {
         assert_eq!(raw, 0);
         None
     }
 
-    pub unsafe fn to_raw(&self, _store: impl AsContextMut) -> Result<u32> {
+    pub fn to_raw(&self, _store: impl AsContextMut) -> Result<u32> {
         match *self {}
     }
 

crates/wasmtime/src/runtime/gc/disabled/exnref.rs

@@ -16,19 +16,19 @@ pub enum ExnRef {}
 impl GcRefImpl for ExnRef {}
 
 impl ExnRef {
-    pub unsafe fn from_raw(_store: impl AsContextMut, _raw: u32) -> Option<Rooted<Self>> {
+    pub fn from_raw(_store: impl AsContextMut, _raw: u32) -> Option<Rooted<Self>> {
         None
     }
 
     pub(crate) fn _from_raw(_store: &mut AutoAssertNoGc, _raw: u32) -> Option<Rooted<Self>> {
         None
     }
 
-    pub unsafe fn to_raw(&self, _store: impl AsContextMut) -> Result<u32> {
+    pub fn to_raw(&self, _store: impl AsContextMut) -> Result<u32> {
         Ok(0)
     }
 
-    pub(crate) unsafe fn _to_raw(&self, _store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
+    pub(crate) fn _to_raw(&self, _store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
         Ok(0)
     }
 

crates/wasmtime/src/runtime/gc/disabled/externref.rs

@@ -38,17 +38,17 @@ impl ExternRef {
         match *self {}
     }
 
-    pub unsafe fn from_raw(_store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
+    pub fn from_raw(_store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
         assert_eq!(raw, 0);
         None
     }
 
-    pub unsafe fn _from_raw(_store: &mut AutoAssertNoGc<'_>, raw: u32) -> Option<Rooted<Self>> {
+    pub fn _from_raw(_store: &mut AutoAssertNoGc<'_>, raw: u32) -> Option<Rooted<Self>> {
         assert_eq!(raw, 0);
         None
     }
 
-    pub unsafe fn to_raw(&self, _store: impl AsContextMut) -> Result<u32> {
+    pub fn to_raw(&self, _store: impl AsContextMut) -> Result<u32> {
         match *self {}
     }
 }

crates/wasmtime/src/runtime/gc/enabled/anyref.rs

@@ -249,27 +249,30 @@ impl AnyRef {
     /// This function assumes that `raw` is an `anyref` value which is currently
     /// rooted within the [`Store`].
     ///
-    /// # Unsafety
+    /// # Correctness
     ///
-    /// This function is particularly `unsafe` because `raw` not only must be a
-    /// valid `anyref` value produced prior by [`AnyRef::to_raw`] but it must
-    /// also be correctly rooted within the store. When arguments are provided
-    /// to a callback with [`Func::new_unchecked`], for example, or returned via
-    /// [`Func::call_unchecked`], if a GC is performed within the store then
-    /// floating `anyref` values are not rooted and will be GC'd, meaning that
-    /// this function will no longer be safe to call with the values cleaned up.
-    /// This function must be invoked *before* possible GC operations can happen
-    /// (such as calling Wasm).
+    /// This function is tricky to get right because `raw` not only must be a
+    /// valid `anyref` value produced prior by [`AnyRef::to_raw`] but it
+    /// must also be correctly rooted within the store. When arguments are
+    /// provided to a callback with [`Func::new_unchecked`], for example, or
+    /// returned via [`Func::call_unchecked`], if a GC is performed within the
+    /// store then floating `anyref` values are not rooted and will be GC'd,
+    /// meaning that this function will no longer be correct to call with the
+    /// values cleaned up. This function must be invoked *before* possible GC
+    /// operations can happen (such as calling Wasm).
     ///
-    /// When in doubt try to not use this. Instead use the safe Rust APIs of
-    /// [`TypedFunc`] and friends.
+    ///
+    /// When in doubt try to not use this. Instead use the Rust APIs of
+    /// [`TypedFunc`] and friends. Note though that this function is not
+    /// `unsafe` as any value can be passed in. Incorrect values can result in
+    /// runtime panics, however, so care must still be taken with this method.
     ///
     /// [`Func::call_unchecked`]: crate::Func::call_unchecked
     /// [`Func::new_unchecked`]: crate::Func::new_unchecked
     /// [`Store`]: crate::Store
     /// [`TypedFunc`]: crate::TypedFunc
     /// [`ValRaw`]: crate::ValRaw
-    pub unsafe fn from_raw(mut store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
+    pub fn from_raw(mut store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
         let mut store = AutoAssertNoGc::new(store.as_context_mut().0);
         Self::_from_raw(&mut store, raw)
     }
@@ -320,19 +323,19 @@ impl AnyRef {
     ///
     /// Returns an error if this `anyref` has been unrooted.
     ///
-    /// # Unsafety
+    /// # Correctness
     ///
-    /// Produces a raw value which is only safe to pass into a store if a GC
+    /// Produces a raw value which is only valid to pass into a store if a GC
     /// doesn't happen between when the value is produce and when it's passed
     /// into the store.
     ///
     /// [`ValRaw`]: crate::ValRaw
-    pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> Result<u32> {
+    pub fn to_raw(&self, mut store: impl AsContextMut) -> Result<u32> {
         let mut store = AutoAssertNoGc::new(store.as_context_mut().0);
         self._to_raw(&mut store)
     }
 
-    pub(crate) unsafe fn _to_raw(&self, store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
+    pub(crate) fn _to_raw(&self, store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
         let gc_ref = self.inner.try_clone_gc_ref(store)?;
         let raw = if gc_ref.is_i31() {
             gc_ref.as_raw_non_zero_u32()

crates/wasmtime/src/runtime/gc/enabled/exnref.rs

@@ -136,27 +136,29 @@ impl ExnRef {
     /// This function assumes that `raw` is an `exnref` value which is currently
     /// rooted within the [`Store`].
     ///
-    /// # Unsafety
+    /// # Correctness
     ///
-    /// This function is particularly `unsafe` because `raw` not only must be a
+    /// This function is tricky to get right because `raw` not only must be a
     /// valid `exnref` value produced prior by [`ExnRef::to_raw`] but it must
     /// also be correctly rooted within the store. When arguments are provided
     /// to a callback with [`Func::new_unchecked`], for example, or returned via
     /// [`Func::call_unchecked`], if a GC is performed within the store then
     /// floating `exnref` values are not rooted and will be GC'd, meaning that
-    /// this function will no longer be safe to call with the values cleaned up.
-    /// This function must be invoked *before* possible GC operations can happen
-    /// (such as calling Wasm).
+    /// this function will no longer be correct to call with the values cleaned
+    /// up. This function must be invoked *before* possible GC operations can
+    /// happen (such as calling Wasm).
     ///
-    /// When in doubt try to not use this. Instead use the safe Rust APIs of
-    /// [`TypedFunc`] and friends.
+    /// When in doubt try to not use this. Instead use the Rust APIs of
+    /// [`TypedFunc`] and friends. Note though that this function is not
+    /// `unsafe` as any value can be passed in. Incorrect values can result in
+    /// runtime panics, however, so care must still be taken with this method.
     ///
     /// [`Func::call_unchecked`]: crate::Func::call_unchecked
     /// [`Func::new_unchecked`]: crate::Func::new_unchecked
     /// [`Store`]: crate::Store
     /// [`TypedFunc`]: crate::TypedFunc
     /// [`ValRaw`]: crate::ValRaw
-    pub unsafe fn from_raw(mut store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
+    pub fn from_raw(mut store: impl AsContextMut, raw: u32) -> Option<Rooted<Self>> {
         let mut store = AutoAssertNoGc::new(store.as_context_mut().0);
         Self::_from_raw(&mut store, raw)
     }
@@ -402,19 +404,19 @@ impl ExnRef {
     ///
     /// Returns an error if this `exnref` has been unrooted.
     ///
-    /// # Unsafety
+    /// # Correctness
     ///
-    /// Produces a raw value which is only safe to pass into a store if a GC
+    /// Produces a raw value which is only valid to pass into a store if a GC
     /// doesn't happen between when the value is produce and when it's passed
     /// into the store.
     ///
     /// [`ValRaw`]: crate::ValRaw
-    pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> Result<u32> {
+    pub fn to_raw(&self, mut store: impl AsContextMut) -> Result<u32> {
         let mut store = AutoAssertNoGc::new(store.as_context_mut().0);
         self._to_raw(&mut store)
     }
 
-    pub(crate) unsafe fn _to_raw(&self, store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
+    pub(crate) fn _to_raw(&self, store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
         let gc_ref = self.inner.try_clone_gc_ref(store)?;
         let raw = if gc_ref.is_i31() {
             gc_ref.as_raw_non_zero_u32()

crates/wasmtime/src/runtime/gc/enabled/externref.rs

@@ -548,27 +548,30 @@ impl ExternRef {
     /// This function assumes that `raw` is an externref value which is
     /// currently rooted within the [`Store`].
     ///
-    /// # Unsafety
+    /// # Correctness
     ///
-    /// This function is particularly `unsafe` because `raw` not only must be a
-    /// valid externref value produced prior by `to_raw` but it must also be
-    /// correctly rooted within the store. When arguments are provided to a
-    /// callback with [`Func::new_unchecked`], for example, or returned via
-    /// [`Func::call_unchecked`], if a GC is performed within the store then
-    /// floating externref values are not rooted and will be GC'd, meaning that
-    /// this function will no longer be safe to call with the values cleaned up.
-    /// This function must be invoked *before* possible GC operations can happen
-    /// (such as calling wasm).
+    /// This function is tricky to get right because `raw` not only must be a
+    /// valid `externref` value produced prior by [`ExternRef::to_raw`] but it
+    /// must also be correctly rooted within the store. When arguments are
+    /// provided to a callback with [`Func::new_unchecked`], for example, or
+    /// returned via [`Func::call_unchecked`], if a GC is performed within the
+    /// store then floating `externref` values are not rooted and will be GC'd,
+    /// meaning that this function will no longer be correct to call with the
+    /// values cleaned up. This function must be invoked *before* possible GC
+    /// operations can happen (such as calling Wasm).
     ///
-    /// When in doubt try to not use this. Instead use the safe Rust APIs of
-    /// [`TypedFunc`] and friends.
+    ///
+    /// When in doubt try to not use this. Instead use the Rust APIs of
+    /// [`TypedFunc`] and friends. Note though that this function is not
+    /// `unsafe` as any value can be passed in. Incorrect values can result in
+    /// runtime panics, however, so care must still be taken with this method.
     ///
     /// [`Func::call_unchecked`]: crate::Func::call_unchecked
     /// [`Func::new_unchecked`]: crate::Func::new_unchecked
     /// [`Store`]: crate::Store
     /// [`TypedFunc`]: crate::TypedFunc
     /// [`ValRaw`]: crate::ValRaw
-    pub unsafe fn from_raw(mut store: impl AsContextMut, raw: u32) -> Option<Rooted<ExternRef>> {
+    pub fn from_raw(mut store: impl AsContextMut, raw: u32) -> Option<Rooted<ExternRef>> {
         let mut store = AutoAssertNoGc::new(store.as_context_mut().0);
         Self::_from_raw(&mut store, raw)
     }
@@ -585,14 +588,14 @@ impl ExternRef {
     ///
     /// Returns an error if this `externref` has been unrooted.
     ///
-    /// # Unsafety
+    /// # Correctness
     ///
-    /// Produces a raw value which is only safe to pass into a store if a GC
+    /// Produces a raw value which is only valid to pass into a store if a GC
     /// doesn't happen between when the value is produce and when it's passed
     /// into the store.
     ///
     /// [`ValRaw`]: crate::ValRaw
-    pub unsafe fn to_raw(&self, mut store: impl AsContextMut) -> Result<u32> {
+    pub fn to_raw(&self, mut store: impl AsContextMut) -> Result<u32> {
         let mut store = AutoAssertNoGc::new(store.as_context_mut().0);
         self._to_raw(&mut store)
     }

crates/wasmtime/src/runtime/values.rs

@@ -230,11 +230,12 @@ impl Val {
     /// Returns an error if this value is a GC reference and the GC reference
     /// has been unrooted.
     ///
-    /// # Unsafety
+    /// # Safety
     ///
-    /// This method is unsafe for the reasons that [`ExternRef::to_raw`] and
-    /// [`Func::to_raw`] are unsafe.
-    pub unsafe fn to_raw(&self, store: impl AsContextMut) -> Result<ValRaw> {
+    /// The returned [`ValRaw`] does not carry type information and is only safe
+    /// to use within the context of this store itself. For more information see
+    /// [`ExternRef::to_raw`] and [`Func::to_raw`].
+    pub fn to_raw(&self, store: impl AsContextMut) -> Result<ValRaw> {
         match self {
             Val::I32(i) => Ok(ValRaw::i32(*i)),
             Val::I64(i) => Ok(ValRaw::i64(*i)),

crates/wasmtime/src/runtime/vm/const_expr.rs

@@ -88,7 +88,7 @@ impl ConstEvalContext {
 
         let allocator = StructRefPre::_new(store, struct_ty);
         let struct_ref = unsafe { StructRef::new_maybe_async(store, &allocator, &fields)? };
-        let raw = unsafe { struct_ref.to_anyref()._to_raw(store)? };
+        let raw = struct_ref.to_anyref()._to_raw(store)?;
         Ok(ValRaw::anyref(raw))
     }
 
@@ -293,7 +293,7 @@ impl ConstExprEvaluator {
                     let array = unsafe { ArrayRef::new_maybe_async(&mut store, &pre, &elem, len)? };
 
                     self.stack
-                        .push(unsafe { ValRaw::anyref(array.to_anyref()._to_raw(&mut store)?) });
+                        .push(ValRaw::anyref(array.to_anyref()._to_raw(&mut store)?));
                 }
 
                 #[cfg(feature = "gc")]
@@ -311,7 +311,7 @@ impl ConstExprEvaluator {
                     let array = unsafe { ArrayRef::new_maybe_async(&mut store, &pre, &elem, len)? };
 
                     self.stack
-                        .push(unsafe { ValRaw::anyref(array.to_anyref()._to_raw(&mut store)?) });
+                        .push(ValRaw::anyref(array.to_anyref()._to_raw(&mut store)?));
                 }
 
                 #[cfg(feature = "gc")]
@@ -347,7 +347,7 @@ impl ConstExprEvaluator {
                         unsafe { ArrayRef::new_fixed_maybe_async(&mut store, &pre, &elems)? };
 
                     self.stack
-                        .push(unsafe { ValRaw::anyref(array.to_anyref()._to_raw(&mut store)?) });
+                        .push(ValRaw::anyref(array.to_anyref()._to_raw(&mut store)?));
                 }
 
                 #[cfg(feature = "gc")]
@@ -363,13 +363,12 @@ impl ConstExprEvaluator {
 
                 #[cfg(feature = "gc")]
                 ConstOp::AnyConvertExtern => {
-                    let result = match ExternRef::_from_raw(&mut store, self.pop()?.get_externref())
-                    {
-                        Some(externref) => unsafe {
-                            AnyRef::_convert_extern(&mut store, externref)?._to_raw(&mut store)?
-                        },
-                        None => 0,
-                    };
+                    let result =
+                        match ExternRef::_from_raw(&mut store, self.pop()?.get_externref()) {
+                            Some(externref) => AnyRef::_convert_extern(&mut store, externref)?
+                                ._to_raw(&mut store)?,
+                            None => 0,
+                        };
                     self.stack.push(ValRaw::anyref(result));
                 }
             }

tests/all/gc.rs

@@ -818,8 +818,8 @@ fn to_raw_from_raw_doesnt_leak() -> Result<()> {
     {
         let mut scope = RootScope::new(&mut store);
         let x = ExternRef::new(&mut scope, SetFlagOnDrop(flag.clone()))?;
-        let raw = unsafe { x.to_raw(&mut scope)? };
-        let _x = unsafe { ExternRef::from_raw(&mut scope, raw) };
+        let raw = x.to_raw(&mut scope)?;
+        let _x = ExternRef::from_raw(&mut scope, raw);
     }
 
     store.gc(None);