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

Implement runtime version checks in set_code #4548

Merged
merged 17 commits into from
Jan 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
235 changes: 117 additions & 118 deletions Cargo.lock

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions client/executor/common/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ pub enum Error {
/// Runtime failed.
#[display(fmt="Runtime error")]
Runtime,
/// Runtime panicked.
#[display(fmt="Runtime panicked: {}", _0)]
#[from(ignore)]
RuntimePanicked(String),
/// Invalid memory reference.
#[display(fmt="Invalid memory reference")]
InvalidMemoryReference,
Expand Down
4 changes: 1 addition & 3 deletions client/executor/common/src/wasm_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
//! Definitions for a wasm runtime.

use crate::error::Error;
use sp_core::traits::Externalities;
use sp_wasm_interface::Function;

/// A trait that defines an abstract wasm runtime.
Expand All @@ -34,6 +33,5 @@ pub trait WasmRuntime {
fn host_functions(&self) -> &[&'static dyn Function];

/// Call a method in the Substrate runtime by name. Returns the encoded result on success.
fn call(&mut self, ext: &mut dyn Externalities, method: &str, data: &[u8])
-> Result<Vec<u8>, Error>;
fn call(&mut self, method: &str, data: &[u8]) -> Result<Vec<u8>, Error>;
}
2 changes: 1 addition & 1 deletion client/executor/src/integration_tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ fn call_in_wasm<E: Externalities>(
code: &[u8],
heap_pages: u64,
) -> crate::error::Result<Vec<u8>> {
crate::call_in_wasm::<E, HostFunctions>(
crate::call_in_wasm::<HostFunctions>(
function,
call_data,
execution_method,
Expand Down
43 changes: 36 additions & 7 deletions client/executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ mod wasm_runtime;
mod integration_tests;

pub use wasmi;
pub use native_executor::{with_native_environment, NativeExecutor, NativeExecutionDispatch};
pub use native_executor::{with_externalities_safe, NativeExecutor, NativeExecutionDispatch};
pub use sp_version::{RuntimeVersion, NativeVersion};
pub use codec::Codec;
#[doc(hidden)]
Expand All @@ -57,26 +57,55 @@ pub use sc_executor_common::{error, allocator, sandbox};
/// - `call_data`: Will be given as input parameters to `function`
/// - `execution_method`: The execution method to use.
/// - `ext`: The externalities that should be set while executing the wasm function.
/// If `None` is given, no externalities will be set.
/// - `heap_pages`: The number of heap pages to allocate.
///
/// Returns the `Vec<u8>` that contains the return value of the function.
pub fn call_in_wasm<E: Externalities, HF: sp_wasm_interface::HostFunctions>(
pub fn call_in_wasm<HF: sp_wasm_interface::HostFunctions>(
function: &str,
call_data: &[u8],
execution_method: WasmExecutionMethod,
ext: &mut E,
ext: &mut dyn Externalities,
Copy link
Member

Choose a reason for hiding this comment

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

all the storage accesses (and there are a lot) go through externalities. does this really have an insubstantial effect on performance?

Copy link
Member Author

Choose a reason for hiding this comment

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

We store the Externalities as trait pointer in the environmental anyway. The compiler can not optimize this anywax.

code: &[u8],
heap_pages: u64,
allow_missing_imports: bool,
) -> error::Result<Vec<u8>> {
let mut instance = wasm_runtime::create_wasm_runtime_with_code(
call_in_wasm_with_host_functions(
function,
call_data,
execution_method,
heap_pages,
ext,
code,
heap_pages,
HF::host_functions(),
allow_missing_imports,
)
}

/// Non-generic version of [`call_in_wasm`] that takes the `host_functions` as parameter.
/// For more information please see [`call_in_wasm`].
pub fn call_in_wasm_with_host_functions(
function: &str,
call_data: &[u8],
execution_method: WasmExecutionMethod,
ext: &mut dyn Externalities,
code: &[u8],
heap_pages: u64,
host_functions: Vec<&'static dyn sp_wasm_interface::Function>,
allow_missing_imports: bool,
) -> error::Result<Vec<u8>> {
let instance = wasm_runtime::create_wasm_runtime_with_code(
execution_method,
heap_pages,
code,
host_functions,
allow_missing_imports,
)?;
instance.call(ext, function, call_data)

// It is safe, as we delete the instance afterwards.
let mut instance = std::panic::AssertUnwindSafe(instance);

with_externalities_safe(ext, move || instance.call(function, call_data)).and_then(|r| r)
}

/// Provides runtime information.
Expand All @@ -98,7 +127,7 @@ mod tests {
fn call_in_interpreted_wasm_works() {
let mut ext = TestExternalities::default();
let mut ext = ext.ext();
let res = call_in_wasm::<_, sp_io::SubstrateHostFunctions>(
let res = call_in_wasm::<sp_io::SubstrateHostFunctions>(
"test_empty_return",
&[],
WasmExecutionMethod::Interpreted,
Expand Down
76 changes: 53 additions & 23 deletions client/executor/src/native_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use sp_version::{NativeVersion, RuntimeVersion};
use codec::{Decode, Encode};
use sp_core::{NativeOrEncoded, traits::{CodeExecutor, Externalities}};
use log::trace;
use std::{result, cell::RefCell, panic::{UnwindSafe, AssertUnwindSafe}};
use std::{result, cell::RefCell, panic::{UnwindSafe, AssertUnwindSafe}, sync::Arc};
use sp_wasm_interface::{HostFunctions, Function};
use sc_executor_common::wasm_runtime::WasmRuntime;

Expand All @@ -33,22 +33,29 @@ thread_local! {
/// Default num of pages for the heap
const DEFAULT_HEAP_PAGES: u64 = 1024;

pub(crate) fn safe_call<F, U>(f: F) -> Result<U>
where F: UnwindSafe + FnOnce() -> U
{
// Substrate uses custom panic hook that terminates process on panic. Disable
// termination for the native call.
let _guard = sp_panic_handler::AbortGuard::force_unwind();
std::panic::catch_unwind(f).map_err(|_| Error::Runtime)
}

/// Set up the externalities and safe calling environment to execute calls to a native runtime.
/// Set up the externalities and safe calling environment to execute runtime calls.
///
/// If the inner closure panics, it will be caught and return an error.
pub fn with_native_environment<F, U>(ext: &mut dyn Externalities, f: F) -> Result<U>
pub fn with_externalities_safe<F, U>(ext: &mut dyn Externalities, f: F) -> Result<U>
where F: UnwindSafe + FnOnce() -> U
{
sp_externalities::set_and_run_with_externalities(ext, move || safe_call(f))
sp_externalities::set_and_run_with_externalities(
ext,
move || {
// Substrate uses custom panic hook that terminates process on panic. Disable
// termination for the native call.
let _guard = sp_panic_handler::AbortGuard::force_unwind();
std::panic::catch_unwind(f).map_err(|e| {
if let Some(err) = e.downcast_ref::<String>() {
Error::RuntimePanicked(err.clone())
} else if let Some(err) = e.downcast_ref::<&'static str>() {
Error::RuntimePanicked(err.to_string())
} else {
Error::RuntimePanicked("Unknown panic".into())
}
})
},
)
}

/// Delegate for dispatching a CodeExecutor call.
Expand Down Expand Up @@ -80,7 +87,7 @@ pub struct NativeExecutor<D> {
/// The number of 64KB pages to allocate for Wasm execution.
default_heap_pages: u64,
/// The host functions registered with this instance.
host_functions: Vec<&'static dyn Function>,
host_functions: Arc<Vec<&'static dyn Function>>,
}

impl<D: NativeExecutionDispatch> NativeExecutor<D> {
Expand All @@ -107,7 +114,7 @@ impl<D: NativeExecutionDispatch> NativeExecutor<D> {
fallback_method,
native_version: D::native_version(),
default_heap_pages: default_heap_pages.unwrap_or(DEFAULT_HEAP_PAGES),
host_functions,
host_functions: Arc::new(host_functions),
}
}

Expand Down Expand Up @@ -139,7 +146,7 @@ impl<D: NativeExecutionDispatch> NativeExecutor<D> {
ext,
self.fallback_method,
self.default_heap_pages,
&self.host_functions,
&*self.host_functions,
)?;

let runtime = AssertUnwindSafe(runtime);
Expand Down Expand Up @@ -181,7 +188,7 @@ impl<D: NativeExecutionDispatch> RuntimeInfo for NativeExecutor<D> {
}
}

impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
impl<D: NativeExecutionDispatch + 'static> CodeExecutor for NativeExecutor<D> {
type Error = Error;

fn call
Expand Down Expand Up @@ -212,13 +219,15 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
onchain_version,
);

safe_call(
move || runtime.call(&mut **ext, method, data).map(NativeOrEncoded::Encoded)
with_externalities_safe(
&mut **ext,
move || runtime.call(method, data).map(NativeOrEncoded::Encoded)
)
}
(false, _, _) => {
safe_call(
move || runtime.call(&mut **ext, method, data).map(NativeOrEncoded::Encoded)
with_externalities_safe(
&mut **ext,
move || runtime.call(method, data).map(NativeOrEncoded::Encoded)
)
},
(true, true, Some(call)) => {
Expand All @@ -230,7 +239,7 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
);

used_native = true;
let res = with_native_environment(&mut **ext, move || (call)())
let res = with_externalities_safe(&mut **ext, move || (call)())
.and_then(|r| r
.map(NativeOrEncoded::Native)
.map_err(|s| Error::ApiError(s.to_string()))
Expand All @@ -255,6 +264,27 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
}
}

impl<D: NativeExecutionDispatch> sp_core::traits::CallInWasm for NativeExecutor<D> {
fn call_in_wasm(
&self,
wasm_blob: &[u8],
method: &str,
call_data: &[u8],
ext: &mut dyn Externalities,
) -> std::result::Result<Vec<u8>, String> {
crate::call_in_wasm_with_host_functions(
method,
call_data,
self.fallback_method,
ext,
wasm_blob,
self.default_heap_pages,
(*self.host_functions).clone(),
false,
).map_err(|e| e.to_string())
}
}

/// Implements a `NativeExecutionDispatch` for provided parameters.
///
/// # Example
Expand Down Expand Up @@ -318,7 +348,7 @@ macro_rules! native_executor_instance {
method: &str,
data: &[u8]
) -> $crate::error::Result<Vec<u8>> {
$crate::with_native_environment(ext, move || $dispatcher(method, data))?
$crate::with_externalities_safe(ext, move || $dispatcher(method, data))?
.ok_or_else(|| $crate::error::Error::MethodNotFound(method.to_owned()))
}

Expand Down
5 changes: 3 additions & 2 deletions client/executor/src/wasm_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,9 @@ fn create_versioned_wasm_runtime<E: Externalities>(
// The following unwind safety assertion is OK because if the method call panics, the
// runtime will be dropped.
let mut runtime = AssertUnwindSafe(runtime.as_mut());
crate::native_executor::safe_call(
move || runtime.call(&mut **ext, "Core_version", &[])
crate::native_executor::with_externalities_safe(
&mut **ext,
move || runtime.call("Core_version", &[])
).map_err(|_| WasmError::Instantiation("panic in call to get runtime version".into()))?
};
let encoded_version = version_result
Expand Down
1 change: 0 additions & 1 deletion client/executor/wasmi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,3 @@ sc-executor-common = { version = "0.8", path = "../common" }
sp-wasm-interface = { version = "2.0.0", path = "../../../primitives/wasm-interface" }
sp-runtime-interface = { version = "2.0.0", path = "../../../primitives/runtime-interface" }
sp-core = { version = "2.0.0", path = "../../../primitives/core" }
sp-externalities = { version = "0.8.0", path = "../../../primitives/externalities" }
16 changes: 5 additions & 11 deletions client/executor/wasmi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use wasmi::{
memory_units::Pages, RuntimeValue::{I32, I64, self},
};
use codec::{Encode, Decode};
use sp_core::{sandbox as sandbox_primitives, traits::Externalities};
use sp_core::sandbox as sandbox_primitives;
use log::{error, trace};
use parity_wasm::elements::{deserialize_buffer, DataSegment, Instruction, Module as RawModule};
use sp_wasm_interface::{
Expand Down Expand Up @@ -381,7 +381,6 @@ fn get_heap_base(module: &ModuleRef) -> Result<u32, Error> {

/// Call a given method in the given wasm-module runtime.
fn call_in_wasm_module(
ext: &mut dyn Externalities,
module_instance: &ModuleRef,
method: &str,
data: &[u8],
Expand Down Expand Up @@ -410,13 +409,10 @@ fn call_in_wasm_module(
let offset = fec.allocate_memory(data.len() as u32)?;
fec.write_memory(offset, data)?;

let result = sp_externalities::set_and_run_with_externalities(
ext,
|| module_instance.invoke_export(
method,
&[I32(u32::from(offset) as i32), I32(data.len() as i32)],
&mut fec,
),
let result = module_instance.invoke_export(
method,
&[I32(u32::from(offset) as i32), I32(data.len() as i32)],
&mut fec,
);

match result {
Expand Down Expand Up @@ -599,7 +595,6 @@ impl WasmRuntime for WasmiRuntime {

fn call(
&mut self,
ext: &mut dyn Externalities,
method: &str,
data: &[u8],
) -> Result<Vec<u8>, Error> {
Expand All @@ -612,7 +607,6 @@ impl WasmRuntime for WasmiRuntime {
e
})?;
call_in_wasm_module(
ext,
&self.instance,
method,
data,
Expand Down
1 change: 0 additions & 1 deletion client/executor/wasmtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ sc-executor-common = { version = "0.8", path = "../common" }
sp-wasm-interface = { version = "2.0.0", path = "../../../primitives/wasm-interface" }
sp-runtime-interface = { version = "2.0.0", path = "../../../primitives/runtime-interface" }
sp-core = { version = "2.0.0", path = "../../../primitives/core" }
sp-externalities = { version = "0.8.0", path = "../../../primitives/externalities" }

cranelift-codegen = "0.50"
cranelift-entity = "0.50"
Expand Down
13 changes: 4 additions & 9 deletions client/executor/wasmtime/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use sc_executor_common::{
error::{Error, Result, WasmError},
wasm_runtime::WasmRuntime,
};
use sp_core::traits::Externalities;
use sp_wasm_interface::{Pointer, WordSize, Function};
use sp_runtime_interface::unpack_ptr_and_len;

Expand Down Expand Up @@ -70,11 +69,10 @@ impl WasmRuntime for WasmtimeRuntime {
&self.host_functions
}

fn call(&mut self, ext: &mut dyn Externalities, method: &str, data: &[u8]) -> Result<Vec<u8>> {
fn call(&mut self, method: &str, data: &[u8]) -> Result<Vec<u8>> {
call_method(
&mut self.context,
&mut self.module,
ext,
method,
data,
self.heap_pages,
Expand Down Expand Up @@ -146,7 +144,6 @@ fn create_compiled_unit(
fn call_method(
context: &mut Context,
module: &mut CompiledModule,
ext: &mut dyn Externalities,
method: &str,
data: &[u8],
heap_pages: u32,
Expand Down Expand Up @@ -176,11 +173,9 @@ fn call_method(
let args = [RuntimeValue::I32(u32::from(data_ptr) as i32), RuntimeValue::I32(data_len as i32)];

// Invoke the function in the runtime.
let outcome = sp_externalities::set_and_run_with_externalities(ext, || {
context
.invoke(&mut instance, method, &args[..])
.map_err(|e| Error::Other(format!("error calling runtime: {}", e)))
})?;
let outcome = context
.invoke(&mut instance, method, &args[..])
.map_err(|e| Error::Other(format!("error calling runtime: {}", e)))?;
let trap_error = reset_env_state_and_take_trap(context, None)?;
let (output_ptr, output_len) = match outcome {
ActionOutcome::Returned { values } => match values.as_slice() {
Expand Down
2 changes: 1 addition & 1 deletion client/finality-grandpa/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ for Environment<B, E, Block, N, RA, SC, VR>
where
Block: 'static,
B: Backend<Block> + 'static,
E: CallExecutor<Block> + Send + Sync + 'static,
E: CallExecutor<Block> + Send + Sync,
N: NetworkT<Block> + 'static + Send,
SC: SelectChain<Block> + 'static,
VR: VotingRule<Block, Client<B, E, Block, RA>>,
Expand Down
Loading