From f82ed77000b520046918b4e75cd733a2a7650d5b Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 12 Dec 2022 14:30:25 +0100 Subject: [PATCH] Fix duplicated imported linear memories and tables (#593) * add regression tests asserting the existence of the issue * add iterators over internal memories and tables to fix the issue * apply clippy suggestions * fix intra doc links --- crates/wasmi/src/module/instantiate/mod.rs | 7 +- crates/wasmi/src/module/instantiate/tests.rs | 104 +++++++++++++++++++ crates/wasmi/src/module/mod.rs | 32 ++++++ 3 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 crates/wasmi/src/module/instantiate/tests.rs diff --git a/crates/wasmi/src/module/instantiate/mod.rs b/crates/wasmi/src/module/instantiate/mod.rs index d7c3f7e897..525b251564 100644 --- a/crates/wasmi/src/module/instantiate/mod.rs +++ b/crates/wasmi/src/module/instantiate/mod.rs @@ -1,6 +1,9 @@ mod error; mod pre; +#[cfg(test)] +mod tests; + pub use self::{error::InstantiationError, pre::InstancePre}; use super::{export, InitExpr, Module, ModuleImportType}; use crate::{ @@ -190,7 +193,7 @@ impl Module { /// /// [`Store`]: struct.Store.html fn extract_tables(&self, context: &mut impl AsContextMut, builder: &mut InstanceEntityBuilder) { - for table_type in self.tables.iter().copied() { + for table_type in self.internal_tables().copied() { builder.push_table(Table::new(context.as_context_mut(), table_type)); } } @@ -205,7 +208,7 @@ impl Module { context: &mut impl AsContextMut, builder: &mut InstanceEntityBuilder, ) { - for memory_type in self.memories.iter().copied() { + for memory_type in self.internal_memories().copied() { let memory = Memory::new(context.as_context_mut(), memory_type).unwrap_or_else(|error| { panic!( diff --git a/crates/wasmi/src/module/instantiate/tests.rs b/crates/wasmi/src/module/instantiate/tests.rs new file mode 100644 index 0000000000..76689bc153 --- /dev/null +++ b/crates/wasmi/src/module/instantiate/tests.rs @@ -0,0 +1,104 @@ +//! Regression tests for GitHub issue: +//! https://github.com/paritytech/wasmi/issues/587 +//! +//! The problem was that Wasm memories (and tables) were defined twice for a +//! `wasmi` instance for every imported Wasm memory (or table). Since `wasmi` +//! does not support the `multi-memory` Wasm proposal this resulted Wasm +//! instances with more than 1 memory (or table) if the Wasm module imported +//! those entities. + +use crate::{Engine, Error, Instance, Linker, Memory, MemoryType, Module, Store, Table, TableType}; + +fn try_instantiate_from_wat(wat: &str) -> Result<(Store<()>, Instance), Error> { + let wasm = wat::parse_str(wat).unwrap(); + let engine = Engine::default(); + let module = Module::new(&engine, &mut &wasm[..])?; + let mut store = Store::new(&engine, ()); + let mut linker = >::new(); + // Define one memory that can be used by the tests as import. + let memory_type = MemoryType::new(4, None)?; + let memory = Memory::new(&mut store, memory_type)?; + linker.define("env", "memory", memory)?; + // Define one table that can be used by the tests as import. + let table_type = TableType::new(4, None); + let table = Table::new(&mut store, table_type); + linker.define("env", "table", table)?; + let instance = linker + .instantiate(&mut store, &module) + .unwrap() + .start(&mut store)?; + Ok((store, instance)) +} + +fn instantiate_from_wat(wat: &str) -> (Store<()>, Instance) { + try_instantiate_from_wat(wat).unwrap() +} + +fn assert_no_duplicates(store: &Store<()>, instance: Instance) { + assert!(instance.get_memory(store, 1).is_none()); + assert!(instance.get_table(store, 1).is_none()); +} + +#[test] +fn test_import_memory_and_table() { + let wat = r#" + (module + (import "env" "memory" (memory 4)) + (import "env" "table" (table 4 funcref)) + )"#; + let (store, instance) = instantiate_from_wat(wat); + assert!(instance.get_memory(&store, 0).is_some()); + assert!(instance.get_table(&store, 0).is_some()); + assert_no_duplicates(&store, instance); +} + +#[test] +fn test_import_memory() { + let wat = r#" + (module + (import "env" "memory" (memory 4)) + )"#; + let (store, instance) = instantiate_from_wat(wat); + assert!(instance.get_memory(&store, 0).is_some()); + assert!(instance.get_table(&store, 0).is_none()); + assert_no_duplicates(&store, instance); +} + +#[test] +fn test_import_table() { + let wat = r#" + (module + (import "env" "table" (table 4 funcref)) + )"#; + let (store, instance) = instantiate_from_wat(wat); + assert!(instance.get_memory(&store, 0).is_none()); + assert!(instance.get_table(&store, 0).is_some()); + assert_no_duplicates(&store, instance); +} + +#[test] +fn test_no_memory_no_table() { + let wat = "(module)"; + let (store, instance) = instantiate_from_wat(wat); + assert!(instance.get_memory(&store, 0).is_none()); + assert!(instance.get_table(&store, 0).is_none()); + assert_no_duplicates(&store, instance); +} + +#[test] +fn test_internal_memory() { + let wat = "(module (memory 1 10) )"; + let (store, instance) = instantiate_from_wat(wat); + assert!(instance.get_memory(&store, 0).is_some()); + assert!(instance.get_table(&store, 0).is_none()); + assert_no_duplicates(&store, instance); +} + +#[test] +fn test_internal_table() { + let wat = "(module (table 4 funcref) )"; + let (store, instance) = instantiate_from_wat(wat); + assert!(instance.get_memory(&store, 0).is_none()); + assert!(instance.get_table(&store, 0).is_some()); + assert_no_duplicates(&store, instance); +} diff --git a/crates/wasmi/src/module/mod.rs b/crates/wasmi/src/module/mod.rs index d4ffd8589e..b214e45283 100644 --- a/crates/wasmi/src/module/mod.rs +++ b/crates/wasmi/src/module/mod.rs @@ -101,6 +101,14 @@ pub struct ModuleImports { len_funcs: usize, /// The amount of imported [`Global`]. len_globals: usize, + /// The amount of imported [`Memory`]. + /// + /// [`Memory`]: [`crate::Memory`] + len_memories: usize, + /// The amount of imported [`Table`]. + /// + /// [`Table`]: [`crate::Table`] + len_tables: usize, } impl ModuleImports { @@ -108,6 +116,8 @@ impl ModuleImports { fn from_builder(imports: builder::ModuleImports) -> Self { let len_funcs = imports.funcs.len(); let len_globals = imports.globals.len(); + let len_memories = imports.memories.len(); + let len_tables = imports.tables.len(); let funcs = imports.funcs.into_iter().map(Imported::Func); let tables = imports.tables.into_iter().map(Imported::Table); let memories = imports.memories.into_iter().map(Imported::Memory); @@ -122,6 +132,8 @@ impl ModuleImports { items, len_funcs, len_globals, + len_memories, + len_tables, } } } @@ -208,6 +220,26 @@ impl Module { } } + /// Returns an iterator over the [`MemoryType`] of internal linear memories. + fn internal_memories(&self) -> SliceIter { + let len_imported = self.imports.len_memories; + // We skip the first `len_imported` elements in `memories` + // since they refer to imported and not internally defined + // linear memories. + let memories = &self.memories[len_imported..]; + memories.iter() + } + + /// Returns an iterator over the [`TableType`] of internal tables. + fn internal_tables(&self) -> SliceIter { + let len_imported = self.imports.len_tables; + // We skip the first `len_imported` elements in `memories` + // since they refer to imported and not internally defined + // linear memories. + let tables = &self.tables[len_imported..]; + tables.iter() + } + /// Returns an iterator over the internally defined [`Global`]. fn internal_globals(&self) -> InternalGlobalsIter { let len_imported = self.imports.len_globals;