Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Fix WASM executor without instance reuse; cleanups and refactoring #10313

Merged
paritytech-processbot merged 5 commits into from
Nov 23, 2021
Merged

Fix WASM executor without instance reuse; cleanups and refactoring #10313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa01782
Fix WASM executor without instance reuse; cleanups and refactoring
koute Nov 19, 2021
c1b974b
Align to review comments
koute Nov 22, 2021
99607a1
Move the functions for reading/writing memory to `util.rs`
koute Nov 22, 2021
22dd192
Only `#[ignore]` the test in debug builds
koute Nov 22, 2021
e63c7a1
More review comments and minor extra comments
koute Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 101 additions & 107 deletions client/executor/wasmtime/src/host.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
//! This module defines `HostState` and `HostContext` structs which provide logic and state
//! required for execution of host.

use crate::{instance_wrapper::InstanceWrapper, runtime::StoreData};
use crate::runtime::StoreData;
use codec::{Decode, Encode};
use log::trace;
use sc_allocator::FreeingBumpHeapAllocator;
Expand All @@ -30,101 +30,100 @@ use sc_executor_common::{
};
use sp_core::sandbox as sandbox_primitives;
use sp_wasm_interface::{FunctionContext, MemoryId, Pointer, Sandbox, WordSize};
use std::{cell::RefCell, rc::Rc};
use wasmtime::{Caller, Func, Val};

// The sandbox store is inside of a Option<Box<..>>> so that we can temporarily borrow it.
struct SandboxStore(Option<Box<sandbox::Store<Func>>>);

// There are a bunch of `Rc`s within the sandbox store, however we only manipulate
// those within one thread so this should be safe.
unsafe impl Send for SandboxStore {}

/// The state required to construct a HostContext context. The context only lasts for one host
/// call, whereas the state is maintained for the duration of a Wasm runtime call, which may make
/// many different host calls that must share state.
pub struct HostState {
/// We need some interior mutability here since the host state is shared between all host
/// function handlers and the wasmtime backend's `impl WasmRuntime`.
///
/// Furthermore, because of recursive calls (e.g. runtime can create and call an sandboxed
/// instance which in turn can call the runtime back) we have to be very careful with borrowing
/// those.
///
/// Basically, most of the interactions should do temporary borrow immediately releasing the
/// borrow after performing necessary queries/changes.
sandbox_store: Rc<RefCell<sandbox::Store<Func>>>,
allocator: RefCell<FreeingBumpHeapAllocator>,
instance: Rc<InstanceWrapper>,
sandbox_store: SandboxStore,
allocator: FreeingBumpHeapAllocator,
}

impl HostState {
/// Constructs a new `HostState`.
pub fn new(allocator: FreeingBumpHeapAllocator, instance: Rc<InstanceWrapper>) -> Self {
pub fn new(allocator: FreeingBumpHeapAllocator) -> Self {
HostState {
sandbox_store: Rc::new(RefCell::new(sandbox::Store::new(
sandbox_store: SandboxStore(Some(Box::new(sandbox::Store::new(
sandbox::SandboxBackend::TryWasmer,
))),
allocator: RefCell::new(allocator),
instance,
)))),
allocator,
}
}

/// Materialize `HostContext` that can be used to invoke a substrate host `dyn Function`.
pub(crate) fn materialize<'a, 'b, 'c>(
&'a self,
caller: &'b mut Caller<'c, StoreData>,
) -> HostContext<'a, 'b, 'c> {
HostContext { host_state: self, caller }
}
}

/// A `HostContext` implements `FunctionContext` for making host calls from a Wasmtime
/// runtime. The `HostContext` exists only for the lifetime of the call and borrows state from
/// a longer-living `HostState`.
pub(crate) struct HostContext<'a, 'b, 'c> {
host_state: &'a HostState,
caller: &'b mut Caller<'c, StoreData>,
pub(crate) struct HostContext<'a, 'b> {
pub(crate) caller: &'a mut Caller<'b, StoreData>,
}

