Never panic during execution proof check (#3504)

* do not panic during execution proof check

* Update core/state-machine/src/lib.rs

Co-Authored-By: DemiMarie-parity <[email protected]>

* Update core/state-machine/src/lib.rs

Co-Authored-By: DemiMarie-parity <[email protected]>

* Update core/state-machine/src/lib.rs

Co-Authored-By: DemiMarie-parity <[email protected]>

* BackendTrustLevel enum

* up runtime version

* Update core/state-machine/src/lib.rs

Co-Authored-By: DemiMarie-parity <[email protected]>

* Update core/state-machine/src/lib.rs

Co-Authored-By: DemiMarie-parity <[email protected]>

* fixed some grumbles

* Update core/state-machine/src/testing.rs

Co-Authored-By: Gavin Wood <[email protected]>

* Update core/state-machine/src/lib.rs

Co-Authored-By: Gavin Wood <[email protected]>

* mov where

* spaces -> tabs (to restart build)

Author: svyatonik

Cargo.lock

@@ -33,6 +33,7 @@ env_logger = "0.6"
 tempfile = "3.1"
 test-client = { package = "substrate-test-runtime-client", path = "../test-runtime/client" }
 kvdb-memorydb = { git = "https://github.com/paritytech/parity-common", rev="b0317f649ab2c665b7987b8475878fc4d2e1f81d" }
+panic-handler = { package = "substrate-panic-handler", path = "../panic-handler" }
 
 [features]
 default = ["std"]

core/client/Cargo.toml

@@ -43,7 +43,7 @@ use state_machine::{
 	DBValue, Backend as StateBackend, ChangesTrieAnchorBlockId, ExecutionStrategy, ExecutionManager,
 	prove_read, prove_child_read, ChangesTrieRootsStorage, ChangesTrieStorage,
 	ChangesTrieTransaction, ChangesTrieConfigurationRange, key_changes, key_changes_proof,
-	OverlayedChanges,
+	OverlayedChanges, BackendTrustLevel,
 };
 use executor::{RuntimeVersion, RuntimeInfo};
 use consensus::{
@@ -1043,7 +1043,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 				let get_execution_manager = |execution_strategy: ExecutionStrategy| {
 					match execution_strategy {
 						ExecutionStrategy::NativeElseWasm => ExecutionManager::NativeElseWasm,
-						ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm,
+						ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm(BackendTrustLevel::Trusted),
 						ExecutionStrategy::NativeWhenPossible => ExecutionManager::NativeWhenPossible,
 						ExecutionStrategy::Both => ExecutionManager::Both(|wasm_result, native_result| {
 							let header = import_headers.post();

core/client/src/client.rs

@@ -460,26 +460,46 @@ pub fn check_execution_proof<Header, E, H>(
 		E: CodeExecutor<H>,
 		H: Hasher,
 		H::Out: Ord + 'static,
+{
+	check_execution_proof_with_make_header(
+		executor,
+		request,
+		remote_proof,
+		|header| <Header as HeaderT>::new(
+			*header.number() + One::one(),
+			Default::default(),
+			Default::default(),
+			header.hash(),
+			Default::default(),
+		),
+	)
+}
+
+fn check_execution_proof_with_make_header<Header, E, H, MakeNextHeader: Fn(&Header) -> Header>(
+	executor: &E,
+	request: &RemoteCallRequest<Header>,
+	remote_proof: Vec<Vec<u8>>,
+	make_next_header: MakeNextHeader,
+) -> ClientResult<Vec<u8>>
+	where
+		Header: HeaderT,
+		E: CodeExecutor<H>,
+		H: Hasher,
+		H::Out: Ord + 'static,
 {
 	let local_state_root = request.header.state_root();
 	let root: H::Out = convert_hash(&local_state_root);
 
 	// prepare execution environment + check preparation proof
 	let mut changes = OverlayedChanges::default();
 	let trie_backend = create_proof_check_backend(root, remote_proof)?;
-	let next_block = <Header as HeaderT>::new(
-		*request.header.number() + One::one(),
-		Default::default(),
-		Default::default(),
-		request.header.hash(),
-		Default::default(),
-	);
+	let next_header = make_next_header(&request.header);
 	execution_proof_check_on_trie_backend::<H, _>(
 		&trie_backend,
 		&mut changes,
 		executor,
 		"Core_initialize_block",
-		&next_block.encode(),
+		&next_header.encode(),
 		None,
 	)?;
 
@@ -530,6 +550,43 @@ mod tests {
 			(remote_result, local_result)
 		}
 
+		fn execute_with_proof_failure(remote_client: &TestClient, at: u64, method: &'static str) {
+			let remote_block_id = BlockId::Number(at);
+			let remote_header = remote_client.header(&remote_block_id).unwrap().unwrap();
+
+			// 'fetch' execution proof from remote node
+			let (_, remote_execution_proof) = remote_client.execution_proof(
+				&remote_block_id,
+				method,
+				&[]
+			).unwrap();
+
+			// check remote execution proof locally
+			let local_executor = NativeExecutor::<test_client::LocalExecutor>::new(None);
+			let execution_result = check_execution_proof_with_make_header(
+				&local_executor,
+				&RemoteCallRequest {
+					block: test_client::runtime::Hash::default(),
+					header: remote_header,
+					method: method.into(),
+					call_data: vec![],
+					retry_count: None,
+				},
+				remote_execution_proof,
+				|header| <Header as HeaderT>::new(
+					at + 1,
+					Default::default(),
+					Default::default(),
+					header.hash(),
+					header.digest().clone(), // this makes next header wrong
+				),
+			);
+			match execution_result {
+				Err(crate::error::Error::Execution(_)) => (),
+				_ => panic!("Unexpected execution result: {:?}", execution_result),
+			}
+		}
+
 		// prepare remote client
 		let remote_client = test_client::new();
 		for i in 1u32..3u32 {
@@ -546,15 +603,24 @@ mod tests {
 		let (remote, local) = execute(&remote_client, 0, "Core_version");
 		assert_eq!(remote, local);
 
+		let (remote, local) = execute(&remote_client, 2, "Core_version");
+		assert_eq!(remote, local);
+
 		// check method that requires environment
 		let (_, block) = execute(&remote_client, 0, "BlockBuilder_finalize_block");
 		let local_block: Header = Decode::decode(&mut &block[..]).unwrap();
 		assert_eq!(local_block.number, 1);
 
-		// check method that requires environment
 		let (_, block) = execute(&remote_client, 2, "BlockBuilder_finalize_block");
 		let local_block: Header = Decode::decode(&mut &block[..]).unwrap();
 		assert_eq!(local_block.number, 3);
+
+		// check that proof check doesn't panic even if proof is incorrect AND no panic handler is set
+		execute_with_proof_failure(&remote_client, 2, "Core_version");
+
+		// check that proof check doesn't panic even if proof is incorrect AND panic handler is set
+		panic_handler::set("TEST");
+		execute_with_proof_failure(&remote_client, 2, "Core_version");
 	}
 
 	#[test]

core/client/src/light/call_executor.rs

@@ -806,7 +806,7 @@ mod tests {
 			keystore.write().insert_ephemeral_from_seed::<AuthorityPair>(&key.to_seed())
 				.expect("Creates authority key");
 			keystore_paths.push(keystore_path);
-			
+
 			let environ = DummyFactory(client.clone());
 			import_notifications.push(
 				client.import_notification_stream()

core/consensus/aura/src/lib.rs

@@ -207,7 +207,7 @@ fn run_one_test() {
 		let peer = net.peer(*peer_id);
 		let client = peer.client().as_full().expect("Only full clients are used in tests").clone();
 		let select_chain = peer.select_chain().expect("Full client has select_chain");
-		
+
 		let keystore_path = tempfile::tempdir().expect("Creates keystore path");
 		let keystore = keystore::Store::open(keystore_path.path(), None).expect("Creates keystore");
 		keystore.write().insert_ephemeral_from_seed::<AuthorityPair>(seed).expect("Generates authority key");

core/consensus/babe/src/tests.rs

@@ -19,7 +19,7 @@
 //! This module defines and implements the wasm part of Substrate Host Interface and provides
 //! an interface for calling into the wasm runtime.
 
-use std::{convert::TryFrom, str};
+use std::{convert::TryFrom, str, panic};
 use tiny_keccak;
 use secp256k1;
 
@@ -31,8 +31,8 @@ use crate::error::{Error, Result};
 use codec::{Encode, Decode};
 use primitives::{
 	blake2_128, blake2_256, twox_64, twox_128, twox_256, ed25519, sr25519, Pair, crypto::KeyTypeId,
-	offchain, hexdisplay::HexDisplay, sandbox as sandbox_primitives, H256, Blake2Hasher,
-	traits::Externalities, child_storage_key::ChildStorageKey,
+	offchain, hexdisplay::HexDisplay, sandbox as sandbox_primitives, Blake2Hasher,
+	traits::Externalities,
 };
 use trie::{TrieConfiguration, trie_types::Layout};
 use crate::sandbox;
@@ -447,7 +447,9 @@ 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")?;
-			runtime_io::set_storage(&key, &value);
+			with_external_storage(move ||
+				Ok(runtime_io::set_storage(&key, &value))
+			)?;
 			Ok(())
 		}
 
@@ -466,7 +468,9 @@ 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")?;
 
-			runtime_io::set_child_storage(&storage_key, &key, &value);
+			with_external_storage(move ||
+				Ok(runtime_io::set_child_storage(&storage_key, &key, &value))
+			)?;
 			Ok(())
 		}
 
@@ -481,21 +485,27 @@ 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")?;
 
-			runtime_io::clear_child_storage(&storage_key, &key);
+			with_external_storage(move ||
+				Ok(runtime_io::clear_child_storage(&storage_key, &key))
+			)?;
 			Ok(())
 		}
 
 		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")?;
-			runtime_io::clear_storage(&key);
+			with_external_storage(move ||
+				Ok(runtime_io::clear_storage(&key))
+			)?;
 			Ok(())
 		}
 
 		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")?;
-			Ok(if runtime_io::exists_storage(&key) { 1 } else { 0 })
+			with_external_storage(move ||
+				Ok(if runtime_io::exists_storage(&key) { 1 } else { 0 })
+			)
 		}
 
 		ext_exists_child_storage(
@@ -509,13 +519,17 @@ 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")?;
 
-			Ok(if runtime_io::exists_child_storage(&storage_key, &key) { 1 } else { 0 })
+			with_external_storage(move ||
+				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")?;
-			runtime_io::clear_prefix(&prefix);
+			with_external_storage(move ||
+				Ok(runtime_io::clear_prefix(&prefix))
+			)?;
 			Ok(())
 		}
 
@@ -529,15 +543,19 @@ 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")?;
-			runtime_io::clear_child_prefix(&storage_key, &prefix);
+			with_external_storage(move ||
+				Ok(runtime_io::clear_child_prefix(&storage_key, &prefix))
+			)?;
 
 			Ok(())
 		}
 
 		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")?;
-			runtime_io::kill_child_storage(&storage_key);
+			with_external_storage(move ||
+				Ok(runtime_io::kill_child_storage(&storage_key))
+			)?;
 
 			Ok(())
 		}
@@ -549,7 +567,9 @@ 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 = runtime_io::storage(&key);
+			let maybe_value = with_external_storage(move ||
+				Ok(runtime_io::storage(&key))
+			)?;
 
 			if let Some(value) = maybe_value {
 				let offset = context.allocate_memory(value.len() as u32)?;
@@ -577,7 +597,9 @@ 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 = runtime_io::child_storage(&storage_key, &key);
+			let maybe_value = with_external_storage(move ||
+				Ok(runtime_io::child_storage(&storage_key, &key))
+			)?;
 
 			if let Some(value) = maybe_value {
 				let offset = context.allocate_memory(value.len() as u32)?;
@@ -602,7 +624,9 @@ 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 = runtime_io::storage(&key);
+			let maybe_value = with_external_storage(move ||
+				Ok(runtime_io::storage(&key))
+			)?;
 
 			if let Some(value) = maybe_value {
 				let value = &value[value_offset as usize..];
@@ -629,7 +653,9 @@ 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 = runtime_io::child_storage(&storage_key, &key);
+			let maybe_value = with_external_storage(move ||
+				Ok(runtime_io::child_storage(&storage_key, &key))
+			)?;
 
 			if let Some(value) = maybe_value {
 				let value = &value[value_offset as usize..];
@@ -643,7 +669,9 @@ impl_wasm_host_interface! {
 		}
 
 		ext_storage_root(result: Pointer<u8>) {
-			let r = runtime_io::storage_root();
+			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(())
@@ -656,7 +684,9 @@ 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 = runtime_io::child_storage_root(&storage_key);
+			let value = with_external_storage(move ||
+				Ok(runtime_io::child_storage_root(&storage_key))
+			)?;
 
 			let offset = context.allocate_memory(value.len() as u32)?;
 			context.write_memory(offset, &value)
@@ -674,7 +704,9 @@ 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 = runtime_io::storage_changes_root(parent_hash);
+			let r = with_external_storage(move ||
+				Ok(runtime_io::storage_changes_root(parent_hash))
+			)?;
 
 			if let Some(r) = r {
 				context.write_memory(result, &r[..])
@@ -1331,6 +1363,25 @@ impl_wasm_host_interface! {
 	}
 }
 
+/// 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))
+}
+
 /// Wasm rust executor for contracts.
 ///
 /// Executes the provided code in a sandboxed wasm runtime.