From 71fa9d485429e3ee41806b9dcb1ca14802d2e5f5 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Wed, 28 Jun 2023 23:11:25 -0700 Subject: [PATCH 01/23] Sketch of support for ResourceLimiter API --- crates/wasmi/src/engine/executor.rs | 24 +++- crates/wasmi/src/engine/mod.rs | 7 ++ crates/wasmi/src/lib.rs | 2 + crates/wasmi/src/limits.rs | 39 +++++++ crates/wasmi/src/linker.rs | 2 + crates/wasmi/src/memory/error.rs | 5 + crates/wasmi/src/memory/mod.rs | 114 ++++++++++++++----- crates/wasmi/src/module/instantiate/error.rs | 2 + crates/wasmi/src/module/instantiate/mod.rs | 2 + crates/wasmi/src/store.rs | 91 ++++++++++++++- crates/wasmi/src/table/error.rs | 4 + crates/wasmi/src/table/mod.rs | 90 +++++++++++---- 12 files changed, 331 insertions(+), 51 deletions(-) create mode 100644 crates/wasmi/src/limits.rs diff --git a/crates/wasmi/src/engine/executor.rs b/crates/wasmi/src/engine/executor.rs index 64df6d1e5d..9cf0c05d4a 100644 --- a/crates/wasmi/src/engine/executor.rs +++ b/crates/wasmi/src/engine/executor.rs @@ -24,6 +24,7 @@ use crate::{ ValueStack, }, func::FuncEntity, + store::ResourceLimiterRef, table::TableEntity, FuelConsumptionMode, Func, @@ -92,15 +93,25 @@ pub enum ReturnOutcome { /// /// If the Wasm execution traps. #[inline(never)] -pub fn execute_wasm<'engine>( - ctx: &mut StoreInner, +pub fn execute_wasm<'ctx, 'engine>( + ctx: &'ctx mut StoreInner, cache: &'engine mut InstanceCache, value_stack: &'engine mut ValueStack, call_stack: &'engine mut CallStack, code_map: &'engine CodeMap, const_pool: ConstPoolView<'engine>, + resource_limiter: ResourceLimiterRef<'ctx>, ) -> Result { - Executor::new(ctx, cache, value_stack, call_stack, code_map, const_pool).execute() + Executor::new( + ctx, + cache, + value_stack, + call_stack, + code_map, + const_pool, + resource_limiter, + ) + .execute() } /// The function signature of Wasm load operations. @@ -168,6 +179,7 @@ struct Executor<'ctx, 'engine> { code_map: &'engine CodeMap, /// A read-only view to a pool of constant values. const_pool: ConstPoolView<'engine>, + resource_limiter: ResourceLimiterRef<'ctx>, } macro_rules! forward_call { @@ -195,6 +207,7 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { call_stack: &'engine mut CallStack, code_map: &'engine CodeMap, const_pool: ConstPoolView<'engine>, + resource_limiter: ResourceLimiterRef<'ctx>, ) -> Self { let frame = call_stack.pop().expect("must have frame on the call stack"); let sp = value_stack.stack_ptr(); @@ -208,6 +221,7 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { call_stack, code_map, const_pool, + resource_limiter, } } @@ -1097,7 +1111,7 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { let new_pages = this .ctx .resolve_memory_mut(memory) - .grow(delta) + .grow(delta, &mut this.resource_limiter) .map(u32::from) .map_err(|_| EntityGrowError::InvalidGrow)?; // The `memory.grow` operation might have invalidated the cached @@ -1219,7 +1233,7 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { let table = this.cache.get_table(this.ctx, table_index); this.ctx .resolve_table_mut(&table) - .grow_untyped(delta, init) + .grow_untyped(delta, init, &mut this.resource_limiter) .map_err(|_| EntityGrowError::InvalidGrow) }, ); diff --git a/crates/wasmi/src/engine/mod.rs b/crates/wasmi/src/engine/mod.rs index 49912efef8..1194df0901 100644 --- a/crates/wasmi/src/engine/mod.rs +++ b/crates/wasmi/src/engine/mod.rs @@ -48,6 +48,7 @@ pub(crate) use self::{ use crate::{ core::{Trap, TrapCode}, func::FuncEntity, + store::ResourceLimiterRef, AsContext, AsContextMut, Func, @@ -798,6 +799,11 @@ impl<'engine> EngineExecutor<'engine> { let call_stack = &mut self.stack.frames; let code_map = &self.res.code_map; let const_pool = self.res.const_pool.view(); + let resource_limiter = ResourceLimiterRef(match &mut ctx.store.limiter { + Some(q) => Some(q.0(&mut ctx.store.data)), + None => None, + }); + execute_wasm( store_inner, cache, @@ -805,6 +811,7 @@ impl<'engine> EngineExecutor<'engine> { call_stack, code_map, const_pool, + resource_limiter, ) .map_err(make_trap) } diff --git a/crates/wasmi/src/lib.rs b/crates/wasmi/src/lib.rs index 0e17b8d653..5e10ff2d08 100644 --- a/crates/wasmi/src/lib.rs +++ b/crates/wasmi/src/lib.rs @@ -93,6 +93,7 @@ mod externref; mod func; mod global; mod instance; +mod limits; mod linker; mod memory; mod module; @@ -145,6 +146,7 @@ pub use self::{ }, global::{Global, GlobalType, Mutability}, instance::{Export, ExportsIter, Extern, ExternType, Instance}, + limits::ResourceLimiter, linker::Linker, memory::{Memory, MemoryType}, module::{ diff --git a/crates/wasmi/src/limits.rs b/crates/wasmi/src/limits.rs new file mode 100644 index 0000000000..1012ff808f --- /dev/null +++ b/crates/wasmi/src/limits.rs @@ -0,0 +1,39 @@ +use crate::{memory::MemoryError, table::TableError}; + +/// Value returned by [`ResourceLimiter::instances`] default method +pub const DEFAULT_INSTANCE_LIMIT: usize = 10000; + +/// Value returned by [`ResourceLimiter::tables`] default method +pub const DEFAULT_TABLE_LIMIT: usize = 10000; + +/// Value returned by [`ResourceLimiter::memories`] default method +pub const DEFAULT_MEMORY_LIMIT: usize = 10000; + +pub trait ResourceLimiter { + fn memory_growing( + &mut self, + current: usize, + desired: usize, + maximum: Option, + ) -> Result; + + fn table_growing( + &mut self, + current: u32, + desired: u32, + maximum: Option, + ) -> Result; + + // Provided methods + fn memory_grow_failed(&mut self, _error: &MemoryError) {} + fn table_grow_failed(&mut self, _error: &TableError) {} + fn instances(&self) -> usize { + DEFAULT_INSTANCE_LIMIT + } + fn tables(&self) -> usize { + DEFAULT_TABLE_LIMIT + } + fn memories(&self) -> usize { + DEFAULT_MEMORY_LIMIT + } +} diff --git a/crates/wasmi/src/linker.rs b/crates/wasmi/src/linker.rs index 7c2b83ff70..94892b5d7d 100644 --- a/crates/wasmi/src/linker.rs +++ b/crates/wasmi/src/linker.rs @@ -649,6 +649,8 @@ impl Linker { module: &Module, ) -> Result { assert!(Engine::same(self.engine(), context.as_context().engine())); + // TODO: possibly add further resource limtation here on number of externals. + // Not clear that user can't import the same external lots of times to inflate this. let externals = module .imports() .map(|import| self.process_import(&mut context, import)) diff --git a/crates/wasmi/src/memory/error.rs b/crates/wasmi/src/memory/error.rs index 9ed4379a88..13a52fd92a 100644 --- a/crates/wasmi/src/memory/error.rs +++ b/crates/wasmi/src/memory/error.rs @@ -20,6 +20,8 @@ pub enum MemoryError { /// The [`MemoryType`] which is supposed to be a supertype of `ty`. other: MemoryType, }, + /// Tried to create too many memories + TooManyMemories, } impl Display for MemoryError { @@ -40,6 +42,9 @@ impl Display for MemoryError { Self::InvalidSubtype { ty, other } => { write!(f, "memory type {ty:?} is not a subtype of {other:?}",) } + Self::TooManyMemories => { + write!(f, "too many memories") + } } } } diff --git a/crates/wasmi/src/memory/mod.rs b/crates/wasmi/src/memory/mod.rs index 0a3a91cd4a..149e05e07d 100644 --- a/crates/wasmi/src/memory/mod.rs +++ b/crates/wasmi/src/memory/mod.rs @@ -5,6 +5,8 @@ mod error; #[cfg(test)] mod tests; +use crate::store::ResourceLimiterRef; + use self::buffer::ByteBuffer; pub use self::{ data::{DataSegment, DataSegmentEntity, DataSegmentIdx}, @@ -127,17 +129,35 @@ pub struct MemoryEntity { impl MemoryEntity { /// Creates a new memory entity with the given memory type. - pub fn new(memory_type: MemoryType) -> Result { + pub fn new( + memory_type: MemoryType, + limiter: &mut ResourceLimiterRef<'_>, + ) -> Result { let initial_pages = memory_type.initial_pages(); - let initial_len = initial_pages - .to_bytes() - .ok_or(MemoryError::OutOfBoundsAllocation)?; - let memory = Self { - bytes: ByteBuffer::new(initial_len), - memory_type, - current_pages: initial_pages, - }; - Ok(memory) + let initial_len = initial_pages.to_bytes(); + let maximum_pages = memory_type.maximum_pages().unwrap_or_else(Pages::max); + let maximum_len = maximum_pages.to_bytes(); + + if let Some(limiter) = &mut limiter.0 { + if !limiter.memory_growing(0, initial_len.unwrap_or(usize::MAX), maximum_len)? { + return Err(MemoryError::OutOfBoundsAllocation); + } + } + + if let Some(initial_len) = initial_len { + let memory = Self { + bytes: ByteBuffer::new(initial_len), + memory_type, + current_pages: initial_pages, + }; + Ok(memory) + } else { + let err = MemoryError::OutOfBoundsAllocation; + if let Some(limiter) = &mut limiter.0 { + limiter.memory_grow_failed(&err) + } + Err(err) + } } /// Returns the memory type of the linear memory. @@ -171,25 +191,61 @@ impl MemoryEntity { /// /// If the linear memory would grow beyond its maximum limit after /// the grow operation. - pub fn grow(&mut self, additional: Pages) -> Result { + pub fn grow( + &mut self, + additional: Pages, + limiter: &mut ResourceLimiterRef<'_>, + ) -> Result { let current_pages = self.current_pages(); if additional == Pages::from(0) { // Nothing to do in this case. Bail out early. return Ok(current_pages); } + let maximum_pages = self.ty().maximum_pages().unwrap_or_else(Pages::max); - let new_pages = current_pages - .checked_add(additional) - .filter(|&new_pages| new_pages <= maximum_pages) - .ok_or(MemoryError::OutOfBoundsGrowth)?; - let new_size = new_pages - .to_bytes() - .ok_or(MemoryError::OutOfBoundsAllocation)?; - // At this point it is okay to grow the underlying virtual memory - // by the given amount of additional pages. - self.bytes.grow(new_size); - self.current_pages = new_pages; - Ok(current_pages) + let desired_pages = current_pages.checked_add(additional); + + // ResourceLimiter gets first look at the request. + if let Some(limiter) = &mut limiter.0 { + let current_size = current_pages.to_bytes().unwrap_or(usize::MAX); + let desired_size = desired_pages + .unwrap_or_else(Pages::max) + .to_bytes() + .unwrap_or(usize::MAX); + let maximum_size = maximum_pages.to_bytes(); + if !limiter.memory_growing(current_size, desired_size, maximum_size)? { + return Err(MemoryError::OutOfBoundsGrowth); + } + } + + let ret: Result; + + if let Some(new_pages) = desired_pages { + if new_pages <= maximum_pages { + if let Some(new_size) = new_pages.to_bytes() { + // At this point it is okay to grow the underlying virtual memory + // by the given amount of additional pages. + self.bytes.grow(new_size); + self.current_pages = new_pages; + ret = Ok(current_pages) + } else { + ret = Err(MemoryError::OutOfBoundsAllocation); + } + } else { + ret = Err(MemoryError::OutOfBoundsGrowth); + } + } else { + ret = Err(MemoryError::OutOfBoundsGrowth); + } + + // If there was an error, ResourceLimiter gets to see. + if let Err(e) = &ret { + if let Some(limiter) = &mut limiter.0 { + limiter.memory_grow_failed(e) + } + } + + ret } /// Returns a shared slice to the bytes underlying to the byte buffer. @@ -257,8 +313,13 @@ impl Memory { /// /// If more than [`u32::MAX`] much linear memory is allocated. pub fn new(mut ctx: impl AsContextMut, ty: MemoryType) -> Result { - let entity = MemoryEntity::new(ty)?; - let memory = ctx.as_context_mut().store.inner.alloc_memory(entity); + let store = ctx.as_context_mut().store; + let mut resource_limiter = ResourceLimiterRef(match &mut store.limiter { + Some(q) => Some(q.0(&mut store.data)), + None => None, + }); + let entity = MemoryEntity::new(ty, &mut resource_limiter)?; + let memory = store.inner.alloc_memory(entity); Ok(memory) } @@ -318,12 +379,13 @@ impl Memory { &self, mut ctx: impl AsContextMut, additional: Pages, + limiter: &mut ResourceLimiterRef<'_>, ) -> Result { ctx.as_context_mut() .store .inner .resolve_memory_mut(self) - .grow(additional) + .grow(additional, limiter) } /// Returns a shared slice to the bytes underlying the [`Memory`]. diff --git a/crates/wasmi/src/module/instantiate/error.rs b/crates/wasmi/src/module/instantiate/error.rs index 5a7f219980..ca8776ea22 100644 --- a/crates/wasmi/src/module/instantiate/error.rs +++ b/crates/wasmi/src/module/instantiate/error.rs @@ -49,6 +49,7 @@ pub enum InstantiationError { /// The index of the found `start` function. index: u32, }, + TooManyInstances, } #[cfg(feature = "std")] @@ -85,6 +86,7 @@ impl Display for InstantiationError { Self::Table(error) => Display::fmt(error, f), Self::Memory(error) => Display::fmt(error, f), Self::Global(error) => Display::fmt(error, f), + Self::TooManyInstances => write!(f, "too many instances") } } } diff --git a/crates/wasmi/src/module/instantiate/mod.rs b/crates/wasmi/src/module/instantiate/mod.rs index d788b34a75..803523caee 100644 --- a/crates/wasmi/src/module/instantiate/mod.rs +++ b/crates/wasmi/src/module/instantiate/mod.rs @@ -57,6 +57,8 @@ impl Module { let handle = context.as_context_mut().store.inner.alloc_instance(); let mut builder = InstanceEntity::build(self); + context.as_context_mut().store.bump_resource_counts(self)?; + self.extract_imports(&mut context, &mut builder, externals)?; self.extract_functions(&mut context, &mut builder, handle); self.extract_tables(&mut context, &mut builder)?; diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 6740f1c0fb..9d28ca7491 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -9,6 +9,7 @@ use crate::{ ElementSegmentEntity, ElementSegmentIdx, Engine, + Error, Func, FuncEntity, FuncIdx, @@ -67,6 +68,22 @@ impl StoreIdx { /// A stored entity. pub type Stored = GuardedEntity; +pub struct ResourceLimiterRef<'a>(pub(crate) Option<&'a mut (dyn crate::ResourceLimiter)>); +impl<'a> core::fmt::Debug for ResourceLimiterRef<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "ResourceLimiterRef(...)") + } +} + +pub(crate) struct ResourceLimiterQuery( + pub(crate) Box &mut (dyn crate::ResourceLimiter) + Send + Sync>, +); +impl core::fmt::Debug for ResourceLimiterQuery { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "ResourceLimiterQuery(...)") + } +} + /// The store that owns all data associated to Wasm modules. #[derive(Debug)] pub struct Store { @@ -80,7 +97,8 @@ pub struct Store { /// Stored host function trampolines. trampolines: Arena>, /// User provided host data owned by the [`Store`]. - data: T, + pub(crate) data: T, + pub(crate) limiter: Option>, } /// The inner store that owns all data not associated to the host state. @@ -114,6 +132,14 @@ pub struct StoreInner { engine: Engine, /// The fuel of the [`Store`]. fuel: Fuel, + + // Numbers of resources instantiated in this store, and their limits + instance_count: usize, + instance_limit: usize, + memory_count: usize, + memory_limit: usize, + table_count: usize, + table_limit: usize, } #[test] @@ -236,6 +262,17 @@ impl StoreInner { elems: Arena::new(), extern_objects: Arena::new(), fuel: Fuel::default(), + + // Counts and limits of instances, memories and tables tracked by + // ResourceLimiter. Counts are kept separate from the sizes of arenas + // above in order to allow checking "up front" in a single early and + // fallible call to [Store::bump_resource_counts]. + instance_count: 0, + instance_limit: crate::limits::DEFAULT_INSTANCE_LIMIT, + memory_count: 0, + memory_limit: crate::limits::DEFAULT_MEMORY_LIMIT, + table_count: 0, + table_limit: crate::limits::DEFAULT_TABLE_LIMIT, } } @@ -691,6 +728,7 @@ impl Store { inner: StoreInner::new(engine), trampolines: Arena::new(), data, + limiter: None, } } @@ -714,6 +752,57 @@ impl Store { self.data } + pub fn limiter( + &mut self, + mut limiter: impl FnMut(&mut T) -> &mut (dyn crate::ResourceLimiter) + Send + Sync + 'static, + ) { + let inner = &mut self.inner; + let (instance_limit, table_limit, memory_limit) = { + let l = limiter(&mut self.data); + (l.instances(), l.tables(), l.memories()) + }; + inner.instance_limit = instance_limit; + inner.table_limit = table_limit; + inner.memory_limit = memory_limit; + self.limiter = Some(ResourceLimiterQuery(Box::new(limiter))) + } + + pub fn bump_resource_counts(&mut self, module: &crate::Module) -> Result<(), Error> { + fn bump( + slot: &mut usize, + max: usize, + amt: usize, + err: impl FnOnce() -> Error, + ) -> Result<(), Error> { + let new = slot.saturating_add(amt); + if new > max { + return Err(err()); + } + *slot = new; + Ok(()) + } + + let inner = &mut self.inner; + bump(&mut inner.instance_count, inner.instance_limit, 1, || { + crate::module::InstantiationError::TooManyInstances.into() + })?; + bump( + &mut inner.memory_count, + inner.memory_limit, + module.len_memories(), + || crate::memory::MemoryError::TooManyMemories.into(), + )?; + bump( + &mut inner.table_count, + inner.table_limit, + module.len_tables(), + || crate::table::TableError::TooManyTables.into(), + )?; + // TODO: maybe also limit globals, imports and exports? These are not in + // the wasmtime-based ResourceLimiter API. + Ok(()) + } + /// Returns `true` if fuel metering has been enabled. fn is_fuel_metering_enabled(&self) -> bool { self.engine().config().get_consume_fuel() diff --git a/crates/wasmi/src/table/error.rs b/crates/wasmi/src/table/error.rs index 4e9d449c5d..92f9e4dcd6 100644 --- a/crates/wasmi/src/table/error.rs +++ b/crates/wasmi/src/table/error.rs @@ -38,6 +38,7 @@ pub enum TableError { /// The [`TableType`] which is supposed to be a supertype of `ty`. other: TableType, }, + TooManyTables, } impl Display for TableError { @@ -70,6 +71,9 @@ impl Display for TableError { Self::InvalidSubtype { ty, other } => { write!(f, "table type {ty:?} is not a subtype of {other:?}",) } + Self::TooManyTables => { + write!(f, "too many tables") + } } } } diff --git a/crates/wasmi/src/table/mod.rs b/crates/wasmi/src/table/mod.rs index 435e04701e..6a4f536519 100644 --- a/crates/wasmi/src/table/mod.rs +++ b/crates/wasmi/src/table/mod.rs @@ -3,7 +3,7 @@ pub use self::{ error::TableError, }; use super::{AsContext, AsContextMut, Stored}; -use crate::{module::FuncIdx, value::WithType, Func, FuncRef, Value}; +use crate::{module::FuncIdx, store::ResourceLimiterRef, value::WithType, Func, FuncRef, Value}; use alloc::vec::Vec; use core::cmp::max; use wasmi_arena::ArenaIndex; @@ -145,8 +145,23 @@ impl TableEntity { /// # Errors /// /// If `init` does not match the [`TableType`] element type. - pub fn new(ty: TableType, init: Value) -> Result { + pub fn new( + ty: TableType, + init: Value, + limiter: &mut ResourceLimiterRef<'_>, + ) -> Result { ty.matches_element_type(init.ty())?; + + if let Some(limiter) = &mut limiter.0 { + if limiter.table_growing(0, ty.minimum(), ty.maximum())? { + return Err(TableError::GrowOutOfBounds { + maximum: ty.maximum().unwrap_or(u32::MAX), + current: 0, + delta: ty.minimum(), + }); + } + } + let elements = vec![init.into(); ty.minimum() as usize]; Ok(Self { ty, elements }) } @@ -183,9 +198,14 @@ impl TableEntity { /// /// - If the table is grown beyond its maximum limits. /// - If `value` does not match the [`Table`] element type. - pub fn grow(&mut self, delta: u32, init: Value) -> Result { + pub fn grow( + &mut self, + delta: u32, + init: Value, + limiter: &mut ResourceLimiterRef<'_>, + ) -> Result { self.ty().matches_element_type(init.ty())?; - self.grow_untyped(delta, init.into()) + self.grow_untyped(delta, init.into(), limiter) } /// Grows the table by the given amount of elements. @@ -201,19 +221,45 @@ impl TableEntity { /// # Errors /// /// If the table is grown beyond its maximum limits. - pub fn grow_untyped(&mut self, delta: u32, init: UntypedValue) -> Result { - let maximum = self.ty.maximum().unwrap_or(u32::MAX); + pub fn grow_untyped( + &mut self, + delta: u32, + init: UntypedValue, + limiter: &mut ResourceLimiterRef<'_>, + ) -> Result { + // ResourceLimiter gets first look at the request. let current = self.size(); - let new_len = current - .checked_add(delta) - .filter(|&new_len| new_len <= maximum) - .ok_or(TableError::GrowOutOfBounds { - maximum, - current, - delta, - })? as usize; - self.elements.resize(new_len, init); - Ok(current) + let desired = current.checked_add(delta); + let maximum = self.ty.maximum(); + if let Some(limiter) = &mut limiter.0 { + if !limiter.table_growing(current, desired.unwrap_or(u32::MAX), maximum)? { + return Err(TableError::GrowOutOfBounds { + maximum: maximum.unwrap_or(u32::MAX), + current, + delta, + }); + } + } + + let maximum = maximum.unwrap_or(u32::MAX); + if let Some(desired) = desired { + if desired <= maximum { + self.elements.resize(desired as usize, init); + return Ok(current); + } + } + + let err = TableError::GrowOutOfBounds { + maximum, + current, + delta, + }; + + // If there was an error, ResourceLimiter gets to see. + if let Some(limiter) = &mut limiter.0 { + limiter.table_grow_failed(&err); + } + Err(err) } /// Converts the internal [`UntypedValue`] into a [`Value`] for this [`Table`] element type. @@ -473,8 +519,13 @@ impl Table { /// /// If `init` does not match the [`TableType`] element type. pub fn new(mut ctx: impl AsContextMut, ty: TableType, init: Value) -> Result { - let entity = TableEntity::new(ty, init)?; - let table = ctx.as_context_mut().store.inner.alloc_table(entity); + let store = ctx.as_context_mut().store; + let mut resource_limiter = ResourceLimiterRef(match &mut store.limiter { + Some(q) => Some(q.0(&mut store.data)), + None => None, + }); + let entity = TableEntity::new(ty, init, &mut resource_limiter)?; + let table = store.inner.alloc_table(entity); Ok(table) } @@ -535,12 +586,13 @@ impl Table { mut ctx: impl AsContextMut, delta: u32, init: Value, + limiter: &mut ResourceLimiterRef<'_>, ) -> Result { ctx.as_context_mut() .store .inner .resolve_table_mut(self) - .grow(delta, init) + .grow(delta, init, limiter) } /// Returns the [`Table`] element value at `index`. From c4d66eeefaad4d790125ccf04c6e49e5af22bd6d Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Thu, 29 Jun 2023 20:45:43 -0700 Subject: [PATCH 02/23] Address review comments. --- crates/wasmi/src/engine/mod.rs | 7 +------ crates/wasmi/src/memory/mod.rs | 28 ++++++++++++++-------------- crates/wasmi/src/store.rs | 24 ++++++++++++++++++++---- crates/wasmi/src/table/mod.rs | 26 +++++++++++++------------- 4 files changed, 48 insertions(+), 37 deletions(-) diff --git a/crates/wasmi/src/engine/mod.rs b/crates/wasmi/src/engine/mod.rs index 1194df0901..8c76be1ce4 100644 --- a/crates/wasmi/src/engine/mod.rs +++ b/crates/wasmi/src/engine/mod.rs @@ -48,7 +48,6 @@ pub(crate) use self::{ use crate::{ core::{Trap, TrapCode}, func::FuncEntity, - store::ResourceLimiterRef, AsContext, AsContextMut, Func, @@ -794,15 +793,11 @@ impl<'engine> EngineExecutor<'engine> { code.into() } - let store_inner = &mut ctx.store.inner; + let (store_inner, resource_limiter) = ctx.store.store_inner_and_resource_limiter_ref(); let value_stack = &mut self.stack.values; let call_stack = &mut self.stack.frames; let code_map = &self.res.code_map; let const_pool = self.res.const_pool.view(); - let resource_limiter = ResourceLimiterRef(match &mut ctx.store.limiter { - Some(q) => Some(q.0(&mut ctx.store.data)), - None => None, - }); execute_wasm( store_inner, diff --git a/crates/wasmi/src/memory/mod.rs b/crates/wasmi/src/memory/mod.rs index 149e05e07d..fb6667d1af 100644 --- a/crates/wasmi/src/memory/mod.rs +++ b/crates/wasmi/src/memory/mod.rs @@ -138,7 +138,7 @@ impl MemoryEntity { let maximum_pages = memory_type.maximum_pages().unwrap_or_else(Pages::max); let maximum_len = maximum_pages.to_bytes(); - if let Some(limiter) = &mut limiter.0 { + if let Some(limiter) = limiter.as_resource_limiter() { if !limiter.memory_growing(0, initial_len.unwrap_or(usize::MAX), maximum_len)? { return Err(MemoryError::OutOfBoundsAllocation); } @@ -153,7 +153,7 @@ impl MemoryEntity { Ok(memory) } else { let err = MemoryError::OutOfBoundsAllocation; - if let Some(limiter) = &mut limiter.0 { + if let Some(limiter) = limiter.as_resource_limiter() { limiter.memory_grow_failed(&err) } Err(err) @@ -206,7 +206,7 @@ impl MemoryEntity { let desired_pages = current_pages.checked_add(additional); // ResourceLimiter gets first look at the request. - if let Some(limiter) = &mut limiter.0 { + if let Some(limiter) = limiter.as_resource_limiter() { let current_size = current_pages.to_bytes().unwrap_or(usize::MAX); let desired_size = desired_pages .unwrap_or_else(Pages::max) @@ -240,7 +240,7 @@ impl MemoryEntity { // If there was an error, ResourceLimiter gets to see. if let Err(e) = &ret { - if let Some(limiter) = &mut limiter.0 { + if let Some(limiter) = limiter.as_resource_limiter() { limiter.memory_grow_failed(e) } } @@ -313,13 +313,12 @@ impl Memory { /// /// If more than [`u32::MAX`] much linear memory is allocated. pub fn new(mut ctx: impl AsContextMut, ty: MemoryType) -> Result { - let store = ctx.as_context_mut().store; - let mut resource_limiter = ResourceLimiterRef(match &mut store.limiter { - Some(q) => Some(q.0(&mut store.data)), - None => None, - }); + let (inner, mut resource_limiter) = ctx + .as_context_mut() + .store + .store_inner_and_resource_limiter_ref(); let entity = MemoryEntity::new(ty, &mut resource_limiter)?; - let memory = store.inner.alloc_memory(entity); + let memory = inner.alloc_memory(entity); Ok(memory) } @@ -379,13 +378,14 @@ impl Memory { &self, mut ctx: impl AsContextMut, additional: Pages, - limiter: &mut ResourceLimiterRef<'_>, ) -> Result { - ctx.as_context_mut() + let (inner, mut limiter) = ctx + .as_context_mut() .store - .inner + .store_inner_and_resource_limiter_ref(); + inner .resolve_memory_mut(self) - .grow(additional, limiter) + .grow(additional, &mut limiter) } /// Returns a shared slice to the bytes underlying the [`Memory`]. diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 9d28ca7491..b5c623f571 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -75,8 +75,14 @@ impl<'a> core::fmt::Debug for ResourceLimiterRef<'a> { } } -pub(crate) struct ResourceLimiterQuery( - pub(crate) Box &mut (dyn crate::ResourceLimiter) + Send + Sync>, +impl<'a> ResourceLimiterRef<'a> { + pub fn as_resource_limiter(&mut self) -> &mut Option<&'a mut dyn crate::ResourceLimiter> { + &mut self.0 + } +} + +struct ResourceLimiterQuery( + Box &mut (dyn crate::ResourceLimiter) + Send + Sync>, ); impl core::fmt::Debug for ResourceLimiterQuery { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -97,8 +103,8 @@ pub struct Store { /// Stored host function trampolines. trampolines: Arena>, /// User provided host data owned by the [`Store`]. - pub(crate) data: T, - pub(crate) limiter: Option>, + data: T, + limiter: Option>, } /// The inner store that owns all data not associated to the host state. @@ -803,6 +809,16 @@ impl Store { Ok(()) } + pub(crate) fn store_inner_and_resource_limiter_ref( + &mut self, + ) -> (&mut StoreInner, ResourceLimiterRef) { + let resource_limiter = ResourceLimiterRef(match &mut self.limiter { + Some(q) => Some(q.0(&mut self.data)), + None => None, + }); + (&mut self.inner, resource_limiter) + } + /// Returns `true` if fuel metering has been enabled. fn is_fuel_metering_enabled(&self) -> bool { self.engine().config().get_consume_fuel() diff --git a/crates/wasmi/src/table/mod.rs b/crates/wasmi/src/table/mod.rs index 6a4f536519..379f7c6b2e 100644 --- a/crates/wasmi/src/table/mod.rs +++ b/crates/wasmi/src/table/mod.rs @@ -152,7 +152,7 @@ impl TableEntity { ) -> Result { ty.matches_element_type(init.ty())?; - if let Some(limiter) = &mut limiter.0 { + if let Some(limiter) = limiter.as_resource_limiter() { if limiter.table_growing(0, ty.minimum(), ty.maximum())? { return Err(TableError::GrowOutOfBounds { maximum: ty.maximum().unwrap_or(u32::MAX), @@ -231,7 +231,7 @@ impl TableEntity { let current = self.size(); let desired = current.checked_add(delta); let maximum = self.ty.maximum(); - if let Some(limiter) = &mut limiter.0 { + if let Some(limiter) = limiter.as_resource_limiter() { if !limiter.table_growing(current, desired.unwrap_or(u32::MAX), maximum)? { return Err(TableError::GrowOutOfBounds { maximum: maximum.unwrap_or(u32::MAX), @@ -256,7 +256,7 @@ impl TableEntity { }; // If there was an error, ResourceLimiter gets to see. - if let Some(limiter) = &mut limiter.0 { + if let Some(limiter) = limiter.as_resource_limiter() { limiter.table_grow_failed(&err); } Err(err) @@ -519,13 +519,12 @@ impl Table { /// /// If `init` does not match the [`TableType`] element type. pub fn new(mut ctx: impl AsContextMut, ty: TableType, init: Value) -> Result { - let store = ctx.as_context_mut().store; - let mut resource_limiter = ResourceLimiterRef(match &mut store.limiter { - Some(q) => Some(q.0(&mut store.data)), - None => None, - }); + let (inner, mut resource_limiter) = ctx + .as_context_mut() + .store + .store_inner_and_resource_limiter_ref(); let entity = TableEntity::new(ty, init, &mut resource_limiter)?; - let table = store.inner.alloc_table(entity); + let table = inner.alloc_table(entity); Ok(table) } @@ -586,13 +585,14 @@ impl Table { mut ctx: impl AsContextMut, delta: u32, init: Value, - limiter: &mut ResourceLimiterRef<'_>, ) -> Result { - ctx.as_context_mut() + let (inner, mut limiter) = ctx + .as_context_mut() .store - .inner + .store_inner_and_resource_limiter_ref(); + inner .resolve_table_mut(self) - .grow(delta, init, limiter) + .grow(delta, init, &mut limiter) } /// Returns the [`Table`] element value at `index`. From 8e0da2339bde6b1c6c034c828c416cd684db742a Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Thu, 29 Jun 2023 22:08:21 -0700 Subject: [PATCH 03/23] Handle the 3 separate ResourceLimiter response types more correctly. --- crates/core/src/trap.rs | 8 +++++ crates/wasmi/src/engine/executor.rs | 12 +++---- crates/wasmi/src/memory/mod.rs | 26 +++++++-------- crates/wasmi/src/table/error.rs | 21 ++----------- crates/wasmi/src/table/mod.rs | 49 +++++++++++++++-------------- 5 files changed, 56 insertions(+), 60 deletions(-) diff --git a/crates/core/src/trap.rs b/crates/core/src/trap.rs index 7c02cf0511..6e5f0656ce 100644 --- a/crates/core/src/trap.rs +++ b/crates/core/src/trap.rs @@ -284,6 +284,13 @@ pub enum TrapCode { /// internal bytecode so that fuel is consumed for each executed instruction. /// This is useful to deterministically halt or yield a WebAssembly execution. OutOfFuel, + + /// This trap is raised when a growth operation was attempted and an + /// installed `ResourceLimiter` returned `Err(...)` from the associated + /// `table_growing` or `memory_growing` method, indicating a desire on the + /// part of the embedder to trap the interpreter rather than merely fail the + /// growth operation. + GrowthOperationLimited, } impl TrapCode { @@ -305,6 +312,7 @@ impl TrapCode { Self::StackOverflow => "call stack exhausted", Self::BadSignature => "indirect call type mismatch", Self::OutOfFuel => "all fuel consumed by WebAssembly", + Self::GrowthOperationLimited => "growth operation limited", } } } diff --git a/crates/wasmi/src/engine/executor.rs b/crates/wasmi/src/engine/executor.rs index 9cf0c05d4a..0b7f00f9a3 100644 --- a/crates/wasmi/src/engine/executor.rs +++ b/crates/wasmi/src/engine/executor.rs @@ -1112,8 +1112,7 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { .ctx .resolve_memory_mut(memory) .grow(delta, &mut this.resource_limiter) - .map(u32::from) - .map_err(|_| EntityGrowError::InvalidGrow)?; + .map(u32::from)?; // The `memory.grow` operation might have invalidated the cached // linear memory so we need to reset it in order for the cache to // reload in case it is used again. @@ -1231,10 +1230,11 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { |costs| costs.fuel_for_elements(u64::from(delta)), |this| { let table = this.cache.get_table(this.ctx, table_index); - this.ctx - .resolve_table_mut(&table) - .grow_untyped(delta, init, &mut this.resource_limiter) - .map_err(|_| EntityGrowError::InvalidGrow) + this.ctx.resolve_table_mut(&table).grow_untyped( + delta, + init, + &mut this.resource_limiter, + ) }, ); let result = match result { diff --git a/crates/wasmi/src/memory/mod.rs b/crates/wasmi/src/memory/mod.rs index fb6667d1af..a1d8313fec 100644 --- a/crates/wasmi/src/memory/mod.rs +++ b/crates/wasmi/src/memory/mod.rs @@ -5,7 +5,7 @@ mod error; #[cfg(test)] mod tests; -use crate::store::ResourceLimiterRef; +use crate::{engine::executor::EntityGrowError, store::ResourceLimiterRef}; use self::buffer::ByteBuffer; pub use self::{ @@ -14,7 +14,7 @@ pub use self::{ }; use super::{AsContext, AsContextMut, StoreContext, StoreContextMut, Stored}; use wasmi_arena::ArenaIndex; -use wasmi_core::Pages; +use wasmi_core::{Pages, TrapCode}; /// A raw index to a linear memory entity. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] @@ -140,6 +140,9 @@ impl MemoryEntity { if let Some(limiter) = limiter.as_resource_limiter() { if !limiter.memory_growing(0, initial_len.unwrap_or(usize::MAX), maximum_len)? { + // Here there's no meaningful way to map Ok(false) to + // INVALID_GROWTH_ERRCODE, so we just translate it to an + // appropriate Err(...) return Err(MemoryError::OutOfBoundsAllocation); } } @@ -195,7 +198,7 @@ impl MemoryEntity { &mut self, additional: Pages, limiter: &mut ResourceLimiterRef<'_>, - ) -> Result { + ) -> Result { let current_pages = self.current_pages(); if additional == Pages::from(0) { // Nothing to do in this case. Bail out early. @@ -213,12 +216,14 @@ impl MemoryEntity { .to_bytes() .unwrap_or(usize::MAX); let maximum_size = maximum_pages.to_bytes(); - if !limiter.memory_growing(current_size, desired_size, maximum_size)? { - return Err(MemoryError::OutOfBoundsGrowth); + match limiter.memory_growing(current_size, desired_size, maximum_size) { + Ok(true) => (), + Ok(false) => return Err(EntityGrowError::InvalidGrow), + Err(_) => return Err(EntityGrowError::TrapCode(TrapCode::GrowthOperationLimited)), } } - let ret: Result; + let mut ret: Result = Err(EntityGrowError::InvalidGrow); if let Some(new_pages) = desired_pages { if new_pages <= maximum_pages { @@ -228,20 +233,14 @@ impl MemoryEntity { self.bytes.grow(new_size); self.current_pages = new_pages; ret = Ok(current_pages) - } else { - ret = Err(MemoryError::OutOfBoundsAllocation); } - } else { - ret = Err(MemoryError::OutOfBoundsGrowth); } - } else { - ret = Err(MemoryError::OutOfBoundsGrowth); } // If there was an error, ResourceLimiter gets to see. if let Err(e) = &ret { if let Some(limiter) = limiter.as_resource_limiter() { - limiter.memory_grow_failed(e) + limiter.memory_grow_failed(&MemoryError::OutOfBoundsGrowth) } } @@ -386,6 +385,7 @@ impl Memory { inner .resolve_memory_mut(self) .grow(additional, &mut limiter) + .map_err(|_| MemoryError::OutOfBoundsGrowth) } /// Returns a shared slice to the bytes underlying the [`Memory`]. diff --git a/crates/wasmi/src/table/error.rs b/crates/wasmi/src/table/error.rs index 92f9e4dcd6..af9e5c84e1 100644 --- a/crates/wasmi/src/table/error.rs +++ b/crates/wasmi/src/table/error.rs @@ -7,14 +7,7 @@ use core::{fmt, fmt::Display}; #[non_exhaustive] pub enum TableError { /// Occurs when growing a table out of its set bounds. - GrowOutOfBounds { - /// The maximum allowed table size. - maximum: u32, - /// The current table size before the growth operation. - current: u32, - /// The amount of requested invalid growth. - delta: u32, - }, + GrowOutOfBounds, /// Occurs when operating with a [`Table`](crate::Table) and mismatching element types. ElementTypeMismatch { /// Expected element type for the [`Table`](crate::Table). @@ -44,16 +37,8 @@ pub enum TableError { impl Display for TableError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::GrowOutOfBounds { - maximum, - current, - delta, - } => { - write!( - f, - "tried to grow table with size of {current} and maximum of \ - {maximum} by {delta} out of bounds", - ) + Self::GrowOutOfBounds => { + write!(f, "out of bounds table growth") } Self::ElementTypeMismatch { expected, actual } => { write!(f, "encountered mismatching table element type, expected {expected:?} but found {actual:?}") diff --git a/crates/wasmi/src/table/mod.rs b/crates/wasmi/src/table/mod.rs index 379f7c6b2e..2a79b318cb 100644 --- a/crates/wasmi/src/table/mod.rs +++ b/crates/wasmi/src/table/mod.rs @@ -3,7 +3,15 @@ pub use self::{ error::TableError, }; use super::{AsContext, AsContextMut, Stored}; -use crate::{module::FuncIdx, store::ResourceLimiterRef, value::WithType, Func, FuncRef, Value}; +use crate::{ + engine::executor::EntityGrowError, + module::FuncIdx, + store::ResourceLimiterRef, + value::WithType, + Func, + FuncRef, + Value, +}; use alloc::vec::Vec; use core::cmp::max; use wasmi_arena::ArenaIndex; @@ -154,11 +162,10 @@ impl TableEntity { if let Some(limiter) = limiter.as_resource_limiter() { if limiter.table_growing(0, ty.minimum(), ty.maximum())? { - return Err(TableError::GrowOutOfBounds { - maximum: ty.maximum().unwrap_or(u32::MAX), - current: 0, - delta: ty.minimum(), - }); + // Here there's no meaningful way to map Ok(false) to + // INVALID_GROWTH_ERRCODE, so we just translate it to an + // appropriate Err(...) + return Err(TableError::GrowOutOfBounds); } } @@ -203,8 +210,10 @@ impl TableEntity { delta: u32, init: Value, limiter: &mut ResourceLimiterRef<'_>, - ) -> Result { - self.ty().matches_element_type(init.ty())?; + ) -> Result { + self.ty() + .matches_element_type(init.ty()) + .map_err(|_| EntityGrowError::InvalidGrow)?; self.grow_untyped(delta, init.into(), limiter) } @@ -226,18 +235,16 @@ impl TableEntity { delta: u32, init: UntypedValue, limiter: &mut ResourceLimiterRef<'_>, - ) -> Result { + ) -> Result { // ResourceLimiter gets first look at the request. let current = self.size(); let desired = current.checked_add(delta); let maximum = self.ty.maximum(); if let Some(limiter) = limiter.as_resource_limiter() { - if !limiter.table_growing(current, desired.unwrap_or(u32::MAX), maximum)? { - return Err(TableError::GrowOutOfBounds { - maximum: maximum.unwrap_or(u32::MAX), - current, - delta, - }); + match limiter.table_growing(current, desired.unwrap_or(u32::MAX), maximum) { + Ok(true) => (), + Ok(false) => return Err(EntityGrowError::InvalidGrow), + Err(_) => return Err(EntityGrowError::TrapCode(TrapCode::GrowthOperationLimited)), } } @@ -249,17 +256,11 @@ impl TableEntity { } } - let err = TableError::GrowOutOfBounds { - maximum, - current, - delta, - }; - // If there was an error, ResourceLimiter gets to see. if let Some(limiter) = limiter.as_resource_limiter() { - limiter.table_grow_failed(&err); + limiter.table_grow_failed(&TableError::GrowOutOfBounds); } - Err(err) + Err(EntityGrowError::InvalidGrow) } /// Converts the internal [`UntypedValue`] into a [`Value`] for this [`Table`] element type. @@ -585,6 +586,7 @@ impl Table { mut ctx: impl AsContextMut, delta: u32, init: Value, + limiter: &mut ResourceLimiterRef<'_>, ) -> Result { let (inner, mut limiter) = ctx .as_context_mut() @@ -593,6 +595,7 @@ impl Table { inner .resolve_table_mut(self) .grow(delta, init, &mut limiter) + .map_err(|_| TableError::GrowOutOfBounds) } /// Returns the [`Table`] element value at `index`. From 6afb5ef510ef059476d7ab3849674b2ad1d4cb66 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Thu, 29 Jun 2023 22:08:44 -0700 Subject: [PATCH 04/23] Fix typo in existing error message. --- crates/wasmi/src/memory/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasmi/src/memory/error.rs b/crates/wasmi/src/memory/error.rs index 13a52fd92a..ddbb381bab 100644 --- a/crates/wasmi/src/memory/error.rs +++ b/crates/wasmi/src/memory/error.rs @@ -31,7 +31,7 @@ impl Display for MemoryError { write!(f, "out of bounds memory allocation") } Self::OutOfBoundsGrowth => { - write!(f, "out fo bounds memory growth") + write!(f, "out of bounds memory growth") } Self::OutOfBoundsAccess => { write!(f, "out of bounds memory access") From 610fae74dc9072c68ae822f24a4a7837a9b0f622 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Thu, 29 Jun 2023 22:09:14 -0700 Subject: [PATCH 05/23] Add StoreLimits, StoreLimitsBuilder and docs from wasmtime. --- crates/wasmi/src/lib.rs | 2 +- crates/wasmi/src/limits.rs | 252 ++++++++++++++++++++++++++++++++++++- 2 files changed, 252 insertions(+), 2 deletions(-) diff --git a/crates/wasmi/src/lib.rs b/crates/wasmi/src/lib.rs index 5e10ff2d08..166a4fecb6 100644 --- a/crates/wasmi/src/lib.rs +++ b/crates/wasmi/src/lib.rs @@ -146,7 +146,7 @@ pub use self::{ }, global::{Global, GlobalType, Mutability}, instance::{Export, ExportsIter, Extern, ExternType, Instance}, - limits::ResourceLimiter, + limits::{ResourceLimiter, StoreLimits, StoreLimitsBuilder}, linker::Linker, memory::{Memory, MemoryType}, module::{ diff --git a/crates/wasmi/src/limits.rs b/crates/wasmi/src/limits.rs index 1012ff808f..5afda90bfc 100644 --- a/crates/wasmi/src/limits.rs +++ b/crates/wasmi/src/limits.rs @@ -9,7 +9,53 @@ pub const DEFAULT_TABLE_LIMIT: usize = 10000; /// Value returned by [`ResourceLimiter::memories`] default method pub const DEFAULT_MEMORY_LIMIT: usize = 10000; +/// Used by hosts to limit resource consumption of instances. +/// +/// This trait is used in conjunction with the +/// [`Store::limiter`](crate::Store::limiter) to limit the +/// allocation of resources within a store. As a store-level limit this means +/// that all creation of instances, memories, and tables are limited within the +/// store. Resources limited via this trait are primarily related to memory and +/// limiting CPU resources needs to be done with something such as +/// [`Config::consume_fuel`](crate::Config::consume_fuel). +/// +/// Note that this trait does not limit 100% of memory allocated via a +/// [`Store`](crate::Store). Wasmi will still allocate memory to track data +/// structures and additionally embedder-specific memory allocations are not +/// tracked via this trait. This trait only limits resources allocated by a +/// WebAssembly instance itself. pub trait ResourceLimiter { + /// Notifies the resource limiter that an instance's linear memory has been + /// requested to grow. + /// + /// * `current` is the current size of the linear memory in bytes. + /// * `desired` is the desired size of the linear memory in bytes. + /// * `maximum` is either the linear memory's maximum or a maximum from an + /// instance allocator, also in bytes. A value of `None` + /// indicates that the linear memory is unbounded. + /// + /// The `current` and `desired` amounts are guaranteed to always be + /// multiples of the WebAssembly page size, 64KiB. + /// + /// ## Return Value + /// + /// If `Ok(true)` is returned from this function then the growth operation + /// is allowed. This means that the wasm `memory.grow` instruction will + /// return with the `desired` size, in wasm pages. Note that even if + /// `Ok(true)` is returned, though, if `desired` exceeds `maximum` then the + /// growth operation will still fail. + /// + /// If `Ok(false)` is returned then this will cause the `memory.grow` + /// instruction in a module to return -1 (failure), or in the case of an + /// embedder API calling [`Memory::new`](crate::Memory::new) or + /// [`Memory::grow`](crate::Memory::grow) an error will be returned from + /// those methods. + /// + /// If `Err(e)` is returned then the `memory.grow` function will behave + /// as if a trap has been raised. Note that this is not necessarily + /// compliant with the WebAssembly specification but it can be a handy and + /// useful tool to get a precise backtrace at "what requested so much memory + /// to cause a growth failure?". fn memory_growing( &mut self, current: usize, @@ -17,6 +63,16 @@ pub trait ResourceLimiter { maximum: Option, ) -> Result; + /// Notifies the resource limiter that an instance's table has been + /// requested to grow. + /// + /// * `current` is the current number of elements in the table. + /// * `desired` is the desired number of elements in the table. + /// * `maximum` is either the table's maximum or a maximum from an instance + /// allocator. A value of `None` indicates that the table is unbounded. + /// + /// See the details on the return values for `memory_growing` for what the + /// return value of this function indicates. fn table_growing( &mut self, current: u32, @@ -24,16 +80,210 @@ pub trait ResourceLimiter { maximum: Option, ) -> Result; - // Provided methods + /// Notifies the resource limiter that growing a linear memory, permitted by + /// the `memory_growing` method, has failed. fn memory_grow_failed(&mut self, _error: &MemoryError) {} + + /// Notifies the resource limiter that growing a linear memory, permitted by + /// the `table_growing` method, has failed. fn table_grow_failed(&mut self, _error: &TableError) {} + + /// The maximum number of instances that can be created for a `Store`. + /// + /// Module instantiation will fail if this limit is exceeded. + /// + /// This value defaults to 10,000. fn instances(&self) -> usize { DEFAULT_INSTANCE_LIMIT } + + /// The maximum number of tables that can be created for a `Store`. + /// + /// Creation of tables will fail if this limit is exceeded. + /// + /// This value defaults to 10,000. fn tables(&self) -> usize { DEFAULT_TABLE_LIMIT } + + /// The maximum number of linear memories that can be created for a `Store` + /// + /// Creation of memories will fail with an error if this limit is exceeded. + /// + /// This value defaults to 10,000. fn memories(&self) -> usize { DEFAULT_MEMORY_LIMIT } } + +/// Used to build [`StoreLimits`]. +pub struct StoreLimitsBuilder(StoreLimits); + +impl StoreLimitsBuilder { + /// Creates a new [`StoreLimitsBuilder`]. + /// + /// See the documentation on each builder method for the default for each + /// value. + pub fn new() -> Self { + Self(StoreLimits::default()) + } + + /// The maximum number of bytes a linear memory can grow to. + /// + /// Growing a linear memory beyond this limit will fail. This limit is + /// applied to each linear memory individually, so if a wasm module has + /// multiple linear memories then they're all allowed to reach up to the + /// `limit` specified. + /// + /// By default, linear memory will not be limited. + pub fn memory_size(mut self, limit: usize) -> Self { + self.0.memory_size = Some(limit); + self + } + + /// The maximum number of elements in a table. + /// + /// Growing a table beyond this limit will fail. This limit is applied to + /// each table individually, so if a wasm module has multiple tables then + /// they're all allowed to reach up to the `limit` specified. + /// + /// By default, table elements will not be limited. + pub fn table_elements(mut self, limit: u32) -> Self { + self.0.table_elements = Some(limit); + self + } + + /// The maximum number of instances that can be created for a [`Store`](crate::Store). + /// + /// Module instantiation will fail if this limit is exceeded. + /// + /// This value defaults to 10,000. + pub fn instances(mut self, limit: usize) -> Self { + self.0.instances = limit; + self + } + + /// The maximum number of tables that can be created for a [`Store`](crate::Store). + /// + /// Module instantiation will fail if this limit is exceeded. + /// + /// This value defaults to 10,000. + pub fn tables(mut self, tables: usize) -> Self { + self.0.tables = tables; + self + } + + /// The maximum number of linear memories that can be created for a [`Store`](crate::Store). + /// + /// Instantiation will fail with an error if this limit is exceeded. + /// + /// This value defaults to 10,000. + pub fn memories(mut self, memories: usize) -> Self { + self.0.memories = memories; + self + } + + /// Indicates that a trap should be raised whenever a growth operation + /// would fail. + /// + /// This operation will force `memory.grow` and `table.grow` instructions + /// to raise a trap on failure instead of returning -1. This is not + /// necessarily spec-compliant, but it can be quite handy when debugging a + /// module that fails to allocate memory and might behave oddly as a result. + /// + /// This value defaults to `false`. + pub fn trap_on_grow_failure(mut self, trap: bool) -> Self { + self.0.trap_on_grow_failure = trap; + self + } + + /// Consumes this builder and returns the [`StoreLimits`]. + pub fn build(self) -> StoreLimits { + self.0 + } +} + +/// Provides limits for a [`Store`](crate::Store). +/// +/// This type is created with a [`StoreLimitsBuilder`] and is typically used in +/// conjunction with [`Store::limiter`](crate::Store::limiter). +/// +/// This is a convenience type included to avoid needing to implement the +/// [`ResourceLimiter`] trait if your use case fits in the static configuration +/// that this [`StoreLimits`] provides. +#[derive(Clone, Debug)] +pub struct StoreLimits { + memory_size: Option, + table_elements: Option, + instances: usize, + tables: usize, + memories: usize, + trap_on_grow_failure: bool, +} + +impl Default for StoreLimits { + fn default() -> Self { + Self { + memory_size: None, + table_elements: None, + instances: DEFAULT_INSTANCE_LIMIT, + tables: DEFAULT_TABLE_LIMIT, + memories: DEFAULT_MEMORY_LIMIT, + trap_on_grow_failure: false, + } + } +} + +impl ResourceLimiter for StoreLimits { + fn memory_growing( + &mut self, + _current: usize, + desired: usize, + maximum: Option, + ) -> Result { + let allow = match self.memory_size { + Some(limit) if desired > limit => false, + _ => match maximum { + Some(max) if desired > max => false, + _ => true, + }, + }; + if !allow && self.trap_on_grow_failure { + Err(MemoryError::OutOfBoundsGrowth) + } else { + Ok(allow) + } + } + + fn table_growing( + &mut self, + current: u32, + desired: u32, + maximum: Option, + ) -> Result { + let allow = match self.table_elements { + Some(limit) if desired > limit => false, + _ => match maximum { + Some(max) if desired > max => false, + _ => true, + }, + }; + if !allow && self.trap_on_grow_failure { + Err(TableError::GrowOutOfBounds) + } else { + Ok(allow) + } + } + + fn instances(&self) -> usize { + self.instances + } + + fn tables(&self) -> usize { + self.tables + } + + fn memories(&self) -> usize { + self.memories + } +} From 5f0a428c14f674d0bd41254b741224d64a2aa0c3 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Fri, 30 Jun 2023 12:59:49 -0700 Subject: [PATCH 06/23] Fix unused-variable warnings. --- crates/wasmi/src/limits.rs | 2 +- crates/wasmi/src/memory/mod.rs | 2 +- crates/wasmi/src/table/mod.rs | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/wasmi/src/limits.rs b/crates/wasmi/src/limits.rs index 5afda90bfc..2dd03ed5d5 100644 --- a/crates/wasmi/src/limits.rs +++ b/crates/wasmi/src/limits.rs @@ -257,7 +257,7 @@ impl ResourceLimiter for StoreLimits { fn table_growing( &mut self, - current: u32, + _current: u32, desired: u32, maximum: Option, ) -> Result { diff --git a/crates/wasmi/src/memory/mod.rs b/crates/wasmi/src/memory/mod.rs index a1d8313fec..2e24efeee5 100644 --- a/crates/wasmi/src/memory/mod.rs +++ b/crates/wasmi/src/memory/mod.rs @@ -238,7 +238,7 @@ impl MemoryEntity { } // If there was an error, ResourceLimiter gets to see. - if let Err(e) = &ret { + if ret.is_err() { if let Some(limiter) = limiter.as_resource_limiter() { limiter.memory_grow_failed(&MemoryError::OutOfBoundsGrowth) } diff --git a/crates/wasmi/src/table/mod.rs b/crates/wasmi/src/table/mod.rs index 2a79b318cb..39271a49a3 100644 --- a/crates/wasmi/src/table/mod.rs +++ b/crates/wasmi/src/table/mod.rs @@ -586,7 +586,6 @@ impl Table { mut ctx: impl AsContextMut, delta: u32, init: Value, - limiter: &mut ResourceLimiterRef<'_>, ) -> Result { let (inner, mut limiter) = ctx .as_context_mut() From 15ab19719d89c3c26a630da5faf35d508bf20db6 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Fri, 30 Jun 2023 13:01:14 -0700 Subject: [PATCH 07/23] Conditionalize ResourceLimiter on cfg(feature="std") --- crates/wasmi/src/module/instantiate/mod.rs | 5 +++- crates/wasmi/src/store.rs | 30 +++++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/crates/wasmi/src/module/instantiate/mod.rs b/crates/wasmi/src/module/instantiate/mod.rs index 803523caee..eb5a7d5c15 100644 --- a/crates/wasmi/src/module/instantiate/mod.rs +++ b/crates/wasmi/src/module/instantiate/mod.rs @@ -57,7 +57,10 @@ impl Module { let handle = context.as_context_mut().store.inner.alloc_instance(); let mut builder = InstanceEntity::build(self); - context.as_context_mut().store.bump_resource_counts(self)?; + #[cfg(feature = "std")] + { + context.as_context_mut().store.bump_resource_counts(self)?; + } self.extract_imports(&mut context, &mut builder, externals)?; self.extract_functions(&mut context, &mut builder, handle); diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index b5c623f571..8c4a886e0c 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -1,3 +1,5 @@ +#[cfg(feature = "std")] +use crate::Error; use crate::{ engine::DedupFuncType, externref::{ExternObject, ExternObjectEntity, ExternObjectIdx}, @@ -9,7 +11,6 @@ use crate::{ ElementSegmentEntity, ElementSegmentIdx, Engine, - Error, Func, FuncEntity, FuncIdx, @@ -81,9 +82,11 @@ impl<'a> ResourceLimiterRef<'a> { } } +#[cfg(feature = "std")] struct ResourceLimiterQuery( Box &mut (dyn crate::ResourceLimiter) + Send + Sync>, ); +#[cfg(feature = "std")] impl core::fmt::Debug for ResourceLimiterQuery { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "ResourceLimiterQuery(...)") @@ -104,6 +107,8 @@ pub struct Store { trampolines: Arena>, /// User provided host data owned by the [`Store`]. data: T, + /// User provided hook to retrieve a [`ResourceLimiter`]. + #[cfg(feature = "std")] limiter: Option>, } @@ -140,11 +145,17 @@ pub struct StoreInner { fuel: Fuel, // Numbers of resources instantiated in this store, and their limits + #[cfg(feature = "std")] instance_count: usize, + #[cfg(feature = "std")] instance_limit: usize, + #[cfg(feature = "std")] memory_count: usize, + #[cfg(feature = "std")] memory_limit: usize, + #[cfg(feature = "std")] table_count: usize, + #[cfg(feature = "std")] table_limit: usize, } @@ -273,11 +284,17 @@ impl StoreInner { // ResourceLimiter. Counts are kept separate from the sizes of arenas // above in order to allow checking "up front" in a single early and // fallible call to [Store::bump_resource_counts]. + #[cfg(feature = "std")] instance_count: 0, + #[cfg(feature = "std")] instance_limit: crate::limits::DEFAULT_INSTANCE_LIMIT, + #[cfg(feature = "std")] memory_count: 0, + #[cfg(feature = "std")] memory_limit: crate::limits::DEFAULT_MEMORY_LIMIT, + #[cfg(feature = "std")] table_count: 0, + #[cfg(feature = "std")] table_limit: crate::limits::DEFAULT_TABLE_LIMIT, } } @@ -734,6 +751,7 @@ impl Store { inner: StoreInner::new(engine), trampolines: Arena::new(), data, + #[cfg(feature = "std")] limiter: None, } } @@ -758,6 +776,7 @@ impl Store { self.data } + #[cfg(feature = "std")] pub fn limiter( &mut self, mut limiter: impl FnMut(&mut T) -> &mut (dyn crate::ResourceLimiter) + Send + Sync + 'static, @@ -773,6 +792,7 @@ impl Store { self.limiter = Some(ResourceLimiterQuery(Box::new(limiter))) } + #[cfg(feature = "std")] pub fn bump_resource_counts(&mut self, module: &crate::Module) -> Result<(), Error> { fn bump( slot: &mut usize, @@ -809,6 +829,7 @@ impl Store { Ok(()) } + #[cfg(feature = "std")] pub(crate) fn store_inner_and_resource_limiter_ref( &mut self, ) -> (&mut StoreInner, ResourceLimiterRef) { @@ -819,6 +840,13 @@ impl Store { (&mut self.inner, resource_limiter) } + #[cfg(not(feature = "std"))] + pub(crate) fn store_inner_and_resource_limiter_ref( + &mut self, + ) -> (&mut StoreInner, ResourceLimiterRef) { + (&mut self.inner, ResourceLimiterRef(None)) + } + /// Returns `true` if fuel metering has been enabled. fn is_fuel_metering_enabled(&self) -> bool { self.engine().config().get_consume_fuel() From bcfb80be867515448a5dca3e98a843576ef17cf7 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Fri, 30 Jun 2023 14:58:18 -0700 Subject: [PATCH 08/23] Add ResourceLimiter test and fix bugs it uncovered. --- crates/wasmi/src/module/instantiate/mod.rs | 15 +- crates/wasmi/src/table/mod.rs | 2 +- crates/wasmi/tests/e2e/v1/mod.rs | 1 + crates/wasmi/tests/e2e/v1/resource_limiter.rs | 209 ++++++++++++++++++ 4 files changed, 216 insertions(+), 11 deletions(-) create mode 100644 crates/wasmi/tests/e2e/v1/resource_limiter.rs diff --git a/crates/wasmi/src/module/instantiate/mod.rs b/crates/wasmi/src/module/instantiate/mod.rs index eb5a7d5c15..5e3948c906 100644 --- a/crates/wasmi/src/module/instantiate/mod.rs +++ b/crates/wasmi/src/module/instantiate/mod.rs @@ -8,7 +8,7 @@ pub use self::{error::InstantiationError, pre::InstancePre}; use super::{element::ElementSegmentKind, export, ConstExpr, DataSegmentKind, Module}; use crate::{ func::WasmFuncEntity, - memory::DataSegment, + memory::{DataSegment, MemoryError}, value::WithType, AsContext, AsContextMut, @@ -65,7 +65,7 @@ impl Module { self.extract_imports(&mut context, &mut builder, externals)?; self.extract_functions(&mut context, &mut builder, handle); self.extract_tables(&mut context, &mut builder)?; - self.extract_memories(&mut context, &mut builder); + self.extract_memories(&mut context, &mut builder)?; self.extract_globals(&mut context, &mut builder); self.extract_exports(&mut builder); self.extract_start_fn(&mut builder); @@ -209,17 +209,12 @@ impl Module { &self, context: &mut impl AsContextMut, builder: &mut InstanceEntityBuilder, - ) { + ) -> Result<(), MemoryError> { for memory_type in self.internal_memories().copied() { - let memory = - Memory::new(context.as_context_mut(), memory_type).unwrap_or_else(|error| { - panic!( - "encountered unexpected invalid memory type \ - {memory_type:?} after Wasm validation: {error}", - ) - }); + let memory = Memory::new(context.as_context_mut(), memory_type)?; builder.push_memory(memory); } + Ok(()) } /// Extracts the Wasm global variables from the module and stores them into the [`Store`]. diff --git a/crates/wasmi/src/table/mod.rs b/crates/wasmi/src/table/mod.rs index 39271a49a3..1581088f45 100644 --- a/crates/wasmi/src/table/mod.rs +++ b/crates/wasmi/src/table/mod.rs @@ -161,7 +161,7 @@ impl TableEntity { ty.matches_element_type(init.ty())?; if let Some(limiter) = limiter.as_resource_limiter() { - if limiter.table_growing(0, ty.minimum(), ty.maximum())? { + if !limiter.table_growing(0, ty.minimum(), ty.maximum())? { // Here there's no meaningful way to map Ok(false) to // INVALID_GROWTH_ERRCODE, so we just translate it to an // appropriate Err(...) diff --git a/crates/wasmi/tests/e2e/v1/mod.rs b/crates/wasmi/tests/e2e/v1/mod.rs index 1375eec594..11b43595cc 100644 --- a/crates/wasmi/tests/e2e/v1/mod.rs +++ b/crates/wasmi/tests/e2e/v1/mod.rs @@ -2,4 +2,5 @@ mod fuel_consumption_mode; mod fuel_metering; mod func; mod host_calls_wasm; +mod resource_limiter; mod resumable_call; diff --git a/crates/wasmi/tests/e2e/v1/resource_limiter.rs b/crates/wasmi/tests/e2e/v1/resource_limiter.rs new file mode 100644 index 0000000000..7010117c87 --- /dev/null +++ b/crates/wasmi/tests/e2e/v1/resource_limiter.rs @@ -0,0 +1,209 @@ +//! Tests to check if wasmi's ResourceLimiter works as intended. +use wasmi::{ + Config, + Engine, + Error, + Linker, + Module, + Store, + StoreLimits, + StoreLimitsBuilder, + TypedFunc, +}; +use wasmi_core::TrapCode; + +/// Setup [`Engine`] and [`Store`] for resource limiting. +fn test_setup(limits: StoreLimits) -> (Store, Linker) { + let config = Config::default(); + let engine = Engine::new(&config); + let mut store = Store::new(&engine, limits); + store.limiter(|limits| limits); + let linker = Linker::new(&engine); + (store, linker) +} + +/// Converts the `wat` string source into `wasm` encoded byte. +fn wat2wasm(wat: &str) -> Vec { + wat::parse_str(wat).unwrap() +} + +/// Compiles the `wasm` encoded bytes into a [`Module`]. +/// +/// # Panics +/// +/// If an error occurred upon module compilation, validation or translation. +fn create_module(store: &Store, bytes: &[u8]) -> Result { + Module::new(store.engine(), bytes) +} + +struct Test { + store: Store, + memory_grow: TypedFunc<(i32,), i32>, + memory_size: TypedFunc<(), i32>, + table_grow: TypedFunc<(i32,), i32>, + table_size: TypedFunc<(), i32>, +} + +impl Test { + fn new(mem_pages: i32, table_elts: i32, limits: StoreLimits) -> Result { + let wasm = wat2wasm(&format!( + r#" + (module + (memory {}) + (table {} funcref) + (func (export "memory_grow") (param $pages i32) (result i32) (memory.grow (local.get $pages))) + (func (export "memory_size") (result i32) (memory.size)) + (func (export "table_grow") (param $elts i32) (result i32) (table.grow (ref.func 0) (local.get $elts))) + (func (export "table_size") (result i32) (table.size)) + ) + "#, + mem_pages, table_elts + )); + let (mut store, linker) = test_setup(limits); + let module = create_module(&store, &wasm)?; + let instance = linker.instantiate(&mut store, &module)?.start(&mut store)?; + let memory_grow = instance.get_func(&store, "memory_grow").unwrap(); + let memory_size = instance.get_func(&store, "memory_size").unwrap(); + let table_grow = instance.get_func(&store, "table_grow").unwrap(); + let table_size = instance.get_func(&store, "table_size").unwrap(); + let memory_grow = memory_grow.typed::<(i32,), i32>(&store)?; + let memory_size = memory_size.typed::<(), i32>(&store)?; + let table_grow = table_grow.typed::<(i32,), i32>(&store)?; + let table_size = table_size.typed::<(), i32>(&store)?; + Ok(Self { + store, + memory_grow, + memory_size, + table_grow, + table_size, + }) + } +} + +#[test] +fn test_big_memory_fails_to_instantiate() { + let loose_limits = StoreLimitsBuilder::new().memory_size(0x30_0000).build(); + let tight_limits = StoreLimitsBuilder::new().memory_size(0x20_0000).build(); + assert!(Test::new(0x30, 0, loose_limits).is_ok()); + assert!(Test::new(0x30, 0, tight_limits).is_err()); +} + +#[test] +fn test_big_table_fails_to_instantiate() { + let loose_limits = StoreLimitsBuilder::new().table_elements(100).build(); + let tight_limits = StoreLimitsBuilder::new().table_elements(99).build(); + assert!(Test::new(0x30, 100, loose_limits).is_ok()); + assert!(Test::new(0x30, 100, tight_limits).is_err()); +} + +#[test] +fn test_memory_count_limit() { + let limits = StoreLimitsBuilder::new().memories(0).build(); + assert!(Test::new(0x30, 100, limits).is_err()); +} + +#[test] +fn test_instance_count_limit() { + let limits = StoreLimitsBuilder::new().instances(0).build(); + assert!(Test::new(0x30, 100, limits).is_err()); +} + +#[test] +fn test_tables_count_limit() { + let limits = StoreLimitsBuilder::new().tables(0).build(); + assert!(Test::new(0x30, 100, limits).is_err()); +} + +#[test] +fn test_memory_does_not_grow_on_limited_growth() -> Result<(), Error> { + let limits = StoreLimitsBuilder::new().memory_size(0x30_0000).build(); + let mut test = Test::new(0x20, 100, limits)?; + // By default the policy of a memory.grow failure is just for the instruction + // to return -1 and not-grow the underlying memory. We also have the option to + // trap on failure, which is exercised by the next test below. + + // Check memory size is what we expect. + assert_eq!(test.memory_size.call(&mut test.store, ())?, 0x20); + // First memory.grow doesn't hit the limit, so succeeds, returns previous size. + assert_eq!(test.memory_grow.call(&mut test.store, (0x10,))?, 0x20); + // Check memory size is what we expect. + assert_eq!(test.memory_size.call(&mut test.store, ())?, 0x30); + // Second call goes past the limit, so fails to grow the memory, but returns Ok(-1) + assert_eq!(test.memory_grow.call(&mut test.store, (0x10,))?, -1); + // Check memory size is what we expect. + assert_eq!(test.memory_size.call(&mut test.store, ())?, 0x30); + Ok(()) +} + +#[test] +fn test_memory_traps_on_limited_growth() -> Result<(), Error> { + let limits = StoreLimitsBuilder::new() + .memory_size(0x30_0000) + .trap_on_grow_failure(true) + .build(); + let mut test = Test::new(0x20, 100, limits)?; + // Check memory size is what we expect. + assert_eq!(test.memory_size.call(&mut test.store, ())?, 0x20); + // First memory.grow doesn't hit the limit, so succeeds, returns previous size. + assert_eq!(test.memory_grow.call(&mut test.store, (0x10,))?, 0x20); + // Check memory size is what we expect. + assert_eq!(test.memory_size.call(&mut test.store, ())?, 0x30); + // Second call goes past the limit, so fails to grow the memory, and we've configured it to trap. + assert!(matches!( + test.memory_grow + .call(&mut test.store, (0x10,)) + .unwrap_err() + .trap_code(), + Some(TrapCode::GrowthOperationLimited) + )); + // Check memory size is what we expect. + assert_eq!(test.memory_size.call(&mut test.store, ())?, 0x30); + Ok(()) +} + +#[test] +fn test_table_does_not_grow_on_limited_growth() -> Result<(), Error> { + let limits = StoreLimitsBuilder::new().table_elements(100).build(); + let mut test = Test::new(0x20, 99, limits)?; + // By default the policy of a table.grow failure is just for the instruction + // to return -1 and not-grow the underlying table. We also have the option to + // trap on failure, which is exercised by the next test below. + + // Check table size is what we expect. + assert_eq!(test.table_size.call(&mut test.store, ())?, 99); + // First table.grow doesn't hit the limit, so succeeds, returns previous size. + assert_eq!(test.table_grow.call(&mut test.store, (1,))?, 99); + // Check table size is what we expect. + assert_eq!(test.table_size.call(&mut test.store, ())?, 100); + // Second call goes past the limit, so fails to grow the table, but returns Ok(-1) + assert_eq!(test.table_grow.call(&mut test.store, (1,))?, -1); + // Check table size is what we expect. + assert_eq!(test.table_size.call(&mut test.store, ())?, 100); + Ok(()) +} + +#[test] +fn test_table_traps_on_limited_growth() -> Result<(), Error> { + let limits = StoreLimitsBuilder::new() + .table_elements(100) + .trap_on_grow_failure(true) + .build(); + let mut test = Test::new(0x20, 99, limits)?; + // Check table size is what we expect. + assert_eq!(test.table_size.call(&mut test.store, ())?, 99); + // First table.grow doesn't hit the limit, so succeeds, returns previous size. + assert_eq!(test.table_grow.call(&mut test.store, (1,))?, 99); + // Check table size is what we expect. + assert_eq!(test.table_size.call(&mut test.store, ())?, 100); + // Second call goes past the limit, so fails to grow the table, and we've configured it to trap. + assert!(matches!( + test.table_grow + .call(&mut test.store, (1,)) + .unwrap_err() + .trap_code(), + Some(TrapCode::GrowthOperationLimited) + )); + // Check table size is what we expect. + assert_eq!(test.table_size.call(&mut test.store, ())?, 100); + Ok(()) +} From 09aaf8d934dfc1f951ea4269e554c8330465972d Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Fri, 30 Jun 2023 15:41:55 -0700 Subject: [PATCH 09/23] Avoid redundant limit and size storage in previous wasmtime-like design. --- crates/wasmi/src/memory/mod.rs | 1 + crates/wasmi/src/module/instantiate/mod.rs | 17 +++- crates/wasmi/src/store.rs | 110 +++++++-------------- 3 files changed, 50 insertions(+), 78 deletions(-) diff --git a/crates/wasmi/src/memory/mod.rs b/crates/wasmi/src/memory/mod.rs index 2e24efeee5..a94662a3d3 100644 --- a/crates/wasmi/src/memory/mod.rs +++ b/crates/wasmi/src/memory/mod.rs @@ -316,6 +316,7 @@ impl Memory { .as_context_mut() .store .store_inner_and_resource_limiter_ref(); + let entity = MemoryEntity::new(ty, &mut resource_limiter)?; let memory = inner.alloc_memory(entity); Ok(memory) diff --git a/crates/wasmi/src/module/instantiate/mod.rs b/crates/wasmi/src/module/instantiate/mod.rs index 5e3948c906..cc84e4f5d7 100644 --- a/crates/wasmi/src/module/instantiate/mod.rs +++ b/crates/wasmi/src/module/instantiate/mod.rs @@ -54,14 +54,13 @@ impl Module { where I: IntoIterator, { + context + .as_context_mut() + .store + .check_new_instances_limit(1)?; let handle = context.as_context_mut().store.inner.alloc_instance(); let mut builder = InstanceEntity::build(self); - #[cfg(feature = "std")] - { - context.as_context_mut().store.bump_resource_counts(self)?; - } - self.extract_imports(&mut context, &mut builder, externals)?; self.extract_functions(&mut context, &mut builder, handle); self.extract_tables(&mut context, &mut builder)?; @@ -192,6 +191,10 @@ impl Module { context: &mut impl AsContextMut, builder: &mut InstanceEntityBuilder, ) -> Result<(), InstantiationError> { + context + .as_context_mut() + .store + .check_new_tables_limit(self.len_tables())?; for table_type in self.internal_tables().copied() { let init = Value::default(table_type.element()); let table = Table::new(context.as_context_mut(), table_type, init)?; @@ -210,6 +213,10 @@ impl Module { context: &mut impl AsContextMut, builder: &mut InstanceEntityBuilder, ) -> Result<(), MemoryError> { + context + .as_context_mut() + .store + .check_new_memories_limit(self.len_memories())?; for memory_type in self.internal_memories().copied() { let memory = Memory::new(context.as_context_mut(), memory_type)?; builder.push_memory(memory); diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 8c4a886e0c..0946f4be1c 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -1,10 +1,11 @@ #[cfg(feature = "std")] -use crate::Error; use crate::{ engine::DedupFuncType, externref::{ExternObject, ExternObjectEntity, ExternObjectIdx}, func::{Trampoline, TrampolineEntity, TrampolineIdx}, - memory::DataSegment, + memory::{DataSegment, MemoryError}, + module::InstantiationError, + table::TableError, DataSegmentEntity, DataSegmentIdx, ElementSegment, @@ -143,20 +144,6 @@ pub struct StoreInner { engine: Engine, /// The fuel of the [`Store`]. fuel: Fuel, - - // Numbers of resources instantiated in this store, and their limits - #[cfg(feature = "std")] - instance_count: usize, - #[cfg(feature = "std")] - instance_limit: usize, - #[cfg(feature = "std")] - memory_count: usize, - #[cfg(feature = "std")] - memory_limit: usize, - #[cfg(feature = "std")] - table_count: usize, - #[cfg(feature = "std")] - table_limit: usize, } #[test] @@ -279,23 +266,6 @@ impl StoreInner { elems: Arena::new(), extern_objects: Arena::new(), fuel: Fuel::default(), - - // Counts and limits of instances, memories and tables tracked by - // ResourceLimiter. Counts are kept separate from the sizes of arenas - // above in order to allow checking "up front" in a single early and - // fallible call to [Store::bump_resource_counts]. - #[cfg(feature = "std")] - instance_count: 0, - #[cfg(feature = "std")] - instance_limit: crate::limits::DEFAULT_INSTANCE_LIMIT, - #[cfg(feature = "std")] - memory_count: 0, - #[cfg(feature = "std")] - memory_limit: crate::limits::DEFAULT_MEMORY_LIMIT, - #[cfg(feature = "std")] - table_count: 0, - #[cfg(feature = "std")] - table_limit: crate::limits::DEFAULT_TABLE_LIMIT, } } @@ -779,53 +749,47 @@ impl Store { #[cfg(feature = "std")] pub fn limiter( &mut self, - mut limiter: impl FnMut(&mut T) -> &mut (dyn crate::ResourceLimiter) + Send + Sync + 'static, + limiter: impl FnMut(&mut T) -> &mut (dyn crate::ResourceLimiter) + Send + Sync + 'static, ) { - let inner = &mut self.inner; - let (instance_limit, table_limit, memory_limit) = { - let l = limiter(&mut self.data); - (l.instances(), l.tables(), l.memories()) - }; - inner.instance_limit = instance_limit; - inner.table_limit = table_limit; - inner.memory_limit = memory_limit; self.limiter = Some(ResourceLimiterQuery(Box::new(limiter))) } - #[cfg(feature = "std")] - pub fn bump_resource_counts(&mut self, module: &crate::Module) -> Result<(), Error> { - fn bump( - slot: &mut usize, - max: usize, - amt: usize, - err: impl FnOnce() -> Error, - ) -> Result<(), Error> { - let new = slot.saturating_add(amt); - if new > max { - return Err(err()); + pub(crate) fn check_new_instances_limit( + &mut self, + num_new_instances: usize, + ) -> Result<(), InstantiationError> { + let (inner, mut limiter) = self.store_inner_and_resource_limiter_ref(); + if let Some(limiter) = limiter.as_resource_limiter() { + if inner.instances.len().saturating_add(num_new_instances) > limiter.instances() { + return Err(InstantiationError::TooManyInstances.into()); } - *slot = new; - Ok(()) } + Ok(()) + } - let inner = &mut self.inner; - bump(&mut inner.instance_count, inner.instance_limit, 1, || { - crate::module::InstantiationError::TooManyInstances.into() - })?; - bump( - &mut inner.memory_count, - inner.memory_limit, - module.len_memories(), - || crate::memory::MemoryError::TooManyMemories.into(), - )?; - bump( - &mut inner.table_count, - inner.table_limit, - module.len_tables(), - || crate::table::TableError::TooManyTables.into(), - )?; - // TODO: maybe also limit globals, imports and exports? These are not in - // the wasmtime-based ResourceLimiter API. + pub(crate) fn check_new_memories_limit( + &mut self, + num_new_memories: usize, + ) -> Result<(), MemoryError> { + let (inner, mut limiter) = self.store_inner_and_resource_limiter_ref(); + if let Some(limiter) = limiter.as_resource_limiter() { + if inner.memories.len().saturating_add(num_new_memories) > limiter.memories() { + return Err(MemoryError::TooManyMemories); + } + } + Ok(()) + } + + pub(crate) fn check_new_tables_limit( + &mut self, + num_new_tables: usize, + ) -> Result<(), TableError> { + let (inner, mut limiter) = self.store_inner_and_resource_limiter_ref(); + if let Some(limiter) = limiter.as_resource_limiter() { + if inner.tables.len().saturating_add(num_new_tables) > limiter.tables() { + return Err(TableError::TooManyTables); + } + } Ok(()) } From 8eeea83b329ff215b38ea20a50931cbcc9402c70 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Fri, 30 Jun 2023 15:46:51 -0700 Subject: [PATCH 10/23] Thread an ever-so-smaller &mut ResourceLimiterRef into the Executor --- crates/wasmi/src/engine/executor.rs | 6 +++--- crates/wasmi/src/engine/mod.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/wasmi/src/engine/executor.rs b/crates/wasmi/src/engine/executor.rs index 0b7f00f9a3..41c696b602 100644 --- a/crates/wasmi/src/engine/executor.rs +++ b/crates/wasmi/src/engine/executor.rs @@ -100,7 +100,7 @@ pub fn execute_wasm<'ctx, 'engine>( call_stack: &'engine mut CallStack, code_map: &'engine CodeMap, const_pool: ConstPoolView<'engine>, - resource_limiter: ResourceLimiterRef<'ctx>, + resource_limiter: &'ctx mut ResourceLimiterRef<'ctx>, ) -> Result { Executor::new( ctx, @@ -179,7 +179,7 @@ struct Executor<'ctx, 'engine> { code_map: &'engine CodeMap, /// A read-only view to a pool of constant values. const_pool: ConstPoolView<'engine>, - resource_limiter: ResourceLimiterRef<'ctx>, + resource_limiter: &'ctx mut ResourceLimiterRef<'ctx>, } macro_rules! forward_call { @@ -207,7 +207,7 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { call_stack: &'engine mut CallStack, code_map: &'engine CodeMap, const_pool: ConstPoolView<'engine>, - resource_limiter: ResourceLimiterRef<'ctx>, + resource_limiter: &'ctx mut ResourceLimiterRef<'ctx>, ) -> Self { let frame = call_stack.pop().expect("must have frame on the call stack"); let sp = value_stack.stack_ptr(); diff --git a/crates/wasmi/src/engine/mod.rs b/crates/wasmi/src/engine/mod.rs index 8c76be1ce4..ec0adecacd 100644 --- a/crates/wasmi/src/engine/mod.rs +++ b/crates/wasmi/src/engine/mod.rs @@ -793,7 +793,7 @@ impl<'engine> EngineExecutor<'engine> { code.into() } - let (store_inner, resource_limiter) = ctx.store.store_inner_and_resource_limiter_ref(); + let (store_inner, mut resource_limiter) = ctx.store.store_inner_and_resource_limiter_ref(); let value_stack = &mut self.stack.values; let call_stack = &mut self.stack.frames; let code_map = &self.res.code_map; @@ -806,7 +806,7 @@ impl<'engine> EngineExecutor<'engine> { call_stack, code_map, const_pool, - resource_limiter, + &mut resource_limiter, ) .map_err(make_trap) } From 7dc0251f5e1422100019a2b0de010c3d583741a2 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Fri, 30 Jun 2023 22:23:22 -0700 Subject: [PATCH 11/23] Support no_std differently (import alloc::boxed::Box) --- crates/wasmi/src/store.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 0946f4be1c..289c32e803 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -1,4 +1,3 @@ -#[cfg(feature = "std")] use crate::{ engine::DedupFuncType, externref::{ExternObject, ExternObjectEntity, ExternObjectIdx}, @@ -29,6 +28,8 @@ use crate::{ TableEntity, TableIdx, }; +#[cfg(feature = "std")] +use alloc::boxed::Box; use core::{ fmt::{self, Debug}, sync::atomic::{AtomicU32, Ordering}, @@ -83,11 +84,9 @@ impl<'a> ResourceLimiterRef<'a> { } } -#[cfg(feature = "std")] struct ResourceLimiterQuery( Box &mut (dyn crate::ResourceLimiter) + Send + Sync>, ); -#[cfg(feature = "std")] impl core::fmt::Debug for ResourceLimiterQuery { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "ResourceLimiterQuery(...)") @@ -109,7 +108,6 @@ pub struct Store { /// User provided host data owned by the [`Store`]. data: T, /// User provided hook to retrieve a [`ResourceLimiter`]. - #[cfg(feature = "std")] limiter: Option>, } @@ -721,7 +719,6 @@ impl Store { inner: StoreInner::new(engine), trampolines: Arena::new(), data, - #[cfg(feature = "std")] limiter: None, } } @@ -746,7 +743,6 @@ impl Store { self.data } - #[cfg(feature = "std")] pub fn limiter( &mut self, limiter: impl FnMut(&mut T) -> &mut (dyn crate::ResourceLimiter) + Send + Sync + 'static, @@ -793,7 +789,6 @@ impl Store { Ok(()) } - #[cfg(feature = "std")] pub(crate) fn store_inner_and_resource_limiter_ref( &mut self, ) -> (&mut StoreInner, ResourceLimiterRef) { @@ -804,13 +799,6 @@ impl Store { (&mut self.inner, resource_limiter) } - #[cfg(not(feature = "std"))] - pub(crate) fn store_inner_and_resource_limiter_ref( - &mut self, - ) -> (&mut StoreInner, ResourceLimiterRef) { - (&mut self.inner, ResourceLimiterRef(None)) - } - /// Returns `true` if fuel metering has been enabled. fn is_fuel_metering_enabled(&self) -> bool { self.engine().config().get_consume_fuel() From 071df16f176ec1bbb36bda464ece7b23dba7f57d Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Sat, 1 Jul 2023 01:16:50 -0700 Subject: [PATCH 12/23] Avoid storing &ResourceLimiterRef in Executor (maybe breaks SROA?) --- crates/wasmi/src/engine/executor.rs | 45 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/crates/wasmi/src/engine/executor.rs b/crates/wasmi/src/engine/executor.rs index 41c696b602..d36180eb3f 100644 --- a/crates/wasmi/src/engine/executor.rs +++ b/crates/wasmi/src/engine/executor.rs @@ -102,16 +102,8 @@ pub fn execute_wasm<'ctx, 'engine>( const_pool: ConstPoolView<'engine>, resource_limiter: &'ctx mut ResourceLimiterRef<'ctx>, ) -> Result { - Executor::new( - ctx, - cache, - value_stack, - call_stack, - code_map, - const_pool, - resource_limiter, - ) - .execute() + Executor::new(ctx, cache, value_stack, call_stack, code_map, const_pool) + .execute(resource_limiter) } /// The function signature of Wasm load operations. @@ -179,7 +171,6 @@ struct Executor<'ctx, 'engine> { code_map: &'engine CodeMap, /// A read-only view to a pool of constant values. const_pool: ConstPoolView<'engine>, - resource_limiter: &'ctx mut ResourceLimiterRef<'ctx>, } macro_rules! forward_call { @@ -207,7 +198,6 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { call_stack: &'engine mut CallStack, code_map: &'engine CodeMap, const_pool: ConstPoolView<'engine>, - resource_limiter: &'ctx mut ResourceLimiterRef<'ctx>, ) -> Self { let frame = call_stack.pop().expect("must have frame on the call stack"); let sp = value_stack.stack_ptr(); @@ -221,13 +211,15 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { call_stack, code_map, const_pool, - resource_limiter, } } /// Executes the function frame until it returns or traps. #[inline(always)] - fn execute(mut self) -> Result { + fn execute( + mut self, + resource_limiter: &'ctx mut ResourceLimiterRef<'ctx>, + ) -> Result { use Instruction as Instr; loop { match *self.ip.get() { @@ -294,13 +286,13 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { Instr::I64Store16(offset) => self.visit_i64_store_16(offset)?, Instr::I64Store32(offset) => self.visit_i64_store_32(offset)?, Instr::MemorySize => self.visit_memory_size(), - Instr::MemoryGrow => self.visit_memory_grow()?, + Instr::MemoryGrow => self.visit_memory_grow(&mut *resource_limiter)?, Instr::MemoryFill => self.visit_memory_fill()?, Instr::MemoryCopy => self.visit_memory_copy()?, Instr::MemoryInit(segment) => self.visit_memory_init(segment)?, Instr::DataDrop(segment) => self.visit_data_drop(segment), Instr::TableSize(table) => self.visit_table_size(table), - Instr::TableGrow(table) => self.visit_table_grow(table)?, + Instr::TableGrow(table) => self.visit_table_grow(table, &mut *resource_limiter)?, Instr::TableFill(table) => self.visit_table_fill(table)?, Instr::TableGet(table) => self.visit_table_get(table)?, Instr::TableSet(table) => self.visit_table_set(table)?, @@ -1091,7 +1083,10 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { } #[inline(always)] - fn visit_memory_grow(&mut self) -> Result<(), TrapCode> { + fn visit_memory_grow( + &mut self, + resource_limiter: &mut ResourceLimiterRef<'ctx>, + ) -> Result<(), TrapCode> { let delta: u32 = self.sp.pop_as(); let delta = match Pages::new(delta) { Some(pages) => pages, @@ -1111,7 +1106,7 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { let new_pages = this .ctx .resolve_memory_mut(memory) - .grow(delta, &mut this.resource_limiter) + .grow(delta, resource_limiter) .map(u32::from)?; // The `memory.grow` operation might have invalidated the cached // linear memory so we need to reset it in order for the cache to @@ -1223,18 +1218,20 @@ impl<'ctx, 'engine> Executor<'ctx, 'engine> { } #[inline(always)] - fn visit_table_grow(&mut self, table_index: TableIdx) -> Result<(), TrapCode> { + fn visit_table_grow( + &mut self, + table_index: TableIdx, + resource_limiter: &mut ResourceLimiterRef<'ctx>, + ) -> Result<(), TrapCode> { let (init, delta) = self.sp.pop2(); let delta: u32 = delta.into(); let result = self.consume_fuel_with( |costs| costs.fuel_for_elements(u64::from(delta)), |this| { let table = this.cache.get_table(this.ctx, table_index); - this.ctx.resolve_table_mut(&table).grow_untyped( - delta, - init, - &mut this.resource_limiter, - ) + this.ctx + .resolve_table_mut(&table) + .grow_untyped(delta, init, resource_limiter) }, ); let result = match result { From 32da9fffafcc13661610b49ea057d982ddc38761 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Sat, 1 Jul 2023 01:24:38 -0700 Subject: [PATCH 13/23] Fix no_std a little harder. --- crates/wasmi/src/store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 289c32e803..43fd446f9a 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -28,7 +28,7 @@ use crate::{ TableEntity, TableIdx, }; -#[cfg(feature = "std")] +#[cfg(not(feature = "std"))] use alloc::boxed::Box; use core::{ fmt::{self, Debug}, From 9f68c3894534c242db57da3a973a06010c4eb325 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 3 Jul 2023 13:02:28 -0700 Subject: [PATCH 14/23] Restore TableError::GrowOutOfBounds fields --- crates/wasmi/src/limits.rs | 8 ++++++-- crates/wasmi/src/table/error.rs | 21 ++++++++++++++++++--- crates/wasmi/src/table/mod.rs | 24 +++++++++++++++++++----- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/crates/wasmi/src/limits.rs b/crates/wasmi/src/limits.rs index 2dd03ed5d5..9044a6b2c7 100644 --- a/crates/wasmi/src/limits.rs +++ b/crates/wasmi/src/limits.rs @@ -257,7 +257,7 @@ impl ResourceLimiter for StoreLimits { fn table_growing( &mut self, - _current: u32, + current: u32, desired: u32, maximum: Option, ) -> Result { @@ -269,7 +269,11 @@ impl ResourceLimiter for StoreLimits { }, }; if !allow && self.trap_on_grow_failure { - Err(TableError::GrowOutOfBounds) + Err(TableError::GrowOutOfBounds { + maximum: maximum.unwrap_or(u32::MAX), + current: current, + delta: desired - current, + }) } else { Ok(allow) } diff --git a/crates/wasmi/src/table/error.rs b/crates/wasmi/src/table/error.rs index af9e5c84e1..c04f806665 100644 --- a/crates/wasmi/src/table/error.rs +++ b/crates/wasmi/src/table/error.rs @@ -7,7 +7,14 @@ use core::{fmt, fmt::Display}; #[non_exhaustive] pub enum TableError { /// Occurs when growing a table out of its set bounds. - GrowOutOfBounds, + GrowOutOfBounds { + /// The maximum allowed table size. + maximum: u32, + /// The current table size before the growth operation. + current: u32, + /// The amount of requested invalid growth. + delta: u32, + }, /// Occurs when operating with a [`Table`](crate::Table) and mismatching element types. ElementTypeMismatch { /// Expected element type for the [`Table`](crate::Table). @@ -37,8 +44,16 @@ pub enum TableError { impl Display for TableError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::GrowOutOfBounds => { - write!(f, "out of bounds table growth") + Self::GrowOutOfBounds { + maximum, + current, + delta, + } => { + write!( + f, + "tried to grow table with size of {current} and maximum of \ + {maximum} by {delta} out of bounds", + ) } Self::ElementTypeMismatch { expected, actual } => { write!(f, "encountered mismatching table element type, expected {expected:?} but found {actual:?}") diff --git a/crates/wasmi/src/table/mod.rs b/crates/wasmi/src/table/mod.rs index 1581088f45..5e13efb03d 100644 --- a/crates/wasmi/src/table/mod.rs +++ b/crates/wasmi/src/table/mod.rs @@ -165,7 +165,11 @@ impl TableEntity { // Here there's no meaningful way to map Ok(false) to // INVALID_GROWTH_ERRCODE, so we just translate it to an // appropriate Err(...) - return Err(TableError::GrowOutOfBounds); + return Err(TableError::GrowOutOfBounds { + maximum: ty.maximum().unwrap_or(u32::MAX), + current: 0, + delta: ty.minimum(), + }); } } @@ -258,7 +262,11 @@ impl TableEntity { // If there was an error, ResourceLimiter gets to see. if let Some(limiter) = limiter.as_resource_limiter() { - limiter.table_grow_failed(&TableError::GrowOutOfBounds); + limiter.table_grow_failed(&TableError::GrowOutOfBounds { + maximum, + current, + delta, + }); } Err(EntityGrowError::InvalidGrow) } @@ -591,10 +599,16 @@ impl Table { .as_context_mut() .store .store_inner_and_resource_limiter_ref(); - inner - .resolve_table_mut(self) + let table = inner.resolve_table_mut(self); + let current = table.size(); + let maximum = table.ty().maximum().unwrap_or(u32::MAX); + table .grow(delta, init, &mut limiter) - .map_err(|_| TableError::GrowOutOfBounds) + .map_err(|_| TableError::GrowOutOfBounds { + maximum, + current, + delta, + }) } /// Returns the [`Table`] element value at `index`. From 14b3c07921e3055a3b401c9a516c17786db6b440 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 3 Jul 2023 13:03:50 -0700 Subject: [PATCH 15/23] Remove now-redundant pub(crate) --- crates/wasmi/src/store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 43fd446f9a..51b9f048f5 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -71,7 +71,7 @@ impl StoreIdx { /// A stored entity. pub type Stored = GuardedEntity; -pub struct ResourceLimiterRef<'a>(pub(crate) Option<&'a mut (dyn crate::ResourceLimiter)>); +pub struct ResourceLimiterRef<'a>(Option<&'a mut (dyn crate::ResourceLimiter)>); impl<'a> core::fmt::Debug for ResourceLimiterRef<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "ResourceLimiterRef(...)") From 11a07392d2b0f633f19e1e8b767d7a5f31398fb9 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 3 Jul 2023 13:06:10 -0700 Subject: [PATCH 16/23] Remove unnecessary cfg guard on alloc::boxed::Box --- crates/wasmi/src/store.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 51b9f048f5..ac261af709 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -28,7 +28,6 @@ use crate::{ TableEntity, TableIdx, }; -#[cfg(not(feature = "std"))] use alloc::boxed::Box; use core::{ fmt::{self, Debug}, From 8436a1c84788806d8da4f6e1cec442cde12f8910 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 3 Jul 2023 13:19:02 -0700 Subject: [PATCH 17/23] Address clippy warnings --- crates/wasmi/src/limits.rs | 18 ++++++++++++++---- crates/wasmi/src/store.rs | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/wasmi/src/limits.rs b/crates/wasmi/src/limits.rs index 9044a6b2c7..bcc565f785 100644 --- a/crates/wasmi/src/limits.rs +++ b/crates/wasmi/src/limits.rs @@ -51,6 +51,8 @@ pub trait ResourceLimiter { /// [`Memory::grow`](crate::Memory::grow) an error will be returned from /// those methods. /// + /// # Errors + /// /// If `Err(e)` is returned then the `memory.grow` function will behave /// as if a trap has been raised. Note that this is not necessarily /// compliant with the WebAssembly specification but it can be a handy and @@ -71,8 +73,10 @@ pub trait ResourceLimiter { /// * `maximum` is either the table's maximum or a maximum from an instance /// allocator. A value of `None` indicates that the table is unbounded. /// + /// # Errors + /// /// See the details on the return values for `memory_growing` for what the - /// return value of this function indicates. + /// return values of this function indicates. fn table_growing( &mut self, current: u32, @@ -203,6 +207,12 @@ impl StoreLimitsBuilder { } } +impl Default for StoreLimitsBuilder { + fn default() -> Self { + Self::new() + } +} + /// Provides limits for a [`Store`](crate::Store). /// /// This type is created with a [`StoreLimitsBuilder`] and is typically used in @@ -245,7 +255,7 @@ impl ResourceLimiter for StoreLimits { Some(limit) if desired > limit => false, _ => match maximum { Some(max) if desired > max => false, - _ => true, + Some(_) | None => true, }, }; if !allow && self.trap_on_grow_failure { @@ -265,13 +275,13 @@ impl ResourceLimiter for StoreLimits { Some(limit) if desired > limit => false, _ => match maximum { Some(max) if desired > max => false, - _ => true, + Some(_) | None => true, }, }; if !allow && self.trap_on_grow_failure { Err(TableError::GrowOutOfBounds { maximum: maximum.unwrap_or(u32::MAX), - current: current, + current, delta: desired - current, }) } else { diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index ac261af709..9aa5f0ef62 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -756,7 +756,7 @@ impl Store { let (inner, mut limiter) = self.store_inner_and_resource_limiter_ref(); if let Some(limiter) = limiter.as_resource_limiter() { if inner.instances.len().saturating_add(num_new_instances) > limiter.instances() { - return Err(InstantiationError::TooManyInstances.into()); + return Err(InstantiationError::TooManyInstances); } } Ok(()) From 1b07fa465437b501a0d7e9adae431b5ba7342cbb Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 3 Jul 2023 13:22:27 -0700 Subject: [PATCH 18/23] Fix doc reference. --- crates/wasmi/src/store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 9aa5f0ef62..368ee3d0da 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -106,7 +106,7 @@ pub struct Store { trampolines: Arena>, /// User provided host data owned by the [`Store`]. data: T, - /// User provided hook to retrieve a [`ResourceLimiter`]. + /// User provided hook to retrieve a [`crate::ResourceLimiter`]. limiter: Option>, } From c005b8ff39d44ad49c28dcf3be8ab38cb1c45be4 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 3 Jul 2023 19:54:08 -0700 Subject: [PATCH 19/23] Some cleanups as requested in review. --- crates/core/src/trap.rs | 8 ++++---- crates/wasmi/src/store.rs | 24 +++++++++++++++++------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/crates/core/src/trap.rs b/crates/core/src/trap.rs index 6e5f0656ce..637ef97616 100644 --- a/crates/core/src/trap.rs +++ b/crates/core/src/trap.rs @@ -286,10 +286,10 @@ pub enum TrapCode { OutOfFuel, /// This trap is raised when a growth operation was attempted and an - /// installed `ResourceLimiter` returned `Err(...)` from the associated - /// `table_growing` or `memory_growing` method, indicating a desire on the - /// part of the embedder to trap the interpreter rather than merely fail the - /// growth operation. + /// installed [`ResourceLimiter`](crate::ResourceLimiter) returned + /// `Err(...)` from the associated `table_growing` or `memory_growing` + /// method, indicating a desire on the part of the embedder to trap the + /// interpreter rather than merely fail the growth operation. GrowthOperationLimited, } diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 368ee3d0da..604e292a08 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -24,6 +24,7 @@ use crate::{ Memory, MemoryEntity, MemoryIdx, + ResourceLimiter, Table, TableEntity, TableIdx, @@ -70,7 +71,10 @@ impl StoreIdx { /// A stored entity. pub type Stored = GuardedEntity; -pub struct ResourceLimiterRef<'a>(Option<&'a mut (dyn crate::ResourceLimiter)>); +/// A wrapper around an optional `&mut dyn` [`ResourceLimiter`], that exists +/// both to make types a little easier to read and to provide a `Debug` impl so +/// that `#[derive(Debug)]` works on structs that contain it. +pub struct ResourceLimiterRef<'a>(Option<&'a mut (dyn ResourceLimiter)>); impl<'a> core::fmt::Debug for ResourceLimiterRef<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "ResourceLimiterRef(...)") @@ -78,14 +82,19 @@ impl<'a> core::fmt::Debug for ResourceLimiterRef<'a> { } impl<'a> ResourceLimiterRef<'a> { - pub fn as_resource_limiter(&mut self) -> &mut Option<&'a mut dyn crate::ResourceLimiter> { + pub fn as_resource_limiter(&mut self) -> &mut Option<&'a mut dyn ResourceLimiter> { &mut self.0 } } -struct ResourceLimiterQuery( - Box &mut (dyn crate::ResourceLimiter) + Send + Sync>, -); +/// A wrapper around a boxed `dyn FnMut(&mut T)` returning a `&mut dyn` +/// [`ResourceLimiter`]; in other words a function that one can call to retrieve +/// a [`ResourceLimiter`] from the [`State`] object's user data type `T`. +/// +/// This wrapper exists both to make types a little easier to read and to +/// provide a `Debug` impl so that `#[derive(Debug)]` works on structs that +/// contain it. +struct ResourceLimiterQuery(Box &mut (dyn ResourceLimiter) + Send + Sync>); impl core::fmt::Debug for ResourceLimiterQuery { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "ResourceLimiterQuery(...)") @@ -106,7 +115,8 @@ pub struct Store { trampolines: Arena>, /// User provided host data owned by the [`Store`]. data: T, - /// User provided hook to retrieve a [`crate::ResourceLimiter`]. + /// User provided hook to retrieve a + /// [`ResourceLimiter`](crate::ResourceLimiter). limiter: Option>, } @@ -744,7 +754,7 @@ impl Store { pub fn limiter( &mut self, - limiter: impl FnMut(&mut T) -> &mut (dyn crate::ResourceLimiter) + Send + Sync + 'static, + limiter: impl FnMut(&mut T) -> &mut (dyn ResourceLimiter) + Send + Sync + 'static, ) { self.limiter = Some(ResourceLimiterQuery(Box::new(limiter))) } From 831ec4624fa07d4221715b58f1f92141e654ed85 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 12:39:16 -0700 Subject: [PATCH 20/23] Fix wasmi_core doc-link to wasmi, deps do not go that way. --- crates/core/src/trap.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/core/src/trap.rs b/crates/core/src/trap.rs index 637ef97616..a48a2dc3cd 100644 --- a/crates/core/src/trap.rs +++ b/crates/core/src/trap.rs @@ -286,10 +286,10 @@ pub enum TrapCode { OutOfFuel, /// This trap is raised when a growth operation was attempted and an - /// installed [`ResourceLimiter`](crate::ResourceLimiter) returned - /// `Err(...)` from the associated `table_growing` or `memory_growing` - /// method, indicating a desire on the part of the embedder to trap the - /// interpreter rather than merely fail the growth operation. + /// installed `wasmi::ResourceLimiter` returned `Err(...)` from the + /// associated `table_growing` or `memory_growing` method, indicating a + /// desire on the part of the embedder to trap the interpreter rather than + /// merely fail the growth operation. GrowthOperationLimited, } From 61d8e4598564c5b5ea0d21148046aa4544ee3119 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 12:39:42 -0700 Subject: [PATCH 21/23] Fix s/State/Store/ doc typo --- crates/wasmi/src/store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 604e292a08..846857e39b 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -89,7 +89,7 @@ impl<'a> ResourceLimiterRef<'a> { /// A wrapper around a boxed `dyn FnMut(&mut T)` returning a `&mut dyn` /// [`ResourceLimiter`]; in other words a function that one can call to retrieve -/// a [`ResourceLimiter`] from the [`State`] object's user data type `T`. +/// a [`ResourceLimiter`] from the [`Store`] object's user data type `T`. /// /// This wrapper exists both to make types a little easier to read and to /// provide a `Debug` impl so that `#[derive(Debug)]` works on structs that From a11d8c1dfa2e1cbb16fe2c9c50ef564a864db412 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 12:39:55 -0700 Subject: [PATCH 22/23] Add docs on Store::limiter --- crates/wasmi/src/store.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/wasmi/src/store.rs b/crates/wasmi/src/store.rs index 846857e39b..7612ad72fc 100644 --- a/crates/wasmi/src/store.rs +++ b/crates/wasmi/src/store.rs @@ -752,6 +752,9 @@ impl Store { self.data } + /// Installs a function into the [`Store`] that will be called with the user + /// data type `T` to retrieve a [`ResourceLimiter`] any time a limited, + /// growable resource such as a linear memory or table is grown. pub fn limiter( &mut self, limiter: impl FnMut(&mut T) -> &mut (dyn ResourceLimiter) + Send + Sync + 'static, From 25b4e7a8f0548cd7ba5bc0ae80e467ceb0ea4839 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 13:11:36 -0700 Subject: [PATCH 23/23] Fix new nightly clippy nit --- crates/wasmi/src/func/typed_func.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/wasmi/src/func/typed_func.rs b/crates/wasmi/src/func/typed_func.rs index b98fd3bdbf..3dcadd4d64 100644 --- a/crates/wasmi/src/func/typed_func.rs +++ b/crates/wasmi/src/func/typed_func.rs @@ -173,9 +173,7 @@ impl Default for CallResultsTuple { impl Copy for CallResultsTuple {} impl Clone for CallResultsTuple { fn clone(&self) -> Self { - Self { - _marker: PhantomData, - } + *self } }