diff --git a/piecrust/CHANGELOG.md b/piecrust/CHANGELOG.md index 99666317..51413628 100644 --- a/piecrust/CHANGELOG.md +++ b/piecrust/CHANGELOG.md @@ -17,12 +17,10 @@ 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 diff --git a/piecrust/src/imports.rs b/piecrust/src/imports.rs index 6e8917e3..51d6896c 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 mut instance = env.self_instance(); + let 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 mut instance = env.self_instance(); + let 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 mut instance = env.self_instance(); + let 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,8 +222,12 @@ pub(crate) fn c( &memory[mod_id_ofs..][..std::mem::size_of::()], ); - let mut callee = env.instance(mod_id)?; - env.push_callstack(mod_id, callee_limit, callee.mem_len()); + 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"); callee.snap().map_err(|err| Error::MemorySnapshotFailure { reason: None, @@ -238,7 +242,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]); @@ -287,8 +291,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; @@ -323,7 +327,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; @@ -338,7 +342,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]; @@ -361,7 +365,7 @@ fn limit(fenv: Caller) -> u64 { fn spent(fenv: Caller) -> u64 { let env = fenv.data(); - let mut instance = env.self_instance(); + let instance = env.self_instance(); let limit = env.limit(); let remaining = instance.get_remaining_gas(); @@ -373,7 +377,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]; @@ -388,7 +392,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 e9369053..8b60aa60 100644 --- a/piecrust/src/instance.rs +++ b/piecrust/src/instance.rs @@ -5,7 +5,6 @@ // 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}; @@ -18,47 +17,15 @@ use crate::store::Memory; use crate::Error; pub struct WrappedInstance { - 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 } - } + instance: Instance, + arg_buf_ofs: usize, + store: Store, + memory: Memory, } pub(crate) struct Env { self_id: ContractId, session: Session, - instance: MaybeUninit, } impl Deref for Env { @@ -76,8 +43,20 @@ impl DerefMut for Env { } impl Env { - pub fn self_instance(&self) -> WrappedInstance { - unsafe { self.instance.assume_init_ref().clone() } + 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 limit(&self) -> u64 { @@ -115,7 +94,6 @@ impl WrappedInstance { let env = Env { self_id: contract_id, session, - instance: MaybeUninit::uninit(), }; let module = @@ -199,38 +177,31 @@ impl WrappedInstance { // A memory is no longer new after one instantiation memory.is_new = false; - let inner = WrappedInstanceInner { + let wrapped = WrappedInstance { store, instance, arg_buf_ofs, memory, }; - let mut instance = WrappedInstance { - inner: Box::into_raw(Box::new(inner)), - original: true, - }; - let instance_clone = instance.clone(); - - instance.store.data_mut().instance = MaybeUninit::new(instance_clone); - - Ok(instance) + Ok(wrapped) } -} -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(()) } + pub(crate) fn revert(&mut self) -> io::Result<()> { + self.memory.revert()?; + Ok(()) + } + + pub(crate) fn apply(&mut self) -> io::Result<()> { + self.memory.apply()?; + Ok(()) + } + // Write argument into instance pub(crate) fn write_argument(&mut self, arg: &[u8]) { self.with_arg_buf_mut(|buf| buf[..arg.len()].copy_from_slice(arg)) @@ -267,6 +238,11 @@ impl WrappedInstanceInner { 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, @@ -366,7 +342,7 @@ impl WrappedInstanceInner { } fn map_call_err( - instance: &mut WrappedInstanceInner, + instance: &mut WrappedInstance, err: dusk_wasmtime::Error, ) -> Error { if instance.get_remaining_gas() == 0 { diff --git a/piecrust/src/session.rs b/piecrust/src/session.rs index fd3cf14f..8090f347 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(); + self.clear_stack_and_instances(); // SAFETY: this is safe since we guarantee that there is no aliasing // when a session drops. @@ -85,6 +85,7 @@ struct SessionInner { current: ContractId, call_tree: CallTree, + instances: BTreeMap, debug: Vec, data: SessionData, @@ -138,6 +139,7 @@ impl Session { let inner = SessionInner { current: ContractId::uninitialized(), call_tree: CallTree::new(), + instances: BTreeMap::new(), debug: vec![], data, contract_session, @@ -283,7 +285,9 @@ impl Session { .map_err(|err| PersistenceError(Arc::new(err)))?; let instantiate = || { - let mut instance = self.instance(contract_id)?; + self.create_instance(contract_id)?; + let instance = + self.instance(&contract_id).expect("instance should exist"); let has_init = instance.is_function_exported(INIT_METHOD); if has_init && arg.is_none() { @@ -301,13 +305,7 @@ impl Session { )); } - self.call_inner( - contract_id, - INIT_METHOD, - arg, - gas_limit, - &mut instance, - )?; + self.call_inner(contract_id, INIT_METHOD, arg, gas_limit)?; } Ok(()) @@ -386,14 +384,8 @@ impl Session { return Err(InitalizationError("init call not allowed".into())); } - 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 (data, gas_spent, call_tree) = + self.call_inner(contract, fn_name, fn_arg.into(), gas_limit)?; let events = mem::take(&mut self.inner.events); Ok(CallReceipt { @@ -509,8 +501,26 @@ impl Session { .map(|data| data.memory.current_len)) } - fn clear_stack(&mut self) { + 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) { 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. @@ -548,20 +558,7 @@ impl Session { feed.send(data).map_err(Error::FeedPulled) } - 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( + fn new_instance( &mut self, contract_id: ContractId, ) -> Result { @@ -579,30 +576,82 @@ impl Session { )?; self.inner.current = contract_id; - let memory = store_data.memory; let instance = WrappedInstance::new( self.clone(), contract_id, &contract, - memory.clone(), + store_data.memory, )?; 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, - mem_len: usize, - ) { - self.inner.call_tree.push(CallTreeElem { - contract_id, - limit, - spent: 0, - mem_len, - }); + ) -> 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")) } pub(crate) fn move_up_call_tree(&mut self, spent: u64) { @@ -613,17 +662,13 @@ impl Session { self.inner.call_tree.move_up_prune(); } - pub(crate) fn revert_callstack(&mut self) -> Result<(), io::Error> { + pub(crate) fn revert_callstack(&mut self) -> Result<(), std::io::Error> { for elem in self.inner.call_tree.iter() { - let mut memory = self - .inner - .contract_session - .contract(elem.contract_id)? - .unwrap() - .memory; - - memory.revert()?; - memory.current_len = elem.mem_len; + let instance = self + .instance(&elem.contract_id) + .expect("instance should exist"); + instance.revert()?; + instance.set_len(elem.mem_len); } Ok(()) @@ -681,9 +726,11 @@ impl Session { fname: &str, fdata: Vec, limit: u64, - instance: &mut WrappedInstance, ) -> Result<(Vec, u64, CallTree), Error> { - self.push_callstack(contract, limit, instance.mem_len()); + let stack_element = self.push_callstack(contract, limit)?; + let instance = self + .instance(&stack_element.contract_id) + .expect("instance should exist"); instance .snap() @@ -703,7 +750,7 @@ impl Session { }; } self.move_up_prune_call_tree(); - self.clear_stack(); + self.clear_stack_and_instances(); err }) .map_err(Error::normalize)?; @@ -712,20 +759,17 @@ impl Session { let spent = limit - instance.get_remaining_gas(); for elem in self.inner.call_tree.iter() { - 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), - })?; + let instance = self + .instance(&elem.contract_id) + .expect("instance should exist"); + instance + .apply() + .map_err(|err| Error::MemorySnapshotFailure { + reason: None, + io: Arc::new(err), + })?; } - self.clear_stack(); + self.clear_stack_and_instances(); let mut call_tree = CallTree::new(); mem::swap(&mut self.inner.call_tree, &mut call_tree);