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

Author: Eduardo Leegwater Simões

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
 
 <!-- ISSUES -->
+[#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

piecrust/src/call_tree.rs

@@ -10,7 +10,7 @@ use std::mem;
 use piecrust_uplink::ContractId;
 
 /// An element of the call tree.
-#[derive(Debug, Clone, Copy)]
+#[derive(Clone)]
 pub struct CallTreeElem {
     pub contract_id: ContractId,
     pub limit: u64,
@@ -47,7 +47,7 @@ impl CallTree {
     pub(crate) fn move_up(&mut self, spent: u64) -> Option<CallTreeElem> {
         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 +63,7 @@ impl CallTree {
     /// current element.
     pub(crate) fn move_up_prune(&mut self) -> Option<CallTreeElem> {
         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 +98,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.

piecrust/src/imports.rs

@@ -138,12 +138,12 @@ pub(crate) fn hq(
 ) -> WasmtimeResult<u32> {
     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<u32> {
     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<i32> {
     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::<ContractId>()],
         );
 
-        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<Env>, 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<Env>, 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<Env>) -> u64 {
 
 fn spent(fenv: Caller<Env>) -> 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<Env>, 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<Env>, arg_len: u32) -> WasmtimeResult<()> {
 }
 
 fn owner(fenv: Caller<Env>, mod_id_ofs: usize) -> WasmtimeResult<i32> {
-    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();
 

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<Env>,
-    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<WrappedInstance>,
 }
 
 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<Env>,
+    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<F, R>(&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 {