From f9687df64275c197b1662f3f5d672a790e223cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C3=BAl=20Cabrera?= Date: Mon, 26 Oct 2026 10:46:31 -0400 Subject: [PATCH] Security release for 36.0.1 * Externref results may be stale references [GHSA-c9f2-f4vx-xq68](https://github.com/bytecodealliance/wasmtime-rb/security/advisories/GHSA-c9f2-f4vx-xq68) Co-authored-by: Jeff Charles --- bench/externref.rb | 30 +++ ext/src/ruby_api/convert.rs | 62 ++++--- ext/src/ruby_api/externref.rs | 93 ++++++++++ ext/src/ruby_api/func.rs | 55 ++++-- ext/src/ruby_api/global.rs | 39 ++-- ext/src/ruby_api/mod.rs | 1 + ext/src/ruby_api/params.rs | 18 +- ext/src/ruby_api/store.rs | 27 ++- ext/src/ruby_api/table.rs | 89 ++++----- spec/integration/externref_spec.rb | 282 +++++++++++++++++++++++++++++ 10 files changed, 566 insertions(+), 130 deletions(-) create mode 100644 bench/externref.rb create mode 100644 ext/src/ruby_api/externref.rs create mode 100644 spec/integration/externref_spec.rb diff --git a/bench/externref.rb b/bench/externref.rb new file mode 100644 index 00000000..c0dc5fa7 --- /dev/null +++ b/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)" diff --git a/ext/src/ruby_api/convert.rs b/ext/src/ruby_api/convert.rs index 8cc0016b..4aa15f7c 100644 --- a/ext/src/ruby_api/convert.rs +++ b/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; + fn to_ruby_value( + &self, + store: &StoreContextValue, + context: &impl AsContext, + ) -> Result; } impl ToRubyValue for Val { - fn to_ruby_value(&self, store: &StoreContextValue) -> Result { + fn to_ruby_value( + &self, + store: &StoreContextValue, + context: &impl AsContext, + ) -> Result { 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::() - .map(|v| v.0) - .ok_or_else(|| error!("failed to extract externref")), + Some(v) => { + let handle = v + .downcast_ref::() + .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; + fn to_wasm_val( + &self, + context: &mut impl AsContextMut, + ty: ValType, + ) -> Result; } impl ToWasmVal for Value { - fn to_wasm_val(&self, store: &StoreContextValue, ty: ValType) -> Result { + fn to_wasm_val( + &self, + context: &mut impl AsContextMut, + ty: ValType, + ) -> Result { 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 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; } diff --git a/ext/src/ruby_api/externref.rs b/ext/src/ruby_api/externref.rs new file mode 100644 index 00000000..2eeaf814 --- /dev/null +++ b/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>, +} + +#[derive(Default)] +pub struct ExternRefRegistry(Arc>); + +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 { + 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 { + 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>, +} + +// 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); + } + } +} diff --git a/ext/src/ruby_api/func.rs b/ext/src/ruby_api/func.rs index 1de09af3..daa9f018 100644 --- a/ext/src/ruby_api/func.rs +++ b/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 { - 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, ¶ms, &mut results) + func.call(&mut context, ¶ms, &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::>() - .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::>()?; + 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!( diff --git a/ext/src/ruby_api/global.rs b/ext/src/ruby_api/global.rs index 6dac81c2..6c40aab7 100644 --- a/ext/src/ruby_api/global.rs +++ b/ext/src/ruby_api/global.rs @@ -8,7 +8,7 @@ use magnus::{ class, function, gc::Marker, method, prelude::*, typed_data::Obj, DataTypeFunctions, Error, Object, Symbol, TypedData, Value, }; -use wasmtime::{Extern, Global as GlobalImpl, Mutability}; +use wasmtime::{Extern, Global as GlobalImpl, Mutability, RootScope}; #[derive(TypedData)] #[magnus( @@ -103,22 +103,19 @@ impl<'a> Global<'a> { mutability: Mutability, ) -> Result { let wasm_type = value_type.to_val_type()?; - let wasm_default = default.to_wasm_val(&store.into(), wasm_type.clone())?; + let mut context = RootScope::new(store.context_mut()); + let wasm_default = default.to_wasm_val(&mut context, wasm_type.clone())?; let inner = GlobalImpl::new( - store.context_mut(), + &mut context, wasmtime::GlobalType::new(wasm_type, mutability), wasm_default, ) .map_err(|e| error!("{}", e))?; - let global = Self { + Ok(Self { store: store.into(), inner, - }; - - global.retain_non_nil_extern_ref(default)?; - - Ok(global) + }) } pub fn from_inner(store: StoreContextValue<'a>, inner: GlobalImpl) -> Self { @@ -149,9 +146,10 @@ impl<'a> Global<'a> { /// @yard /// @return [Object] The current value of the global. pub fn get(&self) -> Result { + let mut context = RootScope::new(self.store.context_mut()?); self.inner - .get(self.store.context_mut()?) - .to_ruby_value(&self.store) + .get(&mut context) + .to_ruby_value(&self.store, &context) } /// @yard @@ -160,16 +158,12 @@ impl<'a> Global<'a> { /// @param value [Object] An object that can be converted to the global's type. /// @return [nil] pub fn set(&self, value: Value) -> Result<(), Error> { + let ty = self.value_type()?; + let mut context = RootScope::new(self.store.context_mut()?); + let value = value.to_wasm_val(&mut context, ty)?; self.inner - .set( - self.store.context_mut()?, - value.to_wasm_val(&self.store, self.value_type()?)?, - ) + .set(&mut context, value) .map_err(|e| error!("{}", e)) - .and_then(|result| { - self.retain_non_nil_extern_ref(value)?; - Ok(result) - }) } fn ty(&self) -> Result { @@ -180,13 +174,6 @@ impl<'a> Global<'a> { self.ty().map(|ty| ty.content().clone()) } - fn retain_non_nil_extern_ref(&self, value: Value) -> Result<(), Error> { - if !value.is_nil() && self.value_type()?.matches(&wasmtime::ValType::EXTERNREF) { - self.store.retain(value)?; - } - Ok(()) - } - pub fn inner(&self) -> GlobalImpl { self.inner } diff --git a/ext/src/ruby_api/mod.rs b/ext/src/ruby_api/mod.rs index 766551d9..6e37d31c 100644 --- a/ext/src/ruby_api/mod.rs +++ b/ext/src/ruby_api/mod.rs @@ -16,6 +16,7 @@ mod convert; mod engine; mod errors; mod externals; +mod externref; mod func; mod global; mod instance; diff --git a/ext/src/ruby_api/params.rs b/ext/src/ruby_api/params.rs index 44b6f7bc..bc79451d 100644 --- a/ext/src/ruby_api/params.rs +++ b/ext/src/ruby_api/params.rs @@ -1,7 +1,7 @@ -use super::{convert::ToWasmVal, errors::ExceptionMessage, store::StoreContextValue}; +use super::{convert::ToWasmVal, errors::ExceptionMessage, store::StoreData}; use magnus::{error::ErrorType, exception::arg_error, Error, Value}; use static_assertions::assert_eq_size; -use wasmtime::{FuncType, ValType}; +use wasmtime::{AsContextMut, FuncType, ValType}; #[derive(Debug, Clone)] #[repr(C)] @@ -16,9 +16,12 @@ impl Param { Self { index, ty, val } } - fn to_wasmtime_val(&self, store: &StoreContextValue) -> Result { + fn to_wasmtime_val( + &self, + context: &mut impl AsContextMut, + ) -> Result { self.val - .to_wasm_val(store, self.ty.clone()) + .to_wasm_val(context, self.ty.clone()) .map_err(|error| error.append(format!(" (param at index {})", self.index))) } } @@ -40,14 +43,17 @@ impl<'a> Params<'a> { Ok(Self(ty, params_slice)) } - pub fn to_vec(&self, store: &StoreContextValue) -> Result, Error> { + pub fn to_vec( + &self, + context: &mut impl AsContextMut, + ) -> Result, Error> { let mut vals = Vec::with_capacity(self.0.params().len()); for (i, (param, value)) in self.0.params().zip(self.1.iter()).enumerate() { let i: u32 = i .try_into() .map_err(|_| Error::new(arg_error(), "too many params"))?; let param = Param::new(i, param, *value); - vals.push(param.to_wasmtime_val(store)?); + vals.push(param.to_wasmtime_val(context)?); } Ok(vals) diff --git a/ext/src/ruby_api/store.rs b/ext/src/ruby_api/store.rs index 28ef9d46..2d1a09c4 100644 --- a/ext/src/ruby_api/store.rs +++ b/ext/src/ruby_api/store.rs @@ -1,4 +1,5 @@ use super::errors::wasi_exit_error; +use super::externref::ExternRefRegistry; use super::{caller::Caller, engine::Engine, root, trap::Trap}; use crate::{define_rb_intern, error, WasiConfig}; use magnus::value::StaticSymbol; @@ -34,6 +35,7 @@ pub struct StoreData { wasi_p1: Option, wasi: Option, refs: Vec, + pub externrefs: ExternRefRegistry, last_error: Option, store_limits: TrackingResourceLimiter, resource_table: ResourceTable, @@ -71,6 +73,7 @@ impl StoreData { } pub fn mark(&self, marker: &Marker) { + self.externrefs.mark(marker); marker.mark_movable(self.user_data); if let Some(ref error) = self.last_error { @@ -79,17 +82,15 @@ impl StoreData { } } - for value in self.refs.iter() { - marker.mark_movable(*value); - } + // Host closures can capture copies of these values (notably Procs). + // Updating refs alone cannot relocate those copies, so pin them. + // Externrefs use the registry above and remain movable. + marker.mark_slice(&self.refs); } pub fn compact(&mut self, compactor: &Compactor) { + self.externrefs.compact(compactor); self.user_data = compactor.location(self.user_data); - - for value in self.refs.iter_mut() { - *value = compactor.location(*value); - } } } @@ -187,6 +188,7 @@ impl Store { wasi_p1, wasi, refs: Default::default(), + externrefs: Default::default(), last_error: Default::default(), store_limits: limiter, resource_table: Default::default(), @@ -270,6 +272,16 @@ impl Store { unsafe { (*self.inner.get()).as_context_mut() } } + /// @yard + /// Collects unreachable WebAssembly GC objects, releasing their host values. + /// This is separate from Ruby's +GC.start+; Ruby can collect released values + /// on its next collection. Live globals, tables and Wasm references are kept. + /// @def gc + /// @return [nil] + pub fn gc(&self) { + self.context_mut().gc(None); + } + pub fn retain(&self, value: Value) { self.context_mut().data_mut().retain(value); } @@ -411,6 +423,7 @@ pub fn init() -> Result<(), Error> { class.define_singleton_method("new", function!(Store::new, -1))?; class.define_method("data", method!(Store::data, 0))?; + class.define_method("gc", method!(Store::gc, 0))?; class.define_method("get_fuel", method!(Store::get_fuel, 0))?; class.define_method("set_fuel", method!(Store::set_fuel, 1))?; class.define_method("set_epoch_deadline", method!(Store::set_epoch_deadline, 1))?; diff --git a/ext/src/ruby_api/table.rs b/ext/src/ruby_api/table.rs index 36459513..39674902 100644 --- a/ext/src/ruby_api/table.rs +++ b/ext/src/ruby_api/table.rs @@ -9,7 +9,7 @@ use magnus::{ class, function, gc::Marker, method, prelude::*, scan_args, typed_data::Obj, DataTypeFunctions, Error, IntoValue, Object, Symbol, TypedData, Value, }; -use wasmtime::{Extern, Table as TableImpl, Val}; +use wasmtime::{Extern, RootScope, Table as TableImpl, Val}; define_rb_intern!( MIN_SIZE => "min_size", @@ -93,7 +93,8 @@ impl<'a> Table<'a> { let (min,) = kw.required; let (max,) = kw.optional; let wasm_type = value_type.to_val_type()?; - let wasm_default = default.to_wasm_val(&store.into(), wasm_type.clone())?; + let mut context = RootScope::new(store.context_mut()); + let wasm_default = default.to_wasm_val(&mut context, wasm_type.clone())?; let ref_ = wasm_default .ref_() .ok_or_else(|| error!("Expected Ref for table value"))?; @@ -104,20 +105,16 @@ impl<'a> Table<'a> { .clone(); let inner = TableImpl::new( - store.context_mut(), + &mut context, wasmtime::TableType::new(table_type, min, max), ref_, ) .map_err(|e| error!("{}", e))?; - let table = Self { + Ok(Self { store: store.into(), inner, - }; - - table.retain_non_nil_extern_ref(default)?; - - Ok(table) + }) } pub fn from_inner(store: StoreContextValue<'a>, inner: TableImpl) -> Self { @@ -150,8 +147,9 @@ impl<'a> Table<'a> { /// @param index [Integer] /// @return [Object, nil] pub fn get(&self, index: u64) -> Result { - match self.inner.get(self.store.context_mut()?, index) { - Some(wasm_val) => Val::from(wasm_val).to_ruby_value(&self.store), + let mut context = RootScope::new(self.store.context_mut()?); + match self.inner.get(&mut context, index) { + Some(wasm_val) => Val::from(wasm_val).to_ruby_value(&self.store, &context), None => Ok(().into_value()), } } @@ -164,20 +162,20 @@ impl<'a> Table<'a> { /// @param value [Object] /// @return [void] pub fn set(&self, index: u64, value: Value) -> Result<(), Error> { + let ty = wasmtime::ValType::from(self.value_type()?); + let mut context = RootScope::new(self.store.context_mut()?); + let value = value + .to_wasm_val(&mut context, ty)? + .ref_() + .ok_or_else(|| error!("Expected Ref"))?; + if index >= self.inner.size(&context) { + return Err(error!("table element index out of bounds")); + } + // Wasmtime 36's Table::set does not drop an overwritten GC reference. + // fill uses the GC write barrier and releases the previous payload. self.inner - .set( - self.store.context_mut()?, - index, - value - .to_wasm_val(&self.store, wasmtime::ValType::from(self.value_type()?))? - .ref_() - .ok_or_else(|| error!("Expected Ref"))?, - ) + .fill(&mut context, index, value, 1) .map_err(|e| error!("{}", e)) - .and_then(|result| { - self.retain_non_nil_extern_ref(value)?; - Ok(result) - }) } /// @yard @@ -189,20 +187,30 @@ impl<'a> Table<'a> { /// @param initial [Object] The initial value for newly added table slots. /// @return [void] pub fn grow(&self, delta: u64, initial: Value) -> Result { - self.inner - .grow( - self.store.context_mut()?, - delta, - initial - .to_wasm_val(&self.store, wasmtime::ValType::from(self.value_type()?))? - .ref_() - .ok_or_else(|| error!("Expected Ref"))?, - ) - .map_err(|e| error!("{}", e)) - .and_then(|result| { - self.retain_non_nil_extern_ref(initial)?; - Ok(result) - }) + let ty = wasmtime::ValType::from(self.value_type()?); + let is_externref = ty.matches(&wasmtime::ValType::EXTERNREF); + let mut context = RootScope::new(self.store.context_mut()?); + let initial = initial + .to_wasm_val(&mut context, ty)? + .ref_() + .ok_or_else(|| error!("Expected Ref"))?; + if is_externref { + // Wasmtime 36's Table::grow leaks its cloned initial GC reference + // when growth fails. Grow with null, then fill the new slots using + // the still-rooted initial value only after growth succeeds. + let previous_size = self + .inner + .grow(&mut context, delta, wasmtime::Ref::Extern(None)) + .map_err(|e| error!("{}", e))?; + self.inner + .fill(&mut context, previous_size, initial, delta) + .map_err(|e| error!("{}", e))?; + Ok(previous_size) + } else { + self.inner + .grow(&mut context, delta, initial) + .map_err(|e| error!("{}", e)) + } } /// @yard @@ -222,13 +230,6 @@ impl<'a> Table<'a> { Ok(el.clone()) } - fn retain_non_nil_extern_ref(&self, value: Value) -> Result<(), Error> { - if !value.is_nil() && self.value_type()?.matches(&wasmtime::RefType::EXTERNREF) { - self.store.retain(value)?; - } - Ok(()) - } - pub fn inner(&self) -> TableImpl { self.inner } diff --git a/spec/integration/externref_spec.rb b/spec/integration/externref_spec.rb new file mode 100644 index 00000000..97964bca --- /dev/null +++ b/spec/integration/externref_spec.rb @@ -0,0 +1,282 @@ +require "spec_helper" +require "weakref" + +module Wasmtime + RSpec.describe "Externref lifetimes" do + let(:instance) do + compile(<<~WAT) + (module + (global $saved (mut externref) (ref.null extern)) + (table $saved_table 1 externref) + (func (export "save_global") (param externref) + (global.set $saved (local.get 0))) + (func (export "load_global") (result externref) + (global.get $saved)) + (func (export "save_table") (param externref) + (table.set $saved_table (i32.const 0) (local.get 0))) + (func (export "load_table") (result externref) + (table.get $saved_table (i32.const 0))) + (func (export "clear") + (global.set $saved (ref.null extern)) + (table.set $saved_table (i32.const 0) (ref.null extern))) + (func (export "identity") (param externref) (result externref) + (local.get 0)) + (func (export "pair") (param externref externref) (result externref externref) + (local.get 0) (local.get 1)) + (func (export "trap") (param externref) + unreachable) + (func (export "two_args") (param externref i32)) + (func (export "is_null") (param externref) (result i32) + (ref.is_null (local.get 0)))) + WAT + end + + ["global", "table"].each do |kind| + it "keeps a temporary value stored by Wasm in a #{kind} alive and relocates it" do + weak = save_temporary(instance.export("save_#{kind}").to_func) + store.gc + collect_ruby + expect(instance.invoke("load_#{kind}")).to equal(weak.__getobj__) + expect(weak.__getobj__).to eq("temporary value") + end + + it "releases a value removed from the guest #{kind}" do + weak = save_temporary(instance.export("save_#{kind}").to_func) + instance.invoke("clear") + collect(store) + expect(weak.weakref_alive?).to be_falsey + end + end + + it "marks and relocates values in Ruby-created globals and tables" do + global = Global.var(store, :externref, +"global value") + table = Table.new(store, :externref, +"table value", min_size: 1) + table.grow(1, +"grown value") + collect(store) + expect(global.get).to eq("global value") + expect(table.get(0)).to eq("table value") + expect(table.get(1)).to eq("grown value") + global.set(+"replacement global") + table.set(0, +"replacement table") + collect(store) + expect(global.get).to eq("replacement global") + expect(table.get(0)).to eq("replacement table") + end + + it "releases overwritten globals and table entries" do + global = Global.var(store, :externref, nil) + table = Table.new(store, :externref, nil, min_size: 1) + global_value = save_temporary(global.method(:set)) + table_value = save_temporary(->(value) { table.set(0, value) }) + global.set(nil) + table.set(0, nil) + collect(store) + expect(global_value.weakref_alive?).to be_falsey + expect(table_value.weakref_alive?).to be_falsey + end + + it "does not retain values from repeated calls or reads" do + identity = instance.export("identity").to_func + values = without_gc_stress { Array.new(100) { save_temporary(identity) } } + collect(store) + expect(values.any?(&:weakref_alive?)).to be false + end + + it "does not keep extra roots after reading globals and tables" do + global = Global.var(store, :externref, nil) + table = Table.new(store, :externref, nil, min_size: 1) + global_value = save_temporary(global.method(:set)) + table_value = save_temporary(->(value) { table.set(0, value) }) + 100.times do + global.get + table.get(0) + end + global.set(nil) + table.set(0, nil) + collect(store) + expect(global_value.weakref_alive?).to be_falsey + expect(table_value.weakref_alive?).to be_falsey + end + + it "preserves Ruby object identity and nil's non-null externref semantics" do + [BasicObject.new, Object.new, +"string", [], {}, 42, true, false, :symbol].each do |value| + expect(instance.invoke("identity", value)).to equal(value) + end + expect(instance.invoke("identity", nil)).to be_nil + expect(instance.invoke("is_null", nil)).to eq(0) + expect(instance.invoke("load_global")).to be_nil + end + + it "keeps all results rooted while allocating their Ruby result array" do + values = [+"first", +"second"] + result = with_gc_stress { instance.invoke("pair", *values) } + expect(result[0]).to equal(values[0]) + expect(result[1]).to equal(values[1]) + end + + it "keeps fresh host callback results retained by the guest alive" do + weak = nil + callback = Func.new(store, [], [:externref]) do + value = +"callback value" + weak = WeakRef.new(value) + value + end + mod = Module.new(engine, <<~WAT) + (module + (import "" "create" (func $create (result externref))) + (global $saved (mut externref) (ref.null extern)) + (func (export "save") (global.set $saved (call $create))) + (func (export "load") (result externref) (global.get $saved))) + WAT + guest = Instance.new(store, mod, [callback]) + guest.invoke("save") + collect(store) + expect(guest.invoke("load")).to equal(weak.__getobj__) + end + + it "supports nested calls and collection inside host callbacks" do + identity = instance.export("identity").to_func + nested = Func.new(store, [:externref], [:externref]) do |_caller, value| + store.gc + collect_ruby + identity.call(value) + end + value = +"nested value" + expect(nested.call(value)).to equal(value) + end + + it "releases arguments after traps and partial parameter conversion failures" do + trap = instance.export("trap").to_func + two_args = instance.export("two_args").to_func + trapped = save_temporary_failure(trap, Trap) + invalid = save_temporary_failure(->(value) { two_args.call(value, nil) }, TypeError) + collect(store) + expect(trapped.weakref_alive?).to be_falsey + expect(invalid.weakref_alive?).to be_falsey + end + + it "releases partially converted callback results on error" do + weak = nil + callback = Func.new(store, [], [:externref, :i32]) do + value = +"failed callback result" + weak = WeakRef.new(value) + [value, nil] + end + expect { callback.call }.to raise_error(Wasmtime::ResultError) + collect(store) + expect(weak.weakref_alive?).to be_falsey + end + + it "releases values from failed global writes, table writes and table growth" do + global = Global.const(store, :externref, nil) + table = Table.new(store, :externref, nil, min_size: 1, max_size: 1) + failed_global = save_temporary_failure(global.method(:set), Wasmtime::Error) + failed_set = save_temporary_failure(->(value) { table.set(2, value) }, Wasmtime::Error) + failed_grow = save_temporary_failure(->(value) { table.grow(1, value) }, Wasmtime::Error) + collect(store) + [failed_global, failed_set, failed_grow].each do |weak| + expect(weak.weakref_alive?).to be_falsey + end + end + + it "keeps registries isolated between stores" do + other_store = Store.new(engine) + first = Global.var(store, :externref, +"first store") + second = Global.var(other_store, :externref, +"second store") + first.set(nil) + collect(store) + collect(other_store) + expect(second.get).to eq("second store") + expect(first.get).to be_nil + end + + it "releases values when the store is collected" do + weak = temporary_store_value + collect_ruby + expect(weak.weakref_alive?).to be_falsey + end + + it "can collect a store and values that refer back to it" do + weak = temporary_store_cycle + collect_ruby + expect(weak.weakref_alive?).to be_falsey + end + + it "supports host callbacks while another Ruby thread collects and compacts" do + callback = Func.new(store, [:externref], [:externref]) do |_caller, value| + collect_ruby + value + end + mod = Module.new(engine, <<~WAT) + (module + (import "" "callback" (func $callback (param externref) (result externref))) + (func (export "run") (param externref) (result externref) + (call $callback (local.get 0)))) + WAT + run = Instance.new(store, mod, [callback]).export("run").to_func + save = instance.export("save_global").to_func + clear = instance.export("clear").to_func + collector = Thread.new { 5.times { collect_ruby } } + 10.times do + value = +"threaded value" + expect(run.call(value)).to equal(value) + save_temporary(save) + clear.call + end + collector.value + ensure + collector&.join + end + + private + + # If we don't do this, the test will have a reference to `value` which will keep it alive when + # GC is run which prevents us from seeing if the value would've been collected as expected + def save_temporary(callable) + value = +"temporary value" + weak = WeakRef.new(value) + callable.call(value) + weak + end + + # RSpec's raise_error matcher retains its block (and therefore the value), + # so use a plain rescue when testing collection of failed arguments. + def save_temporary_failure(callable, error_class) + value = +"failed value" + weak = WeakRef.new(value) + begin + callable.call(value) + rescue error_class + return weak + end + raise "expected #{error_class}" + end + + def temporary_store_value + local_store = Store.new(engine) + global = Global.var(local_store, :externref, nil) + save_temporary(global.method(:set)) + end + + def temporary_store_cycle + local_store = Store.new(engine) + Global.var(local_store, :externref, [local_store]) + WeakRef.new(local_store) + end + + def collect_ruby + without_gc_stress do + GC.start(full_mark: true, immediate_sweep: true) + if GC.respond_to?(:verify_compaction_references) + GC.verify_compaction_references(toward: :empty) + end + GC.start(full_mark: true, immediate_sweep: true) + end + end + + def collect(target_store) + target_store.gc + collect_ruby + end + end +end