From cfe61349019c361aaf0dc7da1b21a6444a01b494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Leegwater=20Sim=C3=B5es?= Date: Sat, 10 Feb 2024 02:04:05 +0100 Subject: [PATCH] piecrust: one instance per contract function call Changes the instance reclamation code to allow for exactly one module instance per contract function call. This changes the way in which the node processes the state, leading to diverging state roots on errorring calls. Simplifies the instance code by using a boolean to appropriately drop the instance. This allows the data in the struct to be passed around the `Env`, whilst fixing some bad dereferences previously occurring under certain situations. Resolves: #325 --- piecrust/CHANGELOG.md | 3 + piecrust/src/imports.rs | 42 ++++----- piecrust/src/instance.rs | 96 +++++++++++++-------- piecrust/src/session.rs | 182 +++++++++++++++------------------------ 4 files changed, 151 insertions(+), 172 deletions(-) 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);