From 6c0a20d7b83d240c76746406f32973b70f65231e Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 24 May 2023 16:49:38 +0200 Subject: [PATCH 1/8] Fix pin comments --- packages/vm/src/cache.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/vm/src/cache.rs b/packages/vm/src/cache.rs index 2e53663063..19ca83a1e6 100644 --- a/packages/vm/src/cache.rs +++ b/packages/vm/src/cache.rs @@ -243,10 +243,12 @@ where /// Pins a Module that was previously stored via save_wasm. /// - /// The module is lookup first in the memory cache, and then in the file system cache. - /// If not found, the code is loaded from the file system, compiled, and stored into the + /// The module is lookup first in the file system cache. If not found, + /// the code is loaded from the file system, compiled, and stored into the /// pinned cache. - /// If the given ID is not found, or the content does not match the hash (=ID), an error is returned. + /// + /// If the given contract for the given checksum is not found, or the content + /// does not match the checksum, an error is returned. pub fn pin(&self, checksum: &Checksum) -> VmResult<()> { let mut cache = self.inner.lock().unwrap(); if cache.pinned_memory_cache.has(checksum) { @@ -255,7 +257,7 @@ where // We don't load from the memory cache because we had to create new store here and // serialize/deserialize the artifact to get a full clone. Could be done but adds some code - // for a no-so-relevant use case. + // for a not-so-relevant use case. // Try to get module from file system cache let engine = Engine::headless(); From a5db6dabdfe60a45618f9f61a19b2a7365722262 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 24 May 2023 16:49:49 +0200 Subject: [PATCH 2/8] Remove unnecessary type --- packages/vm/src/cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/vm/src/cache.rs b/packages/vm/src/cache.rs index 19ca83a1e6..1206dbe5e6 100644 --- a/packages/vm/src/cache.rs +++ b/packages/vm/src/cache.rs @@ -732,7 +732,7 @@ mod tests { let backend5 = mock_backend(&[]); // from file system - let _instance1: Instance = cache + let _instance1 = cache .get_instance(&checksum, backend1, TESTING_OPTIONS) .unwrap(); assert_eq!(cache.stats().hits_pinned_memory_cache, 0); From 83c3254551e8f5f13046ce900a4e11125eb7ab6d Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 24 May 2023 16:50:07 +0200 Subject: [PATCH 3/8] Rename module to cached_module --- packages/vm/src/modules/{sized_module.rs => cached_module.rs} | 0 packages/vm/src/modules/in_memory_cache.rs | 2 +- packages/vm/src/modules/mod.rs | 4 ++-- packages/vm/src/modules/pinned_memory_cache.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) rename packages/vm/src/modules/{sized_module.rs => cached_module.rs} (100%) diff --git a/packages/vm/src/modules/sized_module.rs b/packages/vm/src/modules/cached_module.rs similarity index 100% rename from packages/vm/src/modules/sized_module.rs rename to packages/vm/src/modules/cached_module.rs diff --git a/packages/vm/src/modules/in_memory_cache.rs b/packages/vm/src/modules/in_memory_cache.rs index ce43eeea5f..ca8bc6a579 100644 --- a/packages/vm/src/modules/in_memory_cache.rs +++ b/packages/vm/src/modules/in_memory_cache.rs @@ -3,7 +3,7 @@ use std::collections::hash_map::RandomState; use std::num::NonZeroUsize; use wasmer::{Engine, Module}; -use super::sized_module::CachedModule; +use super::cached_module::CachedModule; use crate::{Checksum, Size, VmError, VmResult}; // Minimum module size. diff --git a/packages/vm/src/modules/mod.rs b/packages/vm/src/modules/mod.rs index b91f72cadc..c871963e87 100644 --- a/packages/vm/src/modules/mod.rs +++ b/packages/vm/src/modules/mod.rs @@ -1,11 +1,11 @@ +mod cached_module; mod file_system_cache; mod in_memory_cache; mod pinned_memory_cache; -mod sized_module; mod versioning; +pub use cached_module::CachedModule; pub use file_system_cache::{FileSystemCache, NewFileSystemCacheError}; pub use in_memory_cache::InMemoryCache; pub use pinned_memory_cache::PinnedMemoryCache; -pub use sized_module::CachedModule; pub use versioning::current_wasmer_module_version; diff --git a/packages/vm/src/modules/pinned_memory_cache.rs b/packages/vm/src/modules/pinned_memory_cache.rs index 61ae460e1e..bb4b72c04e 100644 --- a/packages/vm/src/modules/pinned_memory_cache.rs +++ b/packages/vm/src/modules/pinned_memory_cache.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use wasmer::{Engine, Module}; -use super::sized_module::CachedModule; +use super::cached_module::CachedModule; use crate::{Checksum, VmResult}; /// An pinned in memory module cache From 1de705aedb65ab339a630814a86e87635c46163f Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 24 May 2023 16:52:11 +0200 Subject: [PATCH 4/8] Dedupe code in make_engine --- packages/vm/src/wasm_backend/store.rs | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/packages/vm/src/wasm_backend/store.rs b/packages/vm/src/wasm_backend/store.rs index 05f3e5e408..5c9fad45d9 100644 --- a/packages/vm/src/wasm_backend/store.rs +++ b/packages/vm/src/wasm_backend/store.rs @@ -37,26 +37,17 @@ pub fn make_engine(middlewares: &[Arc]) -> Engine { let metering = Arc::new(Metering::new(gas_limit, cost)); #[cfg(feature = "cranelift")] - { - let mut compiler = Cranelift::default(); - for middleware in middlewares { - compiler.push_middleware(middleware.clone()); - } - compiler.push_middleware(deterministic); - compiler.push_middleware(metering); - compiler.into() - } + let mut compiler = Cranelift::default(); #[cfg(not(feature = "cranelift"))] - { - let mut compiler = Singlepass::default(); - for middleware in middlewares { - compiler.push_middleware(middleware.clone()); - } - compiler.push_middleware(deterministic); - compiler.push_middleware(metering); - compiler.into() + let mut compiler = Singlepass::default(); + + for middleware in middlewares { + compiler.push_middleware(middleware.clone()); } + compiler.push_middleware(deterministic); + compiler.push_middleware(metering); + compiler.into() } /// Created a store with no compiler and the given memory limit (in bytes) From 95a9c321cbdf46593a996490d6ad3ad4f9a0734e Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 24 May 2023 16:53:10 +0200 Subject: [PATCH 5/8] Add CHANGELOG entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 389e6290f6..7380f7fb7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ and this project adheres to - cosmwasm-vm: Add `Instance::set_debug_handler`/`unset_debug_handler` to allow customizing the handling of debug messages emitted by the contract ([#1667]). - cosmwasm-vm: Add `.wasm` extension to stored wasm files ([#1686]). +- cosmwasm-vm: Upgrade Wasmer to version 3.3.0. - cosmwasm-check: Update clap dependency to version 4 ([#1677]) [#1511]: https://github.com/CosmWasm/cosmwasm/issues/1511 From adcfdb0bae2b194610b3e205738f507119556fba Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 25 May 2023 10:22:39 +0200 Subject: [PATCH 6/8] Polish get_region/set_region code --- packages/vm/src/memory.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/packages/vm/src/memory.rs b/packages/vm/src/memory.rs index 16a25b8792..3b1c97fcfe 100644 --- a/packages/vm/src/memory.rs +++ b/packages/vm/src/memory.rs @@ -95,12 +95,11 @@ pub fn write_region(memory: &wasmer::MemoryView, ptr: u32, data: &[u8]) -> VmRes Ok(()) } -/// Reads in a Region at ptr in wasm memory and returns a copy of it -fn get_region(memory: &wasmer::MemoryView, ptr: u32) -> CommunicationResult { - let wptr = WasmPtr::::new(ptr); - // TODO: double check error handling +/// Reads in a Region at offset in Wasm memory and returns a copy of it +fn get_region(memory: &wasmer::MemoryView, offset: u32) -> CommunicationResult { + let wptr = WasmPtr::::new(offset); let region = wptr.deref(memory).read().map_err(|_err| { - CommunicationError::deref_err(ptr, "Could not dereference this pointer to a Region") + CommunicationError::deref_err(offset, "Could not dereference this pointer to a Region") })?; validate_region(®ion)?; Ok(region) @@ -127,13 +126,11 @@ fn validate_region(region: &Region) -> RegionValidationResult<()> { Ok(()) } -/// Overrides a Region at ptr in wasm memory with data -fn set_region(memory: &wasmer::MemoryView, ptr: u32, data: Region) -> CommunicationResult<()> { - let wptr = WasmPtr::::new(ptr); - - // TODO: double check error handling +/// Overrides a Region at offset in Wasm memory +fn set_region(memory: &wasmer::MemoryView, offset: u32, data: Region) -> CommunicationResult<()> { + let wptr = WasmPtr::::new(offset); wptr.deref(memory).write(data).map_err(|_err| { - CommunicationError::deref_err(ptr, "Could not dereference this pointer to a Region") + CommunicationError::deref_err(offset, "Could not dereference this pointer to a Region") })?; Ok(()) } From 2f1ac40e18f14ddf95f8b8dea4fdcdb8f7361885 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 25 May 2023 10:24:16 +0200 Subject: [PATCH 7/8] Add RegionAccessErr --- packages/vm/src/errors/communication_error.rs | 14 +++++++++++++ packages/vm/src/memory.rs | 20 +++++++------------ 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/packages/vm/src/errors/communication_error.rs b/packages/vm/src/errors/communication_error.rs index a5fb88e0ee..263980efba 100644 --- a/packages/vm/src/errors/communication_error.rs +++ b/packages/vm/src/errors/communication_error.rs @@ -2,6 +2,7 @@ use std::fmt::Debug; use thiserror::Error; use super::region_validation_error::RegionValidationError; +use crate::memory::Region; /// An error in the communcation between contract and host. Those happen around imports and exports. #[derive(Error, Debug)] @@ -32,6 +33,12 @@ pub enum CommunicationError { RegionLengthTooBig { length: usize, max_length: usize }, #[error("Region too small. Got {}, required {}", size, required)] RegionTooSmall { size: usize, required: usize }, + #[error("Tried to access memory of region {:?} in Wasm memory of size {} bytes. This typically happens when the given Region pointer does not point to a proper Region struct.", region, memory_size)] + RegionAccessErr { + region: Region, + /// Current size of the linear memory in bytes + memory_size: usize, + }, #[error("Got a zero Wasm address")] ZeroAddress {}, } @@ -64,6 +71,13 @@ impl CommunicationError { CommunicationError::RegionTooSmall { size, required } } + pub(crate) fn region_access_err(region: Region, memory_size: usize) -> Self { + CommunicationError::RegionAccessErr { + region, + memory_size, + } + } + pub(crate) fn zero_address() -> Self { CommunicationError::ZeroAddress {} } diff --git a/packages/vm/src/memory.rs b/packages/vm/src/memory.rs index 3b1c97fcfe..04aa6f054e 100644 --- a/packages/vm/src/memory.rs +++ b/packages/vm/src/memory.rs @@ -17,7 +17,7 @@ use crate::errors::{ /// but defined here to allow Wasmer specific implementation. #[repr(C)] #[derive(Default, Clone, Copy, Debug)] -struct Region { +pub struct Region { /// The beginning of the region expressed as bytes from the beginning of the linear memory pub offset: u32, /// The number of bytes available in this region @@ -47,12 +47,9 @@ pub fn read_region(memory: &wasmer::MemoryView, ptr: u32, max_length: usize) -> } let mut result = vec![0u8; region.length as usize]; - // TODO: double check error handling - memory.read(region.offset as u64, &mut result).map_err(|_err| CommunicationError::deref_err(region.offset, format!( - "Tried to access memory of region {:?} in wasm memory of size {} bytes. This typically happens when the given Region pointer does not point to a proper Region struct.", - region, - memory.size().bytes().0 - )))?; + memory + .read(region.offset as u64, &mut result) + .map_err(|_err| CommunicationError::region_access_err(region, memory.size().bytes().0))?; Ok(result) } @@ -82,12 +79,9 @@ pub fn write_region(memory: &wasmer::MemoryView, ptr: u32, data: &[u8]) -> VmRes return Err(CommunicationError::region_too_small(region_capacity, data.len()).into()); } - // TODO: double check error handling - memory.write(region.offset as u64, data).map_err(|_err| CommunicationError::deref_err(region.offset, format!( - "Tried to access memory of region {:?} in wasm memory of size {} bytes. This typically happens when the given Region pointer does not point to a proper Region struct.", - region, - memory.size().bytes().0 - )))?; + memory + .write(region.offset as u64, data) + .map_err(|_err| CommunicationError::region_access_err(region, memory.size().bytes().0))?; region.length = data.len() as u32; set_region(memory, ptr, region)?; From 39a83d969f03e142d2107b3e96104f4b24a19480 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 25 May 2023 10:36:49 +0200 Subject: [PATCH 8/8] Test calling extra imports --- packages/vm/src/instance.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/vm/src/instance.rs b/packages/vm/src/instance.rs index b8bf87da9f..0b176a387c 100644 --- a/packages/vm/src/instance.rs +++ b/packages/vm/src/instance.rs @@ -602,6 +602,8 @@ mod tests { let (engine, module) = compile(&wasm, &[]).unwrap(); let mut store = make_store_with_engine(engine, memory_limit); + let called = Arc::new(AtomicBool::new(false)); + #[derive(Clone)] struct MyEnv { // This can be mutated across threads safely. We initialize it as `false` @@ -609,11 +611,12 @@ mod tests { called: Arc, } - let my_env = MyEnv { - called: Arc::new(AtomicBool::new(false)), - }; - - let fe = FunctionEnv::new(&mut store, my_env); + let fe = FunctionEnv::new( + &mut store, + MyEnv { + called: called.clone(), + }, + ); let fun = Function::new_typed_with_env(&mut store, &fe, move |fe_mut: FunctionEnvMut| { @@ -636,8 +639,7 @@ mod tests { instance.call_function0("main", &[]).unwrap(); - // TODO: allow inspecting env - // assert!(fe.as_ref(&store).called.load(Ordering::Relaxed)); + assert!(called.load(Ordering::Relaxed)); } #[test]