impl<'a, 'b, 'c> std::ops::Deref for HostContext<'a, 'b, 'c> {
type Target = HostState;
fn deref(&self) -> &HostState {
self.host_state
impl<'a, 'b> HostContext<'a, 'b> {
fn host_state(&self) -> &HostState {
self.caller.data().host_state().expect("host state cannot be empty")
}

fn host_state_mut(&mut self) -> &mut HostState {
self.caller.data_mut().host_state_mut().expect("host state cannot be empty")
}

fn sandbox_store(&self) -> &sandbox::Store<Func> {
self.host_state()
.sandbox_store
.0
.as_ref()
.expect("sandbox store is only empty when temporarily borrowed")
}

fn sandbox_store_mut(&mut self) -> &mut sandbox::Store<Func> {
self.host_state_mut()
.sandbox_store
.0
.as_mut()
.expect("sandbox store is only empty when temporarily borrowed")
}
}

impl<'a, 'b, 'c> sp_wasm_interface::FunctionContext for HostContext<'a, 'b, 'c> {
impl<'a, 'b> sp_wasm_interface::FunctionContext for HostContext<'a, 'b> {
fn read_memory_into(
&self,
address: Pointer<u8>,
dest: &mut [u8],
) -> sp_wasm_interface::Result<()> {
let ctx = &self.caller;
self.host_state
.instance
.read_memory_into(ctx, address, dest)
crate::instance_wrapper::read_memory_into(&self.caller, address, dest)
.map_err(|e| e.to_string())
}

fn write_memory(&mut self, address: Pointer<u8>, data: &[u8]) -> sp_wasm_interface::Result<()> {
let ctx = &mut self.caller;
self.host_state
.instance
.write_memory_from(ctx, address, data)
crate::instance_wrapper::write_memory_from(&mut self.caller, address, data)
.map_err(|e| e.to_string())
}

fn allocate_memory(&mut self, size: WordSize) -> sp_wasm_interface::Result<Pointer<u8>> {
let ctx = &mut self.caller;
let allocator = &self.host_state.allocator;

self.host_state
.instance
.allocate(ctx, &mut *allocator.borrow_mut(), size)
let memory = self.caller.data().memory();
let (memory, data) = memory.data_and_store_mut(&mut self.caller);
data.host_state_mut()
.expect("host state cannot be empty")
.allocator
.allocate(memory, size)
.map_err(|e| e.to_string())
}

fn deallocate_memory(&mut self, ptr: Pointer<u8>) -> sp_wasm_interface::Result<()> {
let ctx = &mut self.caller;
let allocator = &self.host_state.allocator;

self.host_state
.instance
.deallocate(ctx, &mut *allocator.borrow_mut(), ptr)
let memory = self.caller.data().memory();
let (memory, data) = memory.data_and_store_mut(&mut self.caller);
data.host_state_mut()
.expect("host state cannot be empty")
.allocator
.deallocate(memory, ptr)
.map_err(|e| e.to_string())
}

Expand All @@ -133,16 +132,15 @@ impl<'a, 'b, 'c> sp_wasm_interface::FunctionContext for HostContext<'a, 'b, 'c>
}
}

impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
impl<'a, 'b> Sandbox for HostContext<'a, 'b> {
fn memory_get(
&mut self,
memory_id: MemoryId,
offset: WordSize,
buf_ptr: Pointer<u8>,
buf_len: WordSize,
) -> sp_wasm_interface::Result<u32> {
let sandboxed_memory =
self.sandbox_store.borrow().memory(memory_id).map_err(|e| e.to_string())?;
let sandboxed_memory = self.sandbox_store().memory(memory_id).map_err(|e| e.to_string())?;

let len = buf_len as usize;

Expand All @@ -151,8 +149,9 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
Ok(buffer) => buffer,
};

let instance = self.instance.clone();
if let Err(_) = instance.write_memory_from(&mut self.caller, buf_ptr, &buffer) {
if let Err(_) =
crate::instance_wrapper::write_memory_from(&mut self.caller, buf_ptr, &buffer)
{
return Ok(sandbox_primitives::ERR_OUT_OF_BOUNDS)
}

Expand All @@ -166,12 +165,11 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
val_ptr: Pointer<u8>,
val_len: WordSize,
) -> sp_wasm_interface::Result<u32> {
let sandboxed_memory =
self.sandbox_store.borrow().memory(memory_id).map_err(|e| e.to_string())?;
let sandboxed_memory = self.sandbox_store().memory(memory_id).map_err(|e| e.to_string())?;

let len = val_len as usize;

let buffer = match self.instance.read_memory(&self.caller, val_ptr, len) {
let buffer = match crate::instance_wrapper::read_memory(&self.caller, val_ptr, len) {
Err(_) => return Ok(sandbox_primitives::ERR_OUT_OF_BOUNDS),
Ok(buffer) => buffer,
};
Expand All @@ -184,17 +182,11 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
}

fn memory_teardown(&mut self, memory_id: MemoryId) -> sp_wasm_interface::Result<()> {
self.sandbox_store
.borrow_mut()
.memory_teardown(memory_id)
.map_err(|e| e.to_string())
self.sandbox_store_mut().memory_teardown(memory_id).map_err(|e| e.to_string())
}

fn memory_new(&mut self, initial: u32, maximum: u32) -> sp_wasm_interface::Result<u32> {
self.sandbox_store
.borrow_mut()
.new_memory(initial, maximum)
.map_err(|e| e.to_string())
self.sandbox_store_mut().new_memory(initial, maximum).map_err(|e| e.to_string())
}

fn invoke(
Expand All @@ -215,14 +207,10 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
.map(Into::into)
.collect::<Vec<_>>();

let instance =
self.sandbox_store.borrow().instance(instance_id).map_err(|e| e.to_string())?;
let instance = self.sandbox_store().instance(instance_id).map_err(|e| e.to_string())?;

let dispatch_thunk = self
.sandbox_store
.borrow()
.dispatch_thunk(instance_id)
.map_err(|e| e.to_string())?;
let dispatch_thunk =
self.sandbox_store().dispatch_thunk(instance_id).map_err(|e| e.to_string())?;

let result = instance.invoke(
export_name,
Expand All @@ -249,8 +237,7 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
}

fn instance_teardown(&mut self, instance_id: u32) -> sp_wasm_interface::Result<()> {
self.sandbox_store
.borrow_mut()
self.sandbox_store_mut()
.instance_teardown(instance_id)
.map_err(|e| e.to_string())
}
Expand All @@ -264,14 +251,12 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
) -> sp_wasm_interface::Result<u32> {
// Extract a dispatch thunk from the instance's table by the specified index.
let dispatch_thunk = {
let ctx = &mut self.caller;
let table_item = self
.host_state
.instance
let table = self
.caller
.data()
.table()
.as_ref()
.ok_or_else(|| "Runtime doesn't have a table; sandbox is unavailable")?
.get(ctx, dispatch_thunk_id);
.ok_or_else(|| "Runtime doesn't have a table; sandbox is unavailable")?;
let table_item = table.get(&mut self.caller, dispatch_thunk_id);

table_item
.ok_or_else(|| "dispatch_thunk_id is out of bounds")?
Expand All @@ -281,28 +266,38 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
.clone()
};

let guest_env =
match sandbox::GuestEnvironment::decode(&*self.sandbox_store.borrow(), raw_env_def) {
Ok(guest_env) => guest_env,
Err(_) => return Ok(sandbox_primitives::ERR_MODULE as u32),
};
let guest_env = match sandbox::GuestEnvironment::decode(&self.sandbox_store(), raw_env_def)
{
Ok(guest_env) => guest_env,
Err(_) => return Ok(sandbox_primitives::ERR_MODULE as u32),
};

let store = self.sandbox_store.clone();
let store = &mut store.borrow_mut();
let result = store
.instantiate(
let mut store = self
.host_state_mut()
.sandbox_store
.0
.take()
.expect("sandbox store is only empty when borrowed");

// The `catch_unwind` is probably unnecessary here, but let's do it just
// in case so that we can properly unborrow the sandbox store.
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
store.instantiate(
wasm,
guest_env,
state,
&mut SandboxContext { host_context: self, dispatch_thunk: dispatch_thunk.clone() },
)
.map(|i| i.register(store, dispatch_thunk));
}));

let instance_idx_or_err_code = match result {
Ok(instance_idx) => instance_idx,
Err(sandbox::InstantiationError::StartTrapped) => sandbox_primitives::ERR_EXECUTION,
Err(_) => sandbox_primitives::ERR_MODULE,
};
self.host_state_mut().sandbox_store.0 = Some(store);

let instance_idx_or_err_code =
match result.expect("instantiating the sandbox does not panic") {
Copy link
Contributor

@pepyakin pepyakin Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, the sandbox module or runtimes can actually panic!

When state-machine runs, it needs to have access to storage backend. This storage backend can be trusted, i.e. the on-disk storage. We assume it never panics and if it does, then that's an unrecoverable error.

However, when running a light-client, a backend could be based on a witness - basically data, and a merkle proof that that data actually comes from where it is claimed to come from. The caveat is that we do not eagerly check this proof, as far as I understand, and there are situations when the runtime may request a storage entry that actually does not present in the witness. In such case, a panic is emitted. You can get more details and my complaints here under the heading "panics". I assumed we could remove this code by now I am not sure → we will need at least some way to communicating such conditions through the sandbox layer (cc @athei) (assuming we will continue to support sandbox API)

To tie back it to this specific case, I assume the following can happen:

  1. on a light client we execute some call with a proof. The proof is corrupted and the backend will panic if X is accessed.
  2. the runtime is spawned.
  3. the runtime instantiates a new sandbox with some wasm code.
  4. In the start function calls back into the runtime and makes the runtime fetch X from storage and thus panics.
  5. it unwinds here and here we panic due to unwrap losing the context of the original panic.

Soi I am wondering, if were better to resume_unwind or not catching at all?

cc @bkchr

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Copy link
Contributor Author

@koute koute Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good to know! Okay, so since this is technically outside of the scope of this PR I'll just do a resume_unwind to keep the original behavior. (I don't know, this might not be be necessary and maybe we could just ignore the panic altogether, however I think it's just simpler to catch it and restore the sandbox store anyway even if might not be necessary now. One less thing you have to think about whether it'll break something or not.)

Ok(instance) => instance.register(&mut self.sandbox_store_mut(), dispatch_thunk),
Err(sandbox::InstantiationError::StartTrapped) => sandbox_primitives::ERR_EXECUTION,
Err(_) => sandbox_primitives::ERR_MODULE,
};

Ok(instance_idx_or_err_code as u32)
}
Expand All @@ -312,20 +307,19 @@ impl<'a, 'b, 'c> Sandbox for HostContext<'a, 'b, 'c> {
instance_idx: u32,
name: &str,
) -> sp_wasm_interface::Result<Option<sp_wasm_interface::Value>> {
self.sandbox_store
.borrow()
self.sandbox_store()
.instance(instance_idx)
.map(|i| i.get_global_val(name))
.map_err(|e| e.to_string())
}
}

