From 6c341f4e9de78f31f1d7e01f15764de6fbc2e194 Mon Sep 17 00:00:00 2001 From: Arkadiy Paronyan Date: Mon, 4 Feb 2019 09:11:29 +0100 Subject: [PATCH] Fixed panic handling (#1667) --- substrate/Cargo.lock | 12 +++- substrate/Cargo.toml | 1 + substrate/core/cli/Cargo.toml | 2 +- substrate/core/cli/src/lib.rs | 5 +- substrate/core/executor/Cargo.toml | 1 + .../core/executor/src/native_executor.rs | 8 +-- substrate/core/panic-handler/Cargo.toml | 10 ++++ .../src/lib.rs} | 59 ++++++++++++++++--- substrate/core/state-machine/Cargo.toml | 1 + substrate/core/state-machine/src/ext.rs | 12 ++++ substrate/node/src/main.rs | 1 + 11 files changed, 94 insertions(+), 18 deletions(-) create mode 100644 substrate/core/panic-handler/Cargo.toml rename substrate/core/{cli/src/panic_hook.rs => panic-handler/src/lib.rs} (61%) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 535a77a056..78f1add8c1 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -3433,7 +3433,6 @@ dependencies = [ "ansi_term 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)", "app_dirs 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "atty 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", - "backtrace 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)", "clap 2.32.0 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", "error-chain 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3449,6 +3448,7 @@ dependencies = [ "structopt 0.2.14 (registry+https://github.com/rust-lang/crates.io-index)", "substrate-client 0.1.0", "substrate-network 0.1.0", + "substrate-panic-handler 0.1.0", "substrate-primitives 0.1.0", "substrate-service 0.3.0", "substrate-telemetry 0.3.0", @@ -3618,6 +3618,7 @@ dependencies = [ "serde_derive 1.0.85 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 0.1.0", "sr-version 0.1.0", + "substrate-panic-handler 0.1.0", "substrate-primitives 0.1.0", "substrate-serializer 0.1.0", "substrate-state-machine 0.1.0", @@ -3749,6 +3750,14 @@ dependencies = [ "void 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "substrate-panic-handler" +version = "0.1.0" +dependencies = [ + "backtrace 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)", + "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "substrate-primitives" version = "0.1.0" @@ -3898,6 +3907,7 @@ dependencies = [ "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "parity-codec 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "parking_lot 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", + "substrate-panic-handler 0.1.0", "substrate-primitives 0.1.0", "substrate-trie 0.4.0", "trie-db 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/substrate/Cargo.toml b/substrate/Cargo.toml index 6e6834068c..da5b83e85a 100644 --- a/substrate/Cargo.toml +++ b/substrate/Cargo.toml @@ -30,6 +30,7 @@ members = [ "core/finality-grandpa/primitives", "core/keyring", "core/network", + "core/panic-handler", "core/primitives", "core/rpc", "core/rpc-servers", diff --git a/substrate/core/cli/Cargo.toml b/substrate/core/cli/Cargo.toml index 1d3ef9254b..eaddd3f540 100644 --- a/substrate/core/cli/Cargo.toml +++ b/substrate/core/cli/Cargo.toml @@ -7,7 +7,6 @@ edition = "2018" [dependencies] clap = "~2.32" -backtrace = "0.3" env_logger = "0.6" error-chain = "0.12" log = "0.4" @@ -23,6 +22,7 @@ futures = "0.1.17" fdlimit = "0.1" exit-future = "0.1" sysinfo = "0.7" +panic-handler = { package = "substrate-panic-handler", path = "../../core/panic-handler" } client = { package = "substrate-client", path = "../../core/client" } network = { package = "substrate-network", path = "../../core/network" } runtime_primitives = { package = "sr-primitives", path = "../../core/sr-primitives" } diff --git a/substrate/core/cli/src/lib.rs b/substrate/core/cli/src/lib.rs index 3af64e0d38..7acc0f7099 100644 --- a/substrate/core/cli/src/lib.rs +++ b/substrate/core/cli/src/lib.rs @@ -24,7 +24,6 @@ mod traits; mod params; pub mod error; pub mod informant; -mod panic_hook; use runtime_primitives::traits::As; use service::{ @@ -76,6 +75,8 @@ pub struct VersionInfo { pub description: &'static str, /// Executable file author. pub author: &'static str, + /// Support URL. + pub support_url: &'static str, } /// Something that can be converted into an exit signal. @@ -189,7 +190,7 @@ where I: IntoIterator, T: Into + Clone, { - panic_hook::set(); + panic_handler::set(version.support_url); let full_version = service::config::full_version_from_strs( version.version, diff --git a/substrate/core/executor/Cargo.toml b/substrate/core/executor/Cargo.toml index d0e120c9fa..9de1ab5b2d 100644 --- a/substrate/core/executor/Cargo.toml +++ b/substrate/core/executor/Cargo.toml @@ -13,6 +13,7 @@ trie = { package = "substrate-trie", path = "../trie" } serializer = { package = "substrate-serializer", path = "../serializer" } state_machine = { package = "substrate-state-machine", path = "../state-machine" } runtime_version = { package = "sr-version", path = "../sr-version" } +panic-handler = { package = "substrate-panic-handler", path = "../panic-handler" } serde = "1.0" serde_derive = "1.0" wasmi = { version = "0.4.3" } diff --git a/substrate/core/executor/src/native_executor.rs b/substrate/core/executor/src/native_executor.rs index cd1ab3bca2..7f201de432 100644 --- a/substrate/core/executor/src/native_executor.rs +++ b/substrate/core/executor/src/native_executor.rs @@ -97,11 +97,9 @@ fn fetch_cached_runtime_version<'a, E: Externalities>( fn safe_call(f: F) -> Result where F: UnwindSafe + FnOnce() -> U { - // Substrate uses custom panic hook that terminates process on panic. Disable it for the native call. - let hook = ::std::panic::take_hook(); - let result = ::std::panic::catch_unwind(f).map_err(|_| ErrorKind::Runtime.into()); - ::std::panic::set_hook(hook); - result + // Substrate uses custom panic hook that terminates process on panic. Disable termination for the native call. + let _guard = panic_handler::AbortGuard::new(false); + ::std::panic::catch_unwind(f).map_err(|_| ErrorKind::Runtime.into()) } /// Set up the externalities and safe calling environment to execute calls to a native runtime. diff --git a/substrate/core/panic-handler/Cargo.toml b/substrate/core/panic-handler/Cargo.toml new file mode 100644 index 0000000000..fb870fd8a3 --- /dev/null +++ b/substrate/core/panic-handler/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "substrate-panic-handler" +version = "0.1.0" +authors = ["Parity Technologies "] +description = "Substrate panic handler." +edition = "2018" + +[dependencies] +backtrace = "0.3" +log = "0.4" diff --git a/substrate/core/cli/src/panic_hook.rs b/substrate/core/panic-handler/src/lib.rs similarity index 61% rename from substrate/core/cli/src/panic_hook.rs rename to substrate/core/panic-handler/src/lib.rs index 06227d1135..91f7c0af97 100644 --- a/substrate/core/cli/src/panic_hook.rs +++ b/substrate/core/panic-handler/src/lib.rs @@ -19,20 +19,51 @@ use backtrace::Backtrace; use std::io::{self, Write}; use std::panic::{self, PanicInfo}; +use std::cell::Cell; use std::thread; -/// Set the panic hook -pub fn set() { - panic::set_hook(Box::new(panic_hook)); +thread_local! { + pub static ABORT: Cell = Cell::new(true); } -static ABOUT_PANIC: &str = " +/// Set the panic hook +pub fn set(bug_url: &'static str) { + panic::set_hook(Box::new(move |c| panic_hook(c, bug_url))); +} + +macro_rules! ABOUT_PANIC { + () => (" This is a bug. Please report it at: - https://github.com/paritytech/polkadot/issues/new -"; + {} +")} -fn panic_hook(info: &PanicInfo) { +/// Set aborting flag. Returns previous value of the flag. +pub fn set_abort(enabled: bool) -> bool { + ABORT.with(|flag| { + let prev = flag.get(); + flag.set(enabled); + prev + }) +} + +/// Abort flag guard. Sets abort on construction and reverts to previous setting when dropped. +pub struct AbortGuard(bool); + +impl AbortGuard { + /// Create a new guard and set abort flag to specified value. + pub fn new(enable: bool) -> AbortGuard { + AbortGuard(set_abort(enable)) + } +} + +impl Drop for AbortGuard { + fn drop(&mut self) { + set_abort(self.0); + } +} + +fn panic_hook(info: &PanicInfo, report_url: &'static str) { let location = info.location(); let file = location.as_ref().map(|l| l.file()).unwrap_or(""); let line = location.as_ref().map(|l| l.line()).unwrap_or(0); @@ -63,7 +94,17 @@ fn panic_hook(info: &PanicInfo) { name, msg, file, line ); - let _ = writeln!(stderr, "{}", ABOUT_PANIC); - ::std::process::exit(1); + let _ = writeln!(stderr, ABOUT_PANIC!(), report_url); + ABORT.with(|flag| { + if flag.get() { + ::std::process::exit(1); + } + }) } +#[test] +fn does_not_abort() { + set("test"); + let _guard = AbortGuard::new(false); + ::std::panic::catch_unwind(|| panic!()).ok(); +} diff --git a/substrate/core/state-machine/Cargo.toml b/substrate/core/state-machine/Cargo.toml index a98ad52334..d1aa49ddcb 100644 --- a/substrate/core/state-machine/Cargo.toml +++ b/substrate/core/state-machine/Cargo.toml @@ -15,4 +15,5 @@ trie-db = "0.9" trie-root = "0.9" trie = { package = "substrate-trie", path = "../trie" } primitives = { package = "substrate-primitives", path = "../primitives" } +panic-handler = { package = "substrate-panic-handler", path = "../panic-handler" } parity-codec = "3.0" diff --git a/substrate/core/state-machine/src/ext.rs b/substrate/core/state-machine/src/ext.rs index bd353397e2..e0f164b42b 100644 --- a/substrate/core/state-machine/src/ext.rs +++ b/substrate/core/state-machine/src/ext.rs @@ -181,21 +181,25 @@ where H::Out: Ord + HeapSizeOf, { fn storage(&self, key: &[u8]) -> Option> { + let _guard = panic_handler::AbortGuard::new(true); self.overlay.storage(key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(|| self.backend.storage(key).expect(EXT_NOT_ALLOWED_TO_FAIL)) } fn storage_hash(&self, key: &[u8]) -> Option { + let _guard = panic_handler::AbortGuard::new(true); self.overlay.storage(key).map(|x| x.map(|x| H::hash(x))).unwrap_or_else(|| self.backend.storage_hash(key).expect(EXT_NOT_ALLOWED_TO_FAIL)) } fn child_storage(&self, storage_key: &[u8], key: &[u8]) -> Option> { + let _guard = panic_handler::AbortGuard::new(true); self.overlay.child_storage(storage_key, key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(|| self.backend.child_storage(storage_key, key).expect(EXT_NOT_ALLOWED_TO_FAIL)) } fn exists_storage(&self, key: &[u8]) -> bool { + let _guard = panic_handler::AbortGuard::new(true); match self.overlay.storage(key) { Some(x) => x.is_some(), _ => self.backend.exists_storage(key).expect(EXT_NOT_ALLOWED_TO_FAIL), @@ -203,6 +207,7 @@ where } fn exists_child_storage(&self, storage_key: &[u8], key: &[u8]) -> bool { + let _guard = panic_handler::AbortGuard::new(true); match self.overlay.child_storage(storage_key, key) { Some(x) => x.is_some(), _ => self.backend.exists_child_storage(storage_key, key).expect(EXT_NOT_ALLOWED_TO_FAIL), @@ -210,6 +215,7 @@ where } fn place_storage(&mut self, key: Vec, value: Option>) { + let _guard = panic_handler::AbortGuard::new(true); if is_child_storage_key(&key) { warn!(target: "trie", "Refuse to directly set child storage key"); return; @@ -220,6 +226,7 @@ where } fn place_child_storage(&mut self, storage_key: Vec, key: Vec, value: Option>) -> bool { + let _guard = panic_handler::AbortGuard::new(true); if !is_child_storage_key(&storage_key) || !is_child_trie_key_valid::(&storage_key) { return false; } @@ -231,6 +238,7 @@ where } fn kill_child_storage(&mut self, storage_key: &[u8]) { + let _guard = panic_handler::AbortGuard::new(true); if !is_child_storage_key(storage_key) || !is_child_trie_key_valid::(storage_key) { return; } @@ -243,6 +251,7 @@ where } fn clear_prefix(&mut self, prefix: &[u8]) { + let _guard = panic_handler::AbortGuard::new(true); if is_child_storage_key(prefix) { warn!(target: "trie", "Refuse to directly clear prefix that is part of child storage key"); return; @@ -260,6 +269,7 @@ where } fn storage_root(&mut self) -> H::Out { + let _guard = panic_handler::AbortGuard::new(true); if let Some((_, ref root)) = self.storage_transaction { return root.clone(); } @@ -283,6 +293,7 @@ where } fn child_storage_root(&mut self, storage_key: &[u8]) -> Option> { + let _guard = panic_handler::AbortGuard::new(true); if !is_child_storage_key(storage_key) || !is_child_trie_key_valid::(storage_key) { return None; } @@ -295,6 +306,7 @@ where } fn storage_changes_root(&mut self, parent: H::Out, parent_num: u64) -> Option { + let _guard = panic_handler::AbortGuard::new(true); let root_and_tx = compute_changes_trie_root::<_, T, H>( self.backend, self.changes_trie_storage.clone(), diff --git a/substrate/node/src/main.rs b/substrate/node/src/main.rs index fdc51cdc1e..1c292b6c2b 100644 --- a/substrate/node/src/main.rs +++ b/substrate/node/src/main.rs @@ -60,6 +60,7 @@ fn run() -> cli::error::Result<()> { executable_name: "substrate", author: "Parity Technologies ", description: "Generic substrate node", + support_url: "https://github.com/paritytech/substrate/issues/new", }; cli::run(::std::env::args(), Exit, version) }