Security release for 36.0.1

bench/externref.rb

@@ -0,0 +1,30 @@
+require_relative "bench"
+
+engine = Wasmtime::Engine.new
+store = Wasmtime::Store.new(engine)
+mod = Wasmtime::Module.new(engine, <<~WAT)
+  (module
+    (func (export "identity") (param externref) (result externref)
+      (local.get 0)))
+WAT
+identity = Wasmtime::Instance.new(store, mod).export("identity").to_func
+value_class = Class.new
+value = value_class.new
+
+Bench.ips do |x|
+  x.report("externref roundtrip (1000 calls + Wasmtime GC)") do
+    1000.times { identity.call(value) }
+    store.gc
+  end
+
+  x.report("externref churn (1000 objects + Wasmtime GC)") do
+    1000.times { identity.call(value_class.new) }
+    store.gc
+  end
+
+  x.compare!
+end
+
+store.gc
+GC.start(full_mark: true, immediate_sweep: true)
+puts "Ruby values still alive after collection: #{ObjectSpace.each_object(value_class).count} (expected 1)"

ext/src/ruby_api/convert.rs

@@ -3,13 +3,14 @@ use lazy_static::lazy_static;
 use magnus::{
     prelude::*, try_convert, Error, IntoValue, RArray, Ruby, Symbol, TryConvert, TypedData, Value,
 };
-use wasmtime::{ExternRef, RefType, Val, ValType};
+use wasmtime::{AsContext, AsContextMut, ExternRef, RefType, Val, ValType};
 
 use super::{
+    externref::ExternRefHandle,
     func::{Func, FuncType},
     global::{Global, GlobalType},
     memory::{Memory, MemoryType},
-    store::StoreContextValue,
+    store::{StoreContextValue, StoreData},
     table::{Table, TableType},
 };
 
