Skip to content

Commit

Permalink
Catch native panics when executing the wasm runtime (paritytech#3953)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bkchr
bkchr committed Oct 30, 2019
1 parent 155c217 commit 1fa9b54
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 116 deletions.
107 changes: 18 additions & 89 deletions core/executor/src/host_interface.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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<u8>, 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<u8>, 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(
Expand All @@ -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<u8>, 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(
Expand All @@ -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<u8>, 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(
Expand All @@ -331,11 +302,8 @@ impl_wasm_host_interface! {
) -> Pointer<u8> {
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")?;
Expand All @@ -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")?;
Expand All @@ -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])
Expand All @@ -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])
Expand All @@ -433,12 +390,8 @@ impl_wasm_host_interface! {
}

ext_storage_root(result: Pointer<u8>) {
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(
Expand All @@ -448,9 +401,7 @@ impl_wasm_host_interface! {
) -> Pointer<u8> {
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)
Expand All @@ -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)
Expand Down Expand Up @@ -1146,25 +1094,6 @@ impl ReadPrimitive<u32> 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<T, F>(f: F) -> std::result::Result<T, String>
where
F: panic::UnwindSafe + FnOnce() -> Result<T>
{
// 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<offchain::Timestamp> {
if deadline == 0 {
None
Expand Down
69 changes: 54 additions & 15 deletions core/executor/src/native_executor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.

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;
Expand All @@ -30,7 +30,7 @@ thread_local! {
/// Default num of pages for the heap
const DEFAULT_HEAP_PAGES: u64 = 1024;

fn safe_call<F, U>(f: F) -> Result<U>
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.
Expand Down Expand Up @@ -65,7 +65,7 @@ pub trait NativeExecutionDispatch: Send + Sync {
#[derive(Debug)]
pub struct NativeExecutor<D> {
/// Dummy field to avoid the compiler complaining about us not using `D`.
_dummy: ::std::marker::PhantomData<D>,
_dummy: std::marker::PhantomData<D>,
/// Method used to execute fallback Wasm code.
fallback_method: WasmExecutionMethod,
/// Native runtime version info.
Expand All @@ -92,15 +92,45 @@ impl<D: NativeExecutionDispatch> NativeExecutor<D> {
}
}

/// 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<E, R>(
&self,
ext: &mut E,
f: impl for <'a> FnOnce(&'a mut dyn WasmRuntime, &'a mut E) -> Result<R>,
f: impl for<'a> FnOnce(
AssertUnwindSafe<&'a mut (dyn WasmRuntime + 'static)>,
AssertUnwindSafe<&'a mut E>,
) -> Result<Result<R>>,
) -> Result<R> 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)
}
}
})
}
}
Expand All @@ -125,7 +155,7 @@ impl<D: NativeExecutionDispatch> RuntimeInfo for NativeExecutor<D> {
&self,
ext: &mut E,
) -> Option<RuntimeVersion> {
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);
Expand All @@ -141,8 +171,8 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
fn call
<
E: Externalities,
R:Decode + Encode + PartialEq,
NC: FnOnce() -> result::Result<R, String> + UnwindSafe
R: Decode + Encode + PartialEq,
NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
>(
&self,
ext: &mut E,
Expand All @@ -152,7 +182,7 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
native_call: Option<NC>,
) -> (Result<NativeOrEncoded<R>>, 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,
Expand All @@ -170,9 +200,16 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
.as_ref()
.map_or_else(||"<None>".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",
Expand All @@ -184,11 +221,13 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
);

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!(
Expand All @@ -199,7 +238,7 @@ impl<D: NativeExecutionDispatch> CodeExecutor for NativeExecutor<D> {
);

used_native = true;
D::dispatch(ext, method, data).map(NativeOrEncoded::Encoded)
Ok(D::dispatch(&mut **ext, method, data).map(NativeOrEncoded::Encoded))
}
}
});
Expand Down
Loading

0 comments on commit 1fa9b54

Please sign in to comment.