Skip to content

Commit

Permalink
piecrust: one instance per contract function call
Browse files Browse the repository at this point in the history 
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 erroring
calls.

Simplifies the instance code by using an atomic integer to construct an
atomically reference counted pointer, which then gets shared with the
environment. This fixes some bad dereferences occuring under certain
circumstances.

Resolves: #325
  • Loading branch information
Eduardo Leegwater Simões committed Feb 10, 2024
1 parent bb2f3fc commit c4b7675
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 154 deletions.
3 changes: 3 additions & 0 deletions piecrust/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
11 changes: 7 additions & 4 deletions piecrust/src/call_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -47,7 +50,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() {
Expand All @@ -63,7 +66,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 {
Expand Down Expand Up @@ -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.
Expand Down
38 changes: 18 additions & 20 deletions piecrust/src/imports.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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?
Expand All @@ -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();

Expand All @@ -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,
Expand All @@ -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]);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -327,7 +325,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;
Expand All @@ -342,7 +340,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];
Expand All @@ -365,7 +363,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();
Expand All @@ -377,7 +375,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];
Expand All @@ -392,7 +390,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();

Expand Down
80 changes: 58 additions & 22 deletions piecrust/src/instance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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<Env>,
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<WrappedInstance>,
}

impl Deref for Env {
Expand All @@ -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 {
Expand Down Expand Up @@ -94,6 +113,7 @@ impl WrappedInstance {
let env = Env {
self_id: contract_id,
session,
instance: MaybeUninit::uninit(),
};

let module =
Expand Down Expand Up @@ -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<Env>,
memory: Memory,
count: AtomicUsize,
}

impl WrappedInstanceInner {
pub(crate) fn snap(&mut self) -> io::Result<()> {
self.memory.snap()?;
Ok(())
Expand Down Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit c4b7675

Please sign in to comment.