diff --git a/piecrust/CHANGELOG.md b/piecrust/CHANGELOG.md index f88fff6f..52c5737b 100644 --- a/piecrust/CHANGELOG.md +++ b/piecrust/CHANGELOG.md @@ -13,10 +13,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Change to have one instance per contract function call [#325] - Upgrade `dusk-wasmtime` to version `17` ### Fixed +- Fix bad dereferences in caused by instance reclamation [#325] - Fix overflow in gas limit calculation in inter-contract call ## [0.15.0] - 2024-01-24 @@ -357,6 +359,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#234]: https://github.com/dusk-network/piecrust/pull/234 +[#325]: https://github.com/dusk-network/piecrust/issues/325 [#301]: https://github.com/dusk-network/piecrust/issues/313 [#301]: https://github.com/dusk-network/piecrust/issues/301 [#296]: https://github.com/dusk-network/piecrust/issues/296 diff --git a/piecrust/src/call_tree.rs b/piecrust/src/call_tree.rs index 669a4c3e..1f6c77d6 100644 --- a/piecrust/src/call_tree.rs +++ b/piecrust/src/call_tree.rs @@ -9,13 +9,16 @@ use std::mem; use piecrust_uplink::ContractId; +use crate::instance::WrappedInstance; + /// An element of the call tree. -#[derive(Debug, Clone, Copy)] +#[derive(Clone)] pub struct CallTreeElem { pub contract_id: ContractId, pub limit: u64, pub spent: u64, pub mem_len: usize, + pub instance: WrappedInstance, } /// The tree of contract calls. @@ -47,7 +50,7 @@ impl CallTree { pub(crate) fn move_up(&mut self, spent: u64) -> Option { self.0.map(|inner| unsafe { (*inner).elem.spent = spent; - let elem = (*inner).elem; + let elem = (*inner).elem.clone(); let parent = (*inner).parent; if parent.is_none() { @@ -63,7 +66,7 @@ impl CallTree { /// current element. pub(crate) fn move_up_prune(&mut self) -> Option { self.0.map(|inner| unsafe { - let elem = (*inner).elem; + let elem = (*inner).elem.clone(); let parent = (*inner).parent; if let Some(parent) = parent { @@ -98,7 +101,7 @@ impl CallTree { i += 1; } - current.map(|inner| unsafe { (*inner).elem }) + current.map(|inner| unsafe { (*inner).elem.clone() }) } /// Clears the call tree of all elements. diff --git a/piecrust/src/imports.rs b/piecrust/src/imports.rs index 51d6896c..36d350a9 100644 --- a/piecrust/src/imports.rs +++ b/piecrust/src/imports.rs @@ -138,12 +138,12 @@ pub(crate) fn hq( ) -> WasmtimeResult { let env = fenv.data_mut(); - let instance = env.self_instance(); + let mut instance = env.self_instance(); let name_len = name_len as usize; - check_ptr(instance, name_ofs, name_len)?; - check_arg(instance, arg_len)?; + check_ptr(&instance, name_ofs, name_len)?; + check_arg(&instance, arg_len)?; let name = instance.with_memory(|buf| { // performance: use a dedicated buffer here? @@ -163,11 +163,11 @@ pub(crate) fn hd( ) -> WasmtimeResult { let env = fenv.data_mut(); - let instance = env.self_instance(); + let mut instance = env.self_instance(); let name_len = name_len as usize; - check_ptr(instance, name_ofs, name_len)?; + check_ptr(&instance, name_ofs, name_len)?; let name = instance.with_memory(|buf| { // performance: use a dedicated buffer here? @@ -194,13 +194,13 @@ pub(crate) fn c( ) -> WasmtimeResult { let env = fenv.data_mut(); - let instance = env.self_instance(); + let mut instance = env.self_instance(); let name_len = name_len as usize; - check_ptr(instance, mod_id_ofs, CONTRACT_ID_BYTES)?; - check_ptr(instance, name_ofs, name_len)?; - check_arg(instance, arg_len)?; + check_ptr(&instance, mod_id_ofs, CONTRACT_ID_BYTES)?; + check_ptr(&instance, name_ofs, name_len)?; + check_arg(&instance, arg_len)?; let argbuf_ofs = instance.arg_buffer_offset(); @@ -225,9 +225,7 @@ pub(crate) fn c( let callee_stack_element = env .push_callstack(mod_id, callee_limit) .expect("pushing to the callstack should succeed"); - let callee = env - .instance(&callee_stack_element.contract_id) - .expect("callee instance should exist"); + let mut callee = callee_stack_element.instance; callee.snap().map_err(|err| Error::MemorySnapshotFailure { reason: None, @@ -242,7 +240,7 @@ pub(crate) fn c( let ret_len = callee .call(name, arg.len() as u32, callee_limit) .map_err(Error::normalize)?; - check_arg(callee, ret_len as u32)?; + check_arg(&callee, ret_len as u32)?; // copy back result callee.read_argument(&mut memory[argbuf_ofs..][..ret_len as usize]); @@ -291,8 +289,8 @@ pub(crate) fn emit( let topic_len = topic_len as usize; - check_ptr(instance, topic_ofs, topic_len)?; - check_arg(instance, arg_len)?; + check_ptr(&instance, topic_ofs, topic_len)?; + check_arg(&instance, arg_len)?; let data = instance.with_arg_buf(|buf| { let arg_len = arg_len as usize; @@ -327,7 +325,7 @@ fn feed(mut fenv: Caller, arg_len: u32) -> WasmtimeResult<()> { let env = fenv.data_mut(); let instance = env.self_instance(); - check_arg(instance, arg_len)?; + check_arg(&instance, arg_len)?; let data = instance.with_arg_buf(|buf| { let arg_len = arg_len as usize; @@ -342,7 +340,7 @@ fn hdebug(mut fenv: Caller, msg_len: u32) -> WasmtimeResult<()> { let env = fenv.data_mut(); let instance = env.self_instance(); - check_arg(instance, msg_len)?; + check_arg(&instance, msg_len)?; Ok(instance.with_arg_buf(|buf| { let slice = &buf[..msg_len as usize]; @@ -365,7 +363,7 @@ fn limit(fenv: Caller) -> u64 { fn spent(fenv: Caller) -> u64 { let env = fenv.data(); - let instance = env.self_instance(); + let mut instance = env.self_instance(); let limit = env.limit(); let remaining = instance.get_remaining_gas(); @@ -377,7 +375,7 @@ fn panic(fenv: Caller, arg_len: u32) -> WasmtimeResult<()> { let env = fenv.data(); let instance = env.self_instance(); - check_arg(instance, arg_len)?; + check_arg(&instance, arg_len)?; Ok(instance.with_arg_buf(|buf| { let slice = &buf[..arg_len as usize]; @@ -392,7 +390,7 @@ fn panic(fenv: Caller, arg_len: u32) -> WasmtimeResult<()> { } fn owner(fenv: Caller, mod_id_ofs: usize) -> WasmtimeResult { - check_ptr(fenv.data().self_instance(), mod_id_ofs, CONTRACT_ID_BYTES)?; + check_ptr(&fenv.data().self_instance(), mod_id_ofs, CONTRACT_ID_BYTES)?; let env = fenv.data(); diff --git a/piecrust/src/instance.rs b/piecrust/src/instance.rs index 8b60aa60..0f6df9c3 100644 --- a/piecrust/src/instance.rs +++ b/piecrust/src/instance.rs @@ -5,7 +5,9 @@ // Copyright (c) DUSK NETWORK. All rights reserved. use std::io; +use std::mem::MaybeUninit; use std::ops::{Deref, DerefMut}; +use std::sync::atomic::{AtomicUsize, Ordering}; use dusk_wasmtime::{Instance, Module, Mutability, Store, ValType}; use piecrust_uplink::{ContractId, Event, ARGBUF_LEN}; @@ -16,16 +18,45 @@ use crate::session::Session; use crate::store::Memory; use crate::Error; -pub struct WrappedInstance { - instance: Instance, - arg_buf_ofs: usize, - store: Store, - memory: Memory, +pub struct WrappedInstance(*mut WrappedInstanceInner); + +impl Clone for WrappedInstance { + fn clone(&self) -> Self { + unsafe { + (*self.0).count.fetch_add(1, Ordering::SeqCst); + Self(self.0) + } + } +} + +impl Drop for WrappedInstance { + fn drop(&mut self) { + if self.count.fetch_sub(1, Ordering::SeqCst) == 1 { + unsafe { + let _ = Box::from_raw(self.0); + } + } + } +} + +impl Deref for WrappedInstance { + type Target = WrappedInstanceInner; + + fn deref(&self) -> &Self::Target { + unsafe { &*self.0 } + } +} + +impl DerefMut for WrappedInstance { + fn deref_mut(&mut self) -> &mut Self::Target { + unsafe { &mut *self.0 } + } } pub(crate) struct Env { self_id: ContractId, session: Session, + instance: MaybeUninit, } impl Deref for Env { @@ -43,20 +74,8 @@ impl DerefMut for Env { } impl Env { - pub fn self_instance<'b>(&self) -> &'b mut WrappedInstance { - let stack_element = self - .session - .nth_from_top(0) - .expect("there should be at least one element in the call stack"); - self.instance(&stack_element.contract_id) - .expect("instance should exist") - } - - pub fn instance<'b>( - &self, - contract_id: &ContractId, - ) -> Option<&'b mut WrappedInstance> { - self.session.instance(contract_id) + pub fn self_instance(&self) -> WrappedInstance { + unsafe { self.instance.assume_init_ref().clone() } } pub fn limit(&self) -> u64 { @@ -94,6 +113,7 @@ impl WrappedInstance { let env = Env { self_id: contract_id, session, + instance: MaybeUninit::uninit(), }; let module = @@ -177,16 +197,32 @@ impl WrappedInstance { // A memory is no longer new after one instantiation memory.is_new = false; - let wrapped = WrappedInstance { + let inner = WrappedInstanceInner { store, instance, arg_buf_ofs, memory, + count: AtomicUsize::new(1), }; - Ok(wrapped) + let mut instance = WrappedInstance(Box::into_raw(Box::new(inner))); + let instance_clone = instance.clone(); + + instance.store.data_mut().instance = MaybeUninit::new(instance_clone); + + Ok(instance) } +} + +pub struct WrappedInstanceInner { + instance: Instance, + arg_buf_ofs: usize, + store: Store, + memory: Memory, + count: AtomicUsize, +} +impl WrappedInstanceInner { pub(crate) fn snap(&mut self) -> io::Result<()> { self.memory.snap()?; Ok(()) @@ -342,7 +378,7 @@ impl WrappedInstance { } fn map_call_err( - instance: &mut WrappedInstance, + instance: &mut WrappedInstanceInner, err: dusk_wasmtime::Error, ) -> Error { if instance.get_remaining_gas() == 0 { diff --git a/piecrust/src/session.rs b/piecrust/src/session.rs index 66970b58..e0946cbf 100644 --- a/piecrust/src/session.rs +++ b/piecrust/src/session.rs @@ -69,7 +69,7 @@ impl Drop for Session { if self.original { // ensure the stack is cleared and all instances are removed and // reclaimed on the drop of a session. - self.clear_stack_and_instances(); + self.clear_stack(); // SAFETY: this is safe since we guarantee that there is no aliasing // when a session drops. @@ -85,7 +85,6 @@ struct SessionInner { current: ContractId, call_tree: CallTree, - instances: BTreeMap, debug: Vec, data: SessionData, @@ -139,7 +138,6 @@ impl Session { let inner = SessionInner { current: ContractId::uninitialized(), call_tree: CallTree::new(), - instances: BTreeMap::new(), debug: vec![], data, contract_session, @@ -285,9 +283,9 @@ impl Session { .map_err(|err| PersistenceError(Arc::new(err)))?; let instantiate = || { - self.create_instance(contract_id)?; - let instance = - self.instance(&contract_id).expect("instance should exist"); + let mut stack_element = + self.push_callstack(contract_id, gas_limit)?; + let instance = &mut stack_element.instance; let has_init = instance.is_function_exported(INIT_METHOD); if has_init && arg.is_none() { @@ -499,26 +497,8 @@ impl Session { .map(|data| data.memory.current_len)) } - pub(crate) fn instance<'a>( - &self, - contract_id: &ContractId, - ) -> Option<&'a mut WrappedInstance> { - self.inner.instances.get(contract_id).map(|instance| { - // SAFETY: We guarantee that the instance exists since we're in - // control over if it is dropped with the session. - unsafe { &mut **instance } - }) - } - - fn clear_stack_and_instances(&mut self) { + fn clear_stack(&mut self) { self.inner.call_tree.clear(); - - while !self.inner.instances.is_empty() { - let (_, instance) = self.inner.instances.pop_first().unwrap(); - unsafe { - let _ = Box::from_raw(instance); - }; - } } /// Return the state root of the current state of the session. @@ -556,10 +536,24 @@ impl Session { feed.send(data).map_err(Error::FeedPulled) } - fn new_instance( + pub(crate) fn host_query( + &self, + name: &str, + buf: &mut [u8], + arg_len: u32, + ) -> Option { + self.inner.host_queries.call(name, buf, arg_len) + } + + pub(crate) fn nth_from_top(&self, n: usize) -> Option { + self.inner.call_tree.nth_parent(n) + } + + pub(crate) fn push_callstack( &mut self, contract_id: ContractId, - ) -> Result { + limit: u64, + ) -> Result { let store_data = self .inner .contract_session @@ -574,76 +568,22 @@ impl Session { )?; self.inner.current = contract_id; + let memory = store_data.memory; let instance = WrappedInstance::new( self.clone(), contract_id, &contract, - store_data.memory, + memory.clone(), )?; - Ok(instance) - } - - pub(crate) fn host_query( - &self, - name: &str, - buf: &mut [u8], - arg_len: u32, - ) -> Option { - self.inner.host_queries.call(name, buf, arg_len) - } - - pub(crate) fn nth_from_top(&self, n: usize) -> Option { - self.inner.call_tree.nth_parent(n) - } - - /// Creates a new instance of the given contract, returning its memory - /// length. - fn create_instance( - &mut self, - contract: ContractId, - ) -> Result { - let instance = self.new_instance(contract)?; - if self.inner.instances.get(&contract).is_some() { - panic!("Contract already in the stack: {contract:?}"); - } - - let mem_len = instance.mem_len(); - - let instance = Box::new(instance); - let instance = Box::leak(instance) as *mut WrappedInstance; - - self.inner.instances.insert(contract, instance); - Ok(mem_len) - } - - pub(crate) fn push_callstack( - &mut self, - contract_id: ContractId, - limit: u64, - ) -> Result { - let instance = self.instance(&contract_id); - - match instance { - Some(instance) => { - self.inner.call_tree.push(CallTreeElem { - contract_id, - limit, - spent: 0, - mem_len: instance.mem_len(), - }); - } - None => { - let mem_len = self.create_instance(contract_id)?; - self.inner.call_tree.push(CallTreeElem { - contract_id, - limit, - spent: 0, - mem_len, - }); - } - } + self.inner.call_tree.push(CallTreeElem { + contract_id, + limit, + spent: 0, + mem_len: instance.mem_len(), + instance, + }); Ok(self .inner @@ -662,11 +602,8 @@ impl Session { pub(crate) fn revert_callstack(&mut self) -> Result<(), std::io::Error> { for elem in self.inner.call_tree.iter() { - let instance = self - .instance(&elem.contract_id) - .expect("instance should exist"); - instance.revert()?; - instance.set_len(elem.mem_len); + elem.instance.clone().revert()?; + elem.instance.clone().set_len(elem.mem_len); } Ok(()) @@ -725,10 +662,8 @@ impl Session { fdata: Vec, limit: u64, ) -> Result<(Vec, u64, CallTree), Error> { - let stack_element = self.push_callstack(contract, limit)?; - let instance = self - .instance(&stack_element.contract_id) - .expect("instance should exist"); + let mut stack_element = self.push_callstack(contract, limit)?; + let instance = &mut stack_element.instance; instance .snap() @@ -748,7 +683,7 @@ impl Session { }; } self.move_up_prune_call_tree(); - self.clear_stack_and_instances(); + self.clear_stack(); err }) .map_err(Error::normalize)?; @@ -757,17 +692,14 @@ impl Session { let spent = limit - instance.get_remaining_gas(); for elem in self.inner.call_tree.iter() { - let instance = self - .instance(&elem.contract_id) - .expect("instance should exist"); - instance - .apply() - .map_err(|err| Error::MemorySnapshotFailure { + elem.instance.clone().apply().map_err(|err| { + Error::MemorySnapshotFailure { reason: None, io: Arc::new(err), - })?; + } + })?; } - self.clear_stack_and_instances(); + self.clear_stack(); let mut call_tree = CallTree::new(); mem::swap(&mut self.inner.call_tree, &mut call_tree);