From 86ef98fad6f9bebf69cdd81eca149a2ebcab199e Mon Sep 17 00:00:00 2001 From: Christoph Otter Date: Fri, 12 Jan 2024 17:33:38 +0100 Subject: [PATCH] Use more solid Send and Sync bounds --- packages/std/src/testing/mock.rs | 6 ++++ packages/vm/src/backend.rs | 4 +-- packages/vm/src/environment.rs | 55 ++++++++++++++++++++++-------- packages/vm/src/imports.rs | 2 +- packages/vm/src/instance.rs | 8 ++--- packages/vm/src/testing/querier.rs | 14 ++++++-- 6 files changed, 65 insertions(+), 24 deletions(-) diff --git a/packages/std/src/testing/mock.rs b/packages/std/src/testing/mock.rs index 9f9096082d..56a4d9ab72 100644 --- a/packages/std/src/testing/mock.rs +++ b/packages/std/src/testing/mock.rs @@ -606,6 +606,12 @@ impl Default for WasmQuerier { }; SystemResult::Err(err) }); + + // check that this handler is Send + Sync + // see `cosmwasm_vm::MockQuerier`'s `Send` and `Sync` impls for more details + fn assert_send_sync(_: &(impl Send + Sync)) {} + assert_send_sync(&handler); + Self::new(handler) } } diff --git a/packages/vm/src/backend.rs b/packages/vm/src/backend.rs index b473ecb7e0..a0a92217b0 100644 --- a/packages/vm/src/backend.rs +++ b/packages/vm/src/backend.rs @@ -85,7 +85,7 @@ pub struct Backend { } /// Access to the VM's backend storage, i.e. the chain -pub trait Storage { +pub trait Storage: Send + Sync { /// Returns Err on error. /// Returns Ok(None) when key does not exist. /// Returns Ok(Some(Vec)) when key exists. @@ -171,7 +171,7 @@ pub trait BackendApi: Clone + Send { fn addr_humanize(&self, canonical: &[u8]) -> BackendResult; } -pub trait Querier { +pub trait Querier: Send + Sync { /// This is all that must be implemented for the Querier. /// This allows us to pass through binary queries from one level to another without /// knowing the custom format, or we can decode it, with the knowledge of the allowed diff --git a/packages/vm/src/environment.rs b/packages/vm/src/environment.rs index 23ac343845..153842e608 100644 --- a/packages/vm/src/environment.rs +++ b/packages/vm/src/environment.rs @@ -1,10 +1,8 @@ //! Internal details to be used by instance.rs only use std::borrow::BorrowMut; -use std::cell::RefCell; use std::marker::PhantomData; use std::ptr::NonNull; -use std::rc::Rc; -use std::sync::{Arc, RwLock}; +use std::sync::{Arc, Mutex, RwLock}; use derivative::Derivative; use wasmer::{AsStoreMut, Instance as WasmerInstance, Memory, MemoryView, Value}; @@ -106,7 +104,7 @@ pub struct DebugInfo<'a> { // /- BEGIN TRAIT END TRAIT \ // | | // v v -pub type DebugHandlerFn = dyn for<'a, 'b> FnMut(/* msg */ &'a str, DebugInfo<'b>); +pub type DebugHandlerFn = dyn for<'a, 'b> FnMut(/* msg */ &'a str, DebugInfo<'b>) + Send + Sync; /// A environment that provides access to the ContextData. /// The environment is clonable but clones access the same underlying data. @@ -117,10 +115,6 @@ pub struct Environment { data: Arc>>, } -unsafe impl Send for Environment {} - -unsafe impl Sync for Environment {} - impl Clone for Environment { fn clone(&self) -> Self { Environment { @@ -142,15 +136,15 @@ impl Environment { } } - pub fn set_debug_handler(&self, debug_handler: Option>>) { + pub fn set_debug_handler(&self, debug_handler: Option>>) { self.with_context_data_mut(|context_data| { context_data.debug_handler = debug_handler; }) } - pub fn debug_handler(&self) -> Option>> { + pub fn debug_handler(&self) -> Option>> { self.with_context_data(|context_data| { - // This clone here requires us to wrap the function in Rc instead of Box + // This clone here requires us to wrap the function in Arc instead of Box context_data.debug_handler.clone() }) } @@ -282,7 +276,7 @@ impl Environment { /// Creates a back reference from a contact to its partent instance pub fn set_wasmer_instance(&self, wasmer_instance: Option>) { self.with_context_data_mut(|context_data| { - context_data.wasmer_instance = wasmer_instance; + context_data.wasmer_instance = wasmer_instance.map(WasmerRef); }); } @@ -399,9 +393,42 @@ pub struct ContextData { storage_readonly: bool, call_depth: usize, querier: Option, - debug_handler: Option>>, + debug_handler: Option>>, /// A non-owning link to the wasmer instance - wasmer_instance: Option>, + wasmer_instance: Option, +} + +/// A non-owning link to the wasmer instance +/// +/// This wrapper type allows us to implement `Send` and `Sync`. +#[derive(Clone, Copy)] +struct WasmerRef(NonNull); + +impl WasmerRef { + pub const unsafe fn as_ref<'a>(&self) -> &'a WasmerInstance { + self.0.as_ref() + } +} + +// +unsafe impl Send for WasmerRef {} +unsafe impl Sync for WasmerRef {} + +/// TODO: SAFETY +// unsafe impl Sync for ContextData {} + +/// Implementing `Send` is safe here as long as `WasmerInstance` is Send. +/// This is guaranteed by the function definition below. +// unsafe impl Send for ContextData {} + +#[allow(dead_code)] +fn assert_is_send(_: PhantomData) {} +#[allow(dead_code)] +fn assert_is_sync(_: PhantomData) {} +#[allow(dead_code)] +fn assert_wasmer_instance() { + assert_is_send(PhantomData::); + assert_is_sync(PhantomData::); } impl ContextData { diff --git a/packages/vm/src/imports.rs b/packages/vm/src/imports.rs index a02bd496b8..0ded4dfcd0 100644 --- a/packages/vm/src/imports.rs +++ b/packages/vm/src/imports.rs @@ -436,7 +436,7 @@ pub fn do_debug(&mut self, debug_handler: H) where - H: for<'a, 'b> FnMut(/* msg */ &'a str, DebugInfo<'b>) + 'static, + H: for<'a, 'b> FnMut(/* msg */ &'a str, DebugInfo<'b>) + 'static + Send + Sync, { self.fe .as_ref(&self.store) - .set_debug_handler(Some(Rc::new(RefCell::new(debug_handler)))); + .set_debug_handler(Some(Arc::new(Mutex::new(debug_handler)))); } pub fn unset_debug_handler(&mut self) { diff --git a/packages/vm/src/testing/querier.rs b/packages/vm/src/testing/querier.rs index 046526ed9f..869a75c36d 100644 --- a/packages/vm/src/testing/querier.rs +++ b/packages/vm/src/testing/querier.rs @@ -19,6 +19,14 @@ pub struct MockQuerier { querier: StdMockQuerier, } +// SAFETY: The only thing in `MockQuerier` that is not `Send + Sync` are +// the custom handler and wasm handler. +// The default custom handler does not use the reference it gets, +// so it should be safe to assume it is `Send + Sync`. +// When setting these in `update_wasm` or `with_custom_handler`, we require them to be `Send + Sync`. +unsafe impl Send for MockQuerier {} +unsafe impl Sync for MockQuerier {} + impl MockQuerier { pub fn new(balances: &[(&str, &[Coin])]) -> Self { MockQuerier { @@ -47,14 +55,16 @@ impl MockQuerier { pub fn update_wasm(&mut self, handler: WH) where - WH: Fn(&cosmwasm_std::WasmQuery) -> cosmwasm_std::QuerierResult, + WH: Fn(&cosmwasm_std::WasmQuery) -> cosmwasm_std::QuerierResult + Sync + Send, + // see above for Sync + Send bound explanation { self.querier.update_wasm(handler) } pub fn with_custom_handler(mut self, handler: CH) -> Self where - CH: Fn(&C) -> MockQuerierCustomHandlerResult, + CH: Fn(&C) -> MockQuerierCustomHandlerResult + Sync + Send, + // see above for Sync + Send bound explanation { self.querier = self.querier.with_custom_handler(handler); self