From 5656b23be537e3ec21ac8fcc7f5f90e4c21eba0c Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 4 May 2022 18:18:09 -0400 Subject: [PATCH 1/2] feat: resolve the memory once Instead of looking up the memory export on every syscall, resolve it when we construct the invocation container. fixes #510 --- fvm/src/call_manager/default.rs | 25 ++++++++++++++----------- fvm/src/machine/engine.rs | 16 +++++++++++++--- fvm/src/syscalls/bind.rs | 17 +++++++---------- fvm/src/syscalls/mod.rs | 7 ++++++- 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 5546d84ed..793ce1f01 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -12,7 +12,7 @@ use super::{Backtrace, CallManager, InvocationResult, NO_DATA_BLOCK_ID}; use crate::call_manager::backtrace::Frame; use crate::call_manager::FinishRet; use crate::gas::GasTracker; -use crate::kernel::{ClassifyResult, ExecutionError, Kernel, Result, SyscallError}; +use crate::kernel::{ExecutionError, Kernel, Result, SyscallError}; use crate::machine::Machine; use crate::syscalls::error::Abort; use crate::syscalls::{charge_for_exec, update_gas_available}; @@ -333,18 +333,21 @@ where // Make a store. let mut store = engine.new_store(kernel); - // Instantiate the module. - let instance = match engine - .get_instance(&mut store, &state.code) - .and_then(|i| i.context("actor code not found")) - .or_fatal() - { - Ok(ret) => ret, - Err(err) => return (Err(err), store.into_data().kernel.into_call_manager()), - }; - // From this point on, there are no more syscall errors, only aborts. let result: std::result::Result = (|| { + // Instantiate the module. + let instance = engine + .get_instance(&mut store, &state.code) + .and_then(|i| i.context("actor code not found")) + .map_err(Abort::Fatal)?; + + // Resolve and store a reference to the exported memory. + let memory = instance + .get_memory(&mut store, "memory") + .context("actor has no memory export") + .map_err(Abort::Fatal)?; + store.data_mut().memory = memory; + // Lookup the invoke method. let invoke: wasmtime::TypedFunc<(u32,), u32> = instance .get_typed_func(&mut store, "invoke") diff --git a/fvm/src/machine/engine.rs b/fvm/src/machine/engine.rs index 2137af1f6..a405effe9 100644 --- a/fvm/src/machine/engine.rs +++ b/fvm/src/machine/engine.rs @@ -7,7 +7,7 @@ use anyhow::{anyhow, Context}; use cid::Cid; use fvm_ipld_blockstore::Blockstore; use fvm_wasm_instrument::gas_metering::GAS_COUNTER_NAME; -use wasmtime::{Global, GlobalType, Linker, Module, Mutability, Val, ValType}; +use wasmtime::{Global, GlobalType, Linker, Memory, MemoryType, Module, Mutability, Val, ValType}; use crate::gas::WasmGasPrices; use crate::machine::NetworkConfig; @@ -119,9 +119,13 @@ pub fn default_wasmtime_config() -> wasmtime::Config { struct EngineInner { engine: wasmtime::Engine, - /// dummy gas global used in store costructor to avoid making - /// InvocationData.avail_gas_global an Option + /// These two fields are used used in the store constructor to avoid resolve a chicken & egg + /// situation: We need the store before we can get the real values, but we need to create the + /// `InvocationData` before we can make the store. + /// + /// Alternatively, we could use `Option`s. But then we need to unwrap everywhere. dummy_gas_global: Global, + dummy_memory: Memory, module_cache: Mutex>, instance_cache: Mutex>, @@ -150,8 +154,12 @@ impl Engine { let dummy_gg = Global::new(&mut dummy_store, gg_type, Val::I64(0)) .expect("failed to create dummy gas global"); + let dummy_memory = Memory::new(&mut dummy_store, MemoryType::new(0, Some(0))) + .expect("failed to create dummy memory"); + Ok(Engine(Arc::new(EngineInner { engine, + dummy_memory, dummy_gas_global: dummy_gg, module_cache: Default::default(), instance_cache: Mutex::new(anymap::Map::new()), @@ -299,6 +307,7 @@ impl Engine { None => return Ok(None), }; let instance = cache.linker.instantiate(&mut *store, module)?; + Ok(Some(instance)) } @@ -309,6 +318,7 @@ impl Engine { last_error: None, avail_gas_global: self.0.dummy_gas_global, last_milligas_available: 0, + memory: self.0.dummy_memory, }; let mut store = wasmtime::Store::new(&self.0.engine, id); diff --git a/fvm/src/syscalls/bind.rs b/fvm/src/syscalls/bind.rs index 7d7cdb737..2afcb48dd 100644 --- a/fvm/src/syscalls/bind.rs +++ b/fvm/src/syscalls/bind.rs @@ -2,7 +2,7 @@ use std::mem; use fvm_shared::error::ErrorNumber; use fvm_shared::sys::SyscallSafe; -use wasmtime::{Caller, Linker, Trap, WasmTy}; +use wasmtime::{Caller, Linker, WasmTy}; use super::context::Memory; use super::error::Abort; @@ -89,13 +89,10 @@ where fn memory_and_data<'a, K: Kernel>( caller: &'a mut Caller<'_, InvocationData>, -) -> Result<(&'a mut Memory, &'a mut InvocationData), Trap> { - let (mem, data) = caller - .get_export("memory") - .and_then(|m| m.into_memory()) - .ok_or_else(|| Trap::new("failed to lookup actor memory"))? - .data_and_store_mut(caller); - Ok((Memory::new(mem), data)) +) -> (&'a mut Memory, &'a mut InvocationData) { + let memory_handle = caller.data().memory; + let (mem, data) = memory_handle.data_and_store_mut(caller); + (Memory::new(mem), data) } // Unfortunately, we can't implement this for _all_ functions. So we implement it for functions of up to 6 arguments. @@ -120,7 +117,7 @@ macro_rules! impl_bind_syscalls { self.func_wrap(module, name, move |mut caller: Caller<'_, InvocationData> $(, $t: $t)*| { charge_for_exec(&mut caller)?; - let (mut memory, mut data) = memory_and_data(&mut caller)?; + let (mut memory, mut data) = memory_and_data(&mut caller); let ctx = Context{kernel: &mut data.kernel, memory: &mut memory}; let out = syscall(ctx $(, $t)*).into(); @@ -148,7 +145,7 @@ macro_rules! impl_bind_syscalls { self.func_wrap(module, name, move |mut caller: Caller<'_, InvocationData>, ret: u32 $(, $t: $t)*| { charge_for_exec(&mut caller)?; - let (mut memory, mut data) = memory_and_data(&mut caller)?; + let (mut memory, mut data) = memory_and_data(&mut caller); // We need to check to make sure we can store the return value _before_ we do anything. if (ret as u64) > (memory.len() as u64) diff --git a/fvm/src/syscalls/mod.rs b/fvm/src/syscalls/mod.rs index ec902d59f..a368abe9b 100644 --- a/fvm/src/syscalls/mod.rs +++ b/fvm/src/syscalls/mod.rs @@ -2,7 +2,7 @@ use std::mem; use anyhow::{anyhow, Context as _}; use cid::Cid; -use wasmtime::{AsContextMut, Global, Linker, Val}; +use wasmtime::{AsContextMut, Global, Linker, Memory, Val}; use crate::call_manager::backtrace; use crate::kernel::ExecutionError; @@ -30,16 +30,21 @@ pub(self) use context::Context; pub struct InvocationData { /// The kernel on which this actor is being executed. pub kernel: K, + /// The last-seen syscall error. This error is considered the abort "cause" if an actor aborts /// after receiving this error without calling any other syscalls. pub last_error: Option, /// The global containing remaining available gas. pub avail_gas_global: Global, + /// The last-set milligas limit. When `charge_for_exec` is called, we charge for the /// _difference_ between the current gas available (the wasm global) and the /// `last_milligas_available`. pub last_milligas_available: i64, + + /// The invocation's imported "memory". + pub memory: Memory, } pub fn update_gas_available( From 520c3daa27092b1f03089fef9c3dd9ebf94dc4cf Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 4 May 2022 20:04:04 -0400 Subject: [PATCH 2/2] fix: export a memory --- testing/integration/tests/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/testing/integration/tests/lib.rs b/testing/integration/tests/lib.rs index 00f45a2c0..328e90d5c 100644 --- a/testing/integration/tests/lib.rs +++ b/testing/integration/tests/lib.rs @@ -73,6 +73,7 @@ fn out_of_gas() { const WAT: &str = r#" ;; Mock invoke function (module + (memory (export "memory") 1) (func (export "invoke") (param $x i32) (result i32) (loop (br 0) @@ -127,6 +128,7 @@ fn out_of_stack() { const WAT: &str = r#" ;; Mock invoke function (module + (memory (export "memory") 1) (func (export "invoke") (param $x i32) (result i32) (i64.const 123) (call 1)