struct SandboxContext<'a, 'b, 'c, 'd> {
host_context: &'a mut HostContext<'b, 'c, 'd>,
struct SandboxContext<'a, 'b, 'c> {
host_context: &'a mut HostContext<'b, 'c>,
dispatch_thunk: Func,
}

impl<'a, 'b, 'c, 'd> sandbox::SandboxContext for SandboxContext<'a, 'b, 'c, 'd> {
impl<'a, 'b, 'c> sandbox::SandboxContext for SandboxContext<'a, 'b, 'c> {
fn invoke(
&mut self,
invoke_args_ptr: Pointer<u8>,
Expand Down
15 changes: 2 additions & 13 deletions client/executor/wasmtime/src/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use crate::{
host::HostContext,
runtime::{Store, StoreData},
util,
};
Expand Down Expand Up @@ -191,19 +192,7 @@ fn call_static<'a>(
mut caller: Caller<'a, StoreData>,
) -> Result<(), wasmtime::Trap> {
let unwind_result = {
let host_state = caller
.data()
.host_state()
.expect(
"host functions can be called only from wasm instance;
wasm instance is always called initializing context;
therefore host_ctx cannot be None;
qed
",
)
.clone();

let mut host_ctx = host_state.materialize(&mut caller);
let mut host_ctx = HostContext { caller: &mut caller };

// `from_wasmtime_val` panics if it encounters a value that doesn't fit into the values
// available in substrate.
Expand Down
Loading