@@ -40,11 +41,19 @@ lazy_static! {
 }
 
 pub trait ToRubyValue {
-    fn to_ruby_value(&self, store: &StoreContextValue) -> Result<Value, Error>;
+    fn to_ruby_value(
+        &self,
+        store: &StoreContextValue,
+        context: &impl AsContext<Data = StoreData>,
+    ) -> Result<Value, Error>;
 }
 
 impl ToRubyValue for Val {
-    fn to_ruby_value(&self, store: &StoreContextValue) -> Result<Value, Error> {
+    fn to_ruby_value(
+        &self,
+        store: &StoreContextValue,
+        context: &impl AsContext<Data = StoreData>,
+    ) -> Result<Value, Error> {
         match self {
             Val::I32(i) => Ok(i.into_value()),
             Val::I64(i) => Ok(i.into_value()),
@@ -53,12 +62,15 @@ impl ToRubyValue for Val {
             Val::ExternRef(eref) => match eref {
                 None => Ok(().into_value()),
                 Some(eref) => {
-                    let inner = eref.data(store.context()?).map_err(|e| error!("{e}"))?;
+                    let context = context.as_context();
+                    let inner = eref.data(&context).map_err(|e| error!("{e}"))?;
                     match inner {
-                        Some(v) => v
-                            .downcast_ref::<ExternRefValue>()
-                            .map(|v| v.0)
-                            .ok_or_else(|| error!("failed to extract externref")),
+                        Some(v) => {
+                            let handle = v
+                                .downcast_ref::<ExternRefHandle>()
+                                .ok_or_else(|| error!("failed to extract externref"))?;
+                            context.data().externrefs.get(&Ruby::get().unwrap(), handle)
+                        }
                         None => Ok(().into_value()),
                     }
                 }
@@ -73,22 +85,27 @@ impl ToRubyValue for Val {
     }
 }
 pub trait ToWasmVal {
-    fn to_wasm_val(&self, store: &StoreContextValue, ty: ValType) -> Result<Val, Error>;
+    fn to_wasm_val(
+        &self,
+        context: &mut impl AsContextMut<Data = StoreData>,
+        ty: ValType,
+    ) -> Result<Val, Error>;
 }
 
 impl ToWasmVal for Value {
-    fn to_wasm_val(&self, store: &StoreContextValue, ty: ValType) -> Result<Val, Error> {
+    fn to_wasm_val(
+        &self,
+        context: &mut impl AsContextMut<Data = StoreData>,
+        ty: ValType,
+    ) -> Result<Val, Error> {
         if ty.matches(&ValType::EXTERNREF) {
             // Don't special case `nil` in order to ensure that it's always
             // a rooted value. Even though it's `nil` from Ruby's perspective,
             // it's a host managed object.
-            let extern_ref_value = Some(
-                ExternRef::new(
-                    store.context_mut().map_err(|e| error!("{e}"))?,
-                    ExternRefValue::from(*self),
-                )
-                .map_err(|e| error!("{e}"))?,
-            );
+            let handle = context.as_context().data().externrefs.allocate(*self)?;
+            // If allocation fails, dropping its error/payload releases the entry.
+            let extern_ref_value =
+                Some(ExternRef::new(context, handle).map_err(|e| error!("{e}"))?);
 
             return Ok(Val::ExternRef(extern_ref_value));
         }
@@ -114,15 +131,6 @@ impl ToWasmVal for Value {
     }
 }
 
-struct ExternRefValue(Value);
-impl From<Value> for ExternRefValue {
-    fn from(v: Value) -> Self {
-        Self(v)
-    }
-}
-unsafe impl Send for ExternRefValue {}
-unsafe impl Sync for ExternRefValue {}
-
 pub trait ToExtern {
     fn to_extern(&self, ruby: &Ruby) -> Result<wasmtime::Extern, Error>;
 }

ext/src/ruby_api/externref.rs

@@ -0,0 +1,93 @@
+//! Ruby values held by Wasmtime externrefs.
+
+// The store uses a map of integer handles to Ruby values. The integer handles are what is sent
+// and returned from Wasmtime. Handles are monotically increased and never reused. This allows
+// the Ruby values to be moved by the garbage collector.
+use crate::error;
+use magnus::{
+    gc::{Compactor, Marker},
+    value::Opaque,
+    Error, Ruby, Value,
+};
+use std::{
+    collections::HashMap,
+    sync::{Arc, Mutex, MutexGuard, Weak},
+};
+
+#[derive(Default)]
+struct Entries {
+    next_handle: u64,
+    values: HashMap<u64, Opaque<Value>>,
+}
+
+#[derive(Default)]
+pub struct ExternRefRegistry(Arc<Mutex<Entries>>);
+
+impl ExternRefRegistry {
+    fn entries(&self) -> MutexGuard<'_, Entries> {
+        // Cleanup must not panic, including if another operation panicked.
+        self.0.lock().unwrap_or_else(|error| error.into_inner())
+    }
+
+    pub fn allocate(&self, value: Value) -> Result<ExternRefHandle, Error> {
+        let handle = {
+            let mut entries = self.entries();
+            entries.next_handle.checked_add(1).inspect(|&handle| {
+                entries.next_handle = handle;
+                entries.values.insert(handle, value.into());
+            })
+        };
+        // Construct Ruby errors only after releasing the lock: allocation can
+        // invoke the store's mark/compact hooks, which take the same lock.
+        let handle = handle.ok_or_else(|| error!("externref handles exhausted"))?;
+        Ok(ExternRefHandle {
+            handle,
+            registry: Arc::downgrade(&self.0),
+        })
+    }
+
+    pub fn get(&self, ruby: &Ruby, handle: &ExternRefHandle) -> Result<Value, Error> {
+        if !Weak::ptr_eq(&handle.registry, &Arc::downgrade(&self.0)) {
+            return Err(error!("externref belongs to a different registry"));
+        }
+        let value = self.entries().values.get(&handle.handle).copied();
+        value
+            .map(|value| ruby.get_inner(value))
+            .ok_or_else(|| error!("invalid externref handle"))
+    }
+
+    pub fn mark(&self, marker: &Marker) {
+        for value in self.entries().values.values() {
+            marker.mark_movable(*value);
+        }
+    }
+
+    pub fn compact(&self, compactor: &Compactor) {
+        for value in self.entries().values.values_mut() {
+            *value = compactor.location(*value);
+        }
+    }
+}
+
+/// One registry entry per Wasmtime payload, not per copy of a rooted reference.
+/// Dropping this payload can happen without the GVL, so it must never call Ruby.
+/// A weak reference allows the store and its registry to be destroyed in either
+/// order, without keeping Ruby values alive after the store is gone.
+pub struct ExternRefHandle {
+    handle: u64,
+    registry: Weak<Mutex<Entries>>,
+}
+
+// We need this so values are garbage collected when they're no longer referenced by a table,
+// global, or anything else that would hold a reference to it inside Wasmtime.
+impl Drop for ExternRefHandle {
+    fn drop(&mut self) {
+        if let Some(registry) = self.registry.upgrade() {
+            registry
+                .lock()
+                .unwrap_or_else(|error| error.into_inner())
+                .values
+                .remove(&self.handle);
+        }
+    }
+}

ext/src/ruby_api/func.rs

@@ -12,7 +12,7 @@ use magnus::{
     typed_data::Obj, value::Opaque, DataTypeFunctions, Error, IntoValue, Object, RArray, Ruby,
     TypedData, Value,
 };
-use wasmtime::{Caller as CallerImpl, Func as FuncImpl, Val};
+use wasmtime::{Caller as CallerImpl, Func as FuncImpl, RootScope, Val};
 
 /// @yard
 /// @rename Wasmtime::FuncType
@@ -211,22 +211,27 @@ impl<'a> Func<'a> {
         func: &wasmtime::Func,
         args: &[Value],
     ) -> Result<Value, Error> {
-        let mut context = store.context_mut()?;
+        // Keep parameters and results rooted through Ruby result conversion,
+        // including nested callbacks, traps and partially converted arguments.
+        let mut context = RootScope::new(store.context_mut()?);
         let func_ty = func.ty(&mut context);
-        let params = Params::new(&func_ty, args)?.to_vec(store)?;
+        let params = Params::new(&func_ty, args)?.to_vec(&mut context)?;
         let mut results = vec![Val::null_func_ref(); func_ty.results().len()];
 
-        func.call(context, &params, &mut results)
+        func.call(&mut context, &params, &mut results)
             .map_err(|e| store.handle_wasm_error(e))?;
 
         match results.as_slice() {
             [] => Ok(().into_value()),
-            [result] => result.to_ruby_value(store),
-            _ => results
-                .into_iter()
-                .map(|result| result.to_ruby_value(store))
-                .collect::<Result<RArray, _>>()
-                .map(|ary| ary.as_value()),
+            [result] => result.to_ruby_value(store, &context),
+            _ => {
+                let ary = RArray::with_capacity(results.len());
+                for result in results {
+                    let val = result.to_ruby_value(store, &context)?;
+                    ary.push(val)?;
+                }
+                Ok(ary.as_value())
+            }
         }
     }
 }
@@ -268,15 +273,19 @@ pub fn make_func_closure(
         let wrapped_caller = Obj::wrap(Caller::new(caller_impl));
         let store_context = StoreContextValue::from(wrapped_caller);
 
-        let params_iter = params.iter().enumerate().map(|(i, param)| {
-            param
-                .to_ruby_value(&store_context)
-                .map_err(|e| wasmtime::Error::msg(format!("invalid argument at index {i}: {e}")))
-        });
-
-        let rparams = std::iter::once(Ok(wrapped_caller.as_value()))
-            .chain(params_iter)
-            .collect::<Result<RArray, _>>()?;
+        let rparams = RArray::with_capacity(params.len() + 1);
+        rparams
+            .push(wrapped_caller.as_value())
+            .map_err(|e| wasmtime::Error::msg(format!("failed to push caller: {e}")))?;
+        for (i, param) in params.iter().enumerate() {
+            let val = store_context
+                .context()
+                .and_then(|context| param.to_ruby_value(&store_context, &context))
+                .map_err(|e| wasmtime::Error::msg(format!("invalid argument at index {i}: {e}")))?;
+            rparams.push(val).map_err(|e| {
+                wasmtime::Error::msg(format!("failed to push argument at index {i}: {e}"))
+            })?;
+        }
 
         let ruby = Ruby::get().unwrap();
         let callable = ruby.get_inner(callable);
@@ -315,7 +324,13 @@ pub fn make_func_closure(
                     .zip(ty.results())
                     .enumerate()
                 {
-                    match rb_val.to_wasm_val(&store_context, ty) {
+                    // Wasmtime's host-call scope remains live until it has
+                    // copied these results back to the guest. A local scope
+                    // here would unroot them too early.
+                    match store_context
+                        .context_mut()
+                        .and_then(|mut context| rb_val.to_wasm_val(&mut context, ty))
+                    {
                         Ok(val) => *wasm_val = val,
                         Err(e) => {
                             return result_error!(