From 1fa9b541a409228cde567d6118afa4f517b3078f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Oct 2019 16:34:00 +0100 Subject: [PATCH] Catch native panics when executing the wasm runtime (#3953) As with the native runtime, we now catch all native panics when we execute the wasm runtime. The panics inside the wasm runtime were already catched before by the wasm executor automatically, but any panic in the host functions could bring down the node. The recent switch to execute the native counterpart of the host function in `sr-io`, makes this change required. The native `sr-io` functions just `panic` when something is not provided or any other error occured. --- core/executor/src/host_interface.rs | 107 +++++---------------------- core/executor/src/native_executor.rs | 69 +++++++++++++---- core/executor/src/wasm_runtime.rs | 27 +++++-- core/executor/src/wasmi_execution.rs | 13 +++- core/wasm-interface/src/lib.rs | 2 +- 5 files changed, 102 insertions(+), 116 deletions(-) diff --git a/core/executor/src/host_interface.rs b/core/executor/src/host_interface.rs index 7d87d93333fd2..4d1515d76999b 100644 --- a/core/executor/src/host_interface.rs +++ b/core/executor/src/host_interface.rs @@ -18,10 +18,8 @@ //! //! These are the host functions callable from within the Substrate runtime. -use crate::error::{Error, Result}; - use codec::Encode; -use std::{convert::TryFrom, str, panic}; +use std::{convert::TryFrom, str}; use primitives::{ blake2_128, blake2_256, twox_64, twox_128, twox_256, ed25519, sr25519, Blake2Hasher, Pair, crypto::KeyTypeId, offchain, @@ -211,10 +209,7 @@ impl_wasm_host_interface! { .map_err(|_| "Invalid attempt to determine key in ext_set_storage")?; let value = context.read_memory(value_data, value_len) .map_err(|_| "Invalid attempt to determine value in ext_set_storage")?; - with_external_storage(move || - Ok(runtime_io::set_storage(&key, &value)) - )?; - Ok(()) + Ok(runtime_io::set_storage(&key, &value)) } ext_set_child_storage( @@ -232,10 +227,7 @@ impl_wasm_host_interface! { let value = context.read_memory(value_data, value_len) .map_err(|_| "Invalid attempt to determine value in ext_set_child_storage")?; - with_external_storage(move || - Ok(runtime_io::set_child_storage(&storage_key, &key, &value)) - )?; - Ok(()) + Ok(runtime_io::set_child_storage(&storage_key, &key, &value)) } ext_clear_child_storage( @@ -249,27 +241,19 @@ impl_wasm_host_interface! { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to determine key in ext_clear_child_storage")?; - with_external_storage(move || - Ok(runtime_io::clear_child_storage(&storage_key, &key)) - )?; - Ok(()) + Ok(runtime_io::clear_child_storage(&storage_key, &key)) } ext_clear_storage(key_data: Pointer, key_len: WordSize) { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to determine key in ext_clear_storage")?; - with_external_storage(move || - Ok(runtime_io::clear_storage(&key)) - )?; - Ok(()) + Ok(runtime_io::clear_storage(&key)) } ext_exists_storage(key_data: Pointer, key_len: WordSize) -> u32 { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to determine key in ext_exists_storage")?; - with_external_storage(move || - Ok(if runtime_io::exists_storage(&key) { 1 } else { 0 }) - ) + Ok(if runtime_io::exists_storage(&key) { 1 } else { 0 }) } ext_exists_child_storage( @@ -283,18 +267,13 @@ impl_wasm_host_interface! { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to determine key in ext_exists_child_storage")?; - with_external_storage(move || - Ok(if runtime_io::exists_child_storage(&storage_key, &key) { 1 } else { 0 }) - ) + Ok(if runtime_io::exists_child_storage(&storage_key, &key) { 1 } else { 0 }) } ext_clear_prefix(prefix_data: Pointer, prefix_len: WordSize) { let prefix = context.read_memory(prefix_data, prefix_len) .map_err(|_| "Invalid attempt to determine prefix in ext_clear_prefix")?; - with_external_storage(move || - Ok(runtime_io::clear_prefix(&prefix)) - )?; - Ok(()) + Ok(runtime_io::clear_prefix(&prefix)) } ext_clear_child_prefix( @@ -307,21 +286,13 @@ impl_wasm_host_interface! { .map_err(|_| "Invalid attempt to determine storage_key in ext_clear_child_prefix")?; let prefix = context.read_memory(prefix_data, prefix_len) .map_err(|_| "Invalid attempt to determine prefix in ext_clear_child_prefix")?; - with_external_storage(move || - Ok(runtime_io::clear_child_prefix(&storage_key, &prefix)) - )?; - - Ok(()) + Ok(runtime_io::clear_child_prefix(&storage_key, &prefix)) } ext_kill_child_storage(storage_key_data: Pointer, storage_key_len: WordSize) { let storage_key = context.read_memory(storage_key_data, storage_key_len) .map_err(|_| "Invalid attempt to determine storage_key in ext_kill_child_storage")?; - with_external_storage(move || - Ok(runtime_io::kill_child_storage(&storage_key)) - )?; - - Ok(()) + Ok(runtime_io::kill_child_storage(&storage_key)) } ext_get_allocated_storage( @@ -331,11 +302,8 @@ impl_wasm_host_interface! { ) -> Pointer { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to determine key in ext_get_allocated_storage")?; - let maybe_value = with_external_storage(move || - Ok(runtime_io::storage(&key)) - )?; - if let Some(value) = maybe_value { + if let Some(value) = runtime_io::storage(&key) { let offset = context.allocate_memory(value.len() as u32)?; context.write_memory(offset, &value) .map_err(|_| "Invalid attempt to set memory in ext_get_allocated_storage")?; @@ -361,11 +329,7 @@ impl_wasm_host_interface! { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to determine key in ext_get_allocated_child_storage")?; - let maybe_value = with_external_storage(move || - Ok(runtime_io::child_storage(&storage_key, &key)) - )?; - - if let Some(value) = maybe_value { + if let Some(value) = runtime_io::child_storage(&storage_key, &key) { let offset = context.allocate_memory(value.len() as u32)?; context.write_memory(offset, &value) .map_err(|_| "Invalid attempt to set memory in ext_get_allocated_child_storage")?; @@ -388,11 +352,8 @@ impl_wasm_host_interface! { ) -> WordSize { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to get key in ext_get_storage_into")?; - let maybe_value = with_external_storage(move || - Ok(runtime_io::storage(&key)) - )?; - if let Some(value) = maybe_value { + if let Some(value) = runtime_io::storage(&key) { let data = &value[value.len().min(value_offset as usize)..]; let written = std::cmp::min(value_len as usize, data.len()); context.write_memory(value_data, &data[..written]) @@ -417,11 +378,7 @@ impl_wasm_host_interface! { let key = context.read_memory(key_data, key_len) .map_err(|_| "Invalid attempt to get key in ext_get_child_storage_into")?; - let maybe_value = with_external_storage(move || - Ok(runtime_io::child_storage(&storage_key, &key)) - )?; - - if let Some(value) = maybe_value { + if let Some(value) = runtime_io::child_storage(&storage_key, &key) { let data = &value[value.len().min(value_offset as usize)..]; let written = std::cmp::min(value_len as usize, data.len()); context.write_memory(value_data, &data[..written]) @@ -433,12 +390,8 @@ impl_wasm_host_interface! { } ext_storage_root(result: Pointer) { - let r = with_external_storage(move || - Ok(runtime_io::storage_root()) - )?; - context.write_memory(result, r.as_ref()) - .map_err(|_| "Invalid attempt to set memory in ext_storage_root")?; - Ok(()) + context.write_memory(result, runtime_io::storage_root().as_ref()) + .map_err(|_| "Invalid attempt to set memory in ext_storage_root".into()) } ext_child_storage_root( @@ -448,9 +401,7 @@ impl_wasm_host_interface! { ) -> Pointer { let storage_key = context.read_memory(storage_key_data, storage_key_len) .map_err(|_| "Invalid attempt to determine storage_key in ext_child_storage_root")?; - let value = with_external_storage(move || - Ok(runtime_io::child_storage_root(&storage_key)) - )?; + let value = runtime_io::child_storage_root(&storage_key); let offset = context.allocate_memory(value.len() as u32)?; context.write_memory(offset, &value) @@ -468,11 +419,8 @@ impl_wasm_host_interface! { let mut parent_hash = [0u8; 32]; context.read_memory_into(parent_hash_data, &mut parent_hash[..]) .map_err(|_| "Invalid attempt to get parent_hash in ext_storage_changes_root")?; - let r = with_external_storage(move || - Ok(runtime_io::storage_changes_root(parent_hash)) - )?; - if let Some(r) = r { + if let Some(r) = runtime_io::storage_changes_root(parent_hash) { context.write_memory(result, &r[..]) .map_err(|_| "Invalid attempt to set memory in ext_storage_changes_root")?; Ok(1) @@ -1146,25 +1094,6 @@ impl ReadPrimitive for &mut dyn FunctionContext { } } -/// Execute closure that access external storage. -/// -/// All panics that happen within closure are captured and transformed into -/// runtime error. This requires special panic handler mode to be enabled -/// during the call (see `panic_handler::AbortGuard::never_abort`). -/// If this mode isn't enabled, then all panics within externalities are -/// leading to process abort. -fn with_external_storage(f: F) -> std::result::Result - where - F: panic::UnwindSafe + FnOnce() -> Result -{ - // it is safe beause basic methods of StorageExternalities are guaranteed to touch only - // its internal state + we should discard it on error - panic::catch_unwind(move || f()) - .map_err(|_| Error::Runtime) - .and_then(|result| result) - .map_err(|err| format!("{}", err)) -} - fn deadline_to_timestamp(deadline: u64) -> Option { if deadline == 0 { None diff --git a/core/executor/src/native_executor.rs b/core/executor/src/native_executor.rs index 7323703ed9656..082f0ba2adc33 100644 --- a/core/executor/src/native_executor.rs +++ b/core/executor/src/native_executor.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use std::{result, cell::RefCell, panic::UnwindSafe}; +use std::{result, cell::RefCell, panic::{UnwindSafe, AssertUnwindSafe}}; use crate::error::{Error, Result}; use crate::wasm_runtime::{RuntimesCache, WasmExecutionMethod, WasmRuntime}; use crate::RuntimeInfo; @@ -30,7 +30,7 @@ thread_local! { /// Default num of pages for the heap const DEFAULT_HEAP_PAGES: u64 = 1024; -fn safe_call(f: F) -> Result +pub(crate) fn safe_call(f: F) -> Result where F: UnwindSafe + FnOnce() -> U { // Substrate uses custom panic hook that terminates process on panic. Disable termination for the native call. @@ -65,7 +65,7 @@ pub trait NativeExecutionDispatch: Send + Sync { #[derive(Debug)] pub struct NativeExecutor { /// Dummy field to avoid the compiler complaining about us not using `D`. - _dummy: ::std::marker::PhantomData, + _dummy: std::marker::PhantomData, /// Method used to execute fallback Wasm code. fallback_method: WasmExecutionMethod, /// Native runtime version info. @@ -92,15 +92,45 @@ impl NativeExecutor { } } + /// Execute the given closure `f` with the latest runtime (based on the `CODE` key in `ext`). + /// + /// The closure `f` is expected to return `Err(_)` when there happened a `panic!` in native code + /// while executing the runtime in Wasm. If a `panic!` occurred, the runtime is invalidated to + /// prevent any poisoned state. Native runtime execution does not need to report back + /// any `panic!`. + /// + /// # Safety + /// + /// `runtime` and `ext` are given as `AssertUnwindSafe` to the closure. As described above, the + /// runtime is invalidated on any `panic!` to prevent a poisoned state. `ext` is already + /// implicitly handled as unwind safe, as we store it in a global variable while executing the + /// native runtime. fn with_runtime( &self, ext: &mut E, - f: impl for <'a> FnOnce(&'a mut dyn WasmRuntime, &'a mut E) -> Result, + f: impl for<'a> FnOnce( + AssertUnwindSafe<&'a mut (dyn WasmRuntime + 'static)>, + AssertUnwindSafe<&'a mut E>, + ) -> Result>, ) -> Result where E: Externalities { RUNTIMES_CACHE.with(|cache| { let mut cache = cache.borrow_mut(); - let runtime = cache.fetch_runtime(ext, self.fallback_method, self.default_heap_pages)?; - f(runtime, ext) + let (runtime, code_hash) = cache.fetch_runtime( + ext, + self.fallback_method, + self.default_heap_pages, + )?; + + let runtime = AssertUnwindSafe(runtime); + let ext = AssertUnwindSafe(ext); + + match f(runtime, ext) { + Ok(res) => res, + Err(e) => { + cache.invalidate_runtime(self.fallback_method, code_hash); + Err(e) + } + } }) } } @@ -125,7 +155,7 @@ impl RuntimeInfo for NativeExecutor { &self, ext: &mut E, ) -> Option { - match self.with_runtime(ext, |runtime, _ext| Ok(runtime.version())) { + match self.with_runtime(ext, |runtime, _ext| Ok(Ok(runtime.version()))) { Ok(version) => version, Err(e) => { warn!(target: "executor", "Failed to fetch runtime: {:?}", e); @@ -141,8 +171,8 @@ impl CodeExecutor for NativeExecutor { fn call < E: Externalities, - R:Decode + Encode + PartialEq, - NC: FnOnce() -> result::Result + UnwindSafe + R: Decode + Encode + PartialEq, + NC: FnOnce() -> result::Result + UnwindSafe, >( &self, ext: &mut E, @@ -152,7 +182,7 @@ impl CodeExecutor for NativeExecutor { native_call: Option, ) -> (Result>, bool){ let mut used_native = false; - let result = self.with_runtime(ext, |runtime, ext| { + let result = self.with_runtime(ext, |mut runtime, mut ext| { let onchain_version = runtime.version(); match ( use_native, @@ -170,9 +200,16 @@ impl CodeExecutor for NativeExecutor { .as_ref() .map_or_else(||"".into(), |v| format!("{}", v)) ); - runtime.call(ext, method, data).map(NativeOrEncoded::Encoded) + + safe_call( + move || runtime.call(&mut **ext, method, data).map(NativeOrEncoded::Encoded) + ) } - (false, _, _) => runtime.call(ext, method, data).map(NativeOrEncoded::Encoded), + (false, _, _) => { + safe_call( + move || runtime.call(&mut **ext, method, data).map(NativeOrEncoded::Encoded) + ) + }, (true, true, Some(call)) => { trace!( target: "executor", @@ -184,11 +221,13 @@ impl CodeExecutor for NativeExecutor { ); used_native = true; - with_native_environment(ext, move || (call)()) + let res = with_native_environment(&mut **ext, move || (call)()) .and_then(|r| r .map(NativeOrEncoded::Native) .map_err(|s| Error::ApiError(s.to_string())) - ) + ); + + Ok(res) } _ => { trace!( @@ -199,7 +238,7 @@ impl CodeExecutor for NativeExecutor { ); used_native = true; - D::dispatch(ext, method, data).map(NativeOrEncoded::Encoded) + Ok(D::dispatch(&mut **ext, method, data).map(NativeOrEncoded::Encoded)) } } }); diff --git a/core/executor/src/wasm_runtime.rs b/core/executor/src/wasm_runtime.rs index 8d2291fe04893..29a6c51e39d3b 100644 --- a/core/executor/src/wasm_runtime.rs +++ b/core/executor/src/wasm_runtime.rs @@ -23,7 +23,7 @@ use crate::error::{Error, WasmError}; use crate::wasmi_execution; use log::{trace, warn}; use codec::Decode; -use primitives::{storage::well_known_keys, traits::Externalities}; +use primitives::{storage::well_known_keys, traits::Externalities, H256}; use runtime_version::RuntimeVersion; use std::{collections::hash_map::{Entry, HashMap}}; @@ -99,9 +99,8 @@ impl RuntimesCache { /// /// # Return value /// - /// If no error occurred a tuple `(wasmi::ModuleRef, Option)` is - /// returned. `RuntimeVersion` is contained if the call to `Core_version` returned - /// a version. + /// If no error occurred a tuple `(&mut WasmRuntime, H256)` is + /// returned. `H256` is the hash of the runtime code. /// /// In case of failure one of two errors can be returned: /// @@ -114,7 +113,7 @@ impl RuntimesCache { ext: &mut E, wasm_method: WasmExecutionMethod, default_heap_pages: u64, - ) -> Result<&mut (dyn WasmRuntime + 'static), Error> { + ) -> Result<(&mut (dyn WasmRuntime + 'static), H256), Error> { let code_hash = ext .original_storage_hash(well_known_keys::CODE) .ok_or(Error::InvalidCode("`CODE` not found in storage.".into()))?; @@ -131,7 +130,7 @@ impl RuntimesCache { if !cached_runtime.update_heap_pages(heap_pages) { trace!( target: "runtimes_cache", - "heap_pages were changed. Reinstantiating the instance" + "heap_pages were changed. Reinstantiating the instance", ); *result = create_wasm_runtime(ext, wasm_method, heap_pages); if let Err(ref err) = result { @@ -152,9 +151,23 @@ impl RuntimesCache { }; result.as_mut() - .map(|runtime| runtime.as_mut()) + .map(|runtime| (runtime.as_mut(), code_hash)) .map_err(|ref e| Error::InvalidCode(format!("{:?}", e))) } + + /// Invalidate the runtime for the given `wasm_method` and `code_hash`. + /// + /// Invalidation of a runtime is useful when there was a `panic!` in native while executing it. + /// The `panic!` maybe have brought the runtime into a poisoned state and so, it is better to + /// invalidate this runtime instance. + pub fn invalidate_runtime( + &mut self, + wasm_method: WasmExecutionMethod, + code_hash: H256, + ) { + // Just remove the instance, it will be re-created the next time it is requested. + self.instances.remove(&(wasm_method, code_hash.into())); + } } /// Create a wasm runtime with the given `code`. diff --git a/core/executor/src/wasmi_execution.rs b/core/executor/src/wasmi_execution.rs index 2b832b490641d..a83729b4b6c8d 100644 --- a/core/executor/src/wasmi_execution.rs +++ b/core/executor/src/wasmi_execution.rs @@ -16,7 +16,7 @@ //! Implementation of a Wasm runtime using the Wasmi interpreter. -use std::{str, mem}; +use std::{str, mem, panic::AssertUnwindSafe}; use wasmi::{ Module, ModuleInstance, MemoryInstance, MemoryRef, TableRef, ImportsBuilder, ModuleRef, memory_units::Pages, RuntimeValue::{I32, I64, self}, @@ -625,9 +625,14 @@ pub fn create_instance(ext: &mut E, code: &[u8], heap_pages: u ", ); - let version = call_in_wasm_module(ext, &instance, "Core_version", &[]) - .ok() - .and_then(|v| RuntimeVersion::decode(&mut v.as_slice()).ok()); + let mut ext = AssertUnwindSafe(ext); + let call_instance = AssertUnwindSafe(&instance); + let version = crate::native_executor::safe_call( + move || call_in_wasm_module(&mut **ext, *call_instance, "Core_version", &[]) + .ok() + .and_then(|v| RuntimeVersion::decode(&mut v.as_slice()).ok()) + ).map_err(WasmError::Instantiation)?; + Ok(WasmiRuntime { instance, version, diff --git a/core/wasm-interface/src/lib.rs b/core/wasm-interface/src/lib.rs index b3cbde556eea0..1e67260638900 100644 --- a/core/wasm-interface/src/lib.rs +++ b/core/wasm-interface/src/lib.rs @@ -180,7 +180,7 @@ pub trait Function { fn execute( &self, context: &mut dyn FunctionContext, - args: &mut dyn Iterator, + args: &mut dyn Iterator, ) -> Result>; }