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/imports.rs b/piecrust/src/imports.rs index 51d6896c..6e8917e3 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(); @@ -222,12 +222,8 @@ pub(crate) fn c( &memory[mod_id_ofs..][..std::mem::size_of::()], ); - 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 = env.instance(mod_id)?; + env.push_callstack(mod_id, callee_limit, callee.mem_len()); callee.snap().map_err(|err| Error::MemorySnapshotFailure { reason: None, @@ -242,7 +238,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 +287,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 +323,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 +338,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 +361,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 +373,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 +388,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..e9369053 100644 --- a/piecrust/src/instance.rs +++ b/piecrust/src/instance.rs @@ -5,6 +5,7 @@ // Copyright (c) DUSK NETWORK. All rights reserved. use std::io; +use std::mem::MaybeUninit; use std::ops::{Deref, DerefMut}; use dusk_wasmtime::{Instance, Module, Mutability, Store, ValType}; @@ -17,15 +18,47 @@ use crate::store::Memory; use crate::Error; pub struct WrappedInstance { - instance: Instance, - arg_buf_ofs: usize, - store: Store, - memory: Memory, + inner: *mut WrappedInstanceInner, + original: bool, +} + +impl Clone for WrappedInstance { + fn clone(&self) -> Self { + Self { + inner: self.inner, + original: false, + } + } +} + +impl Drop for WrappedInstance { + fn drop(&mut self) { + if self.original { + unsafe { + let _ = Box::from_raw(self.inner); + } + } + } +} + +impl Deref for WrappedInstance { + type Target = WrappedInstanceInner; + + fn deref(&self) -> &Self::Target { + unsafe { &*self.inner } + } +} + +impl DerefMut for WrappedInstance { + fn deref_mut(&mut self) -> &mut Self::Target { + unsafe { &mut *self.inner } + } } pub(crate) struct Env { self_id: ContractId, session: Session, + instance: MaybeUninit, } impl Deref for Env { @@ -43,20 +76,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 +115,7 @@ impl WrappedInstance { let env = Env { self_id: contract_id, session, + instance: MaybeUninit::uninit(), }; let module = @@ -177,28 +199,35 @@ 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, }; - Ok(wrapped) - } + let mut instance = WrappedInstance { + inner: Box::into_raw(Box::new(inner)), + original: true, + }; + let instance_clone = instance.clone(); - pub(crate) fn snap(&mut self) -> io::Result<()> { - self.memory.snap()?; - Ok(()) - } + instance.store.data_mut().instance = MaybeUninit::new(instance_clone); - pub(crate) fn revert(&mut self) -> io::Result<()> { - self.memory.revert()?; - Ok(()) + Ok(instance) } +} - pub(crate) fn apply(&mut self) -> io::Result<()> { - self.memory.apply()?; +pub struct WrappedInstanceInner { + instance: Instance, + arg_buf_ofs: usize, + store: Store, + memory: Memory, +} + +impl WrappedInstanceInner { + pub(crate) fn snap(&mut self) -> io::Result<()> { + self.memory.snap()?; Ok(()) } @@ -238,11 +267,6 @@ impl WrappedInstance { self.memory.current_len } - /// Sets the length of the memory. - pub(crate) fn set_len(&mut self, len: usize) { - self.memory.current_len = len; - } - pub(crate) fn with_arg_buf(&self, f: F) -> R where F: FnOnce(&[u8]) -> R, @@ -342,7 +366,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..62baf52d 100644 --- a/piecrust/src/session.rs +++ b/piecrust/src/session.rs @@ -7,8 +7,8 @@ use std::borrow::Cow; use std::collections::BTreeMap; use std::fmt::{Debug, Formatter}; -use std::mem; use std::sync::{mpsc, Arc}; +use std::{io, mem}; use bytecheck::CheckBytes; use dusk_wasmtime::{Engine, LinearMemory, MemoryCreator, MemoryType}; @@ -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,7 @@ 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 instance = self.instance(contract_id)?; let has_init = instance.is_function_exported(INIT_METHOD); if has_init && arg.is_none() { @@ -305,7 +301,13 @@ impl Session { )); } - self.call_inner(contract_id, INIT_METHOD, arg, gas_limit)?; + self.call_inner( + contract_id, + INIT_METHOD, + arg, + gas_limit, + &mut instance, + )?; } Ok(()) @@ -383,8 +385,14 @@ impl Session { return Err(InitalizationError("init call not allowed".into())); } - let (data, gas_spent, call_tree) = - self.call_inner(contract, fn_name, fn_arg.into(), gas_limit)?; + let mut instance = self.instance(contract)?; + let (data, gas_spent, call_tree) = self.call_inner( + contract, + fn_name, + fn_arg.into(), + gas_limit, + &mut instance, + )?; let events = mem::take(&mut self.inner.events); Ok(CallReceipt { @@ -499,26 +507,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,7 +546,20 @@ 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 instance( &mut self, contract_id: ContractId, ) -> Result { @@ -574,82 +577,30 @@ 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, - }); - } - } - - Ok(self - .inner - .call_tree - .nth_parent(0) - .expect("We just pushed an element to the stack")) + mem_len: usize, + ) { + self.inner.call_tree.push(CallTreeElem { + contract_id, + limit, + spent: 0, + mem_len, + }); } pub(crate) fn move_up_call_tree(&mut self, spent: u64) { @@ -660,13 +611,17 @@ impl Session { self.inner.call_tree.move_up_prune(); } - pub(crate) fn revert_callstack(&mut self) -> Result<(), std::io::Error> { + pub(crate) fn revert_callstack(&mut self) -> Result<(), 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); + let mut memory = self + .inner + .contract_session + .contract(elem.contract_id)? + .unwrap() + .memory; + + memory.revert()?; + memory.current_len = elem.mem_len; } Ok(()) @@ -724,11 +679,9 @@ impl Session { fname: &str, fdata: Vec, limit: u64, + instance: &mut WrappedInstance, ) -> 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"); + self.push_callstack(contract, limit, instance.mem_len()); instance .snap() @@ -748,7 +701,7 @@ impl Session { }; } self.move_up_prune_call_tree(); - self.clear_stack_and_instances(); + self.clear_stack(); err }) .map_err(Error::normalize)?; @@ -757,17 +710,20 @@ 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 { - reason: None, - io: Arc::new(err), - })?; + let mut memory = self + .inner + .contract_session + .contract(elem.contract_id) + .map_err(|err| Error::PersistenceError(Arc::new(err)))? + .unwrap() + .memory; + + memory.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);