Allow setting the panic hook with parity-clib (#9292)

* Allow setting the panic hook with parity-clib

* Make all FFI functions unsafe

* Fix comment

* Fix concern

Cargo.lock

@@ -1947,6 +1947,7 @@ source = "git+https://github.com/paritytech/parity-common#0045887fecd2fec39e56c9
 name = "parity-clib"
 version = "1.12.0"
 dependencies = [
+ "panic_hook 0.1.0",
  "parity-ethereum 2.1.0",
 ]
 

parity-clib/Cargo.toml

@@ -10,6 +10,7 @@ name = "parity"
 crate-type = ["cdylib", "staticlib"]
 
 [dependencies]
+panic_hook = { path = "../util/panic_hook" }
 parity-ethereum = { path = "../", default-features = false }
 
 [features]

parity-clib/parity.h

@@ -102,6 +102,23 @@ void parity_destroy(void* parity);
 ///
 int parity_rpc(void* parity, const char* rpc, size_t len, char* out_str, size_t* out_len);
 
+/// Sets a callback to call when a panic happens in the Rust code.
+///
+/// The callback takes as parameter the custom param (the one passed to this function), plus the
+/// panic message. You are expected to log the panic message somehow, in order to communicate it to
+/// the user. A panic always indicates a bug in Parity.
+///
+/// Note that this method sets the panic hook for the whole program, and not just for Parity. In
+/// other words, if you use multiple Rust libraries at once (and not just Parity), then a panic
+/// in any Rust code will call this callback as well.
+///
+/// ## Thread safety
+///
+/// The callback can be called from any thread and multiple times simultaneously. Make sure that
+/// your code is thread safe.
+///
+int parity_set_panic_hook(void (*cb)(void* param, const char* msg, size_t msg_len), void* param);
+
 #ifdef __cplusplus
 }
 #endif

parity-clib/src/lib.rs

@@ -18,6 +18,7 @@
 //! duplicating documentation.
 
 extern crate parity_ethereum;
+extern crate panic_hook;
 
 use std::os::raw::{c_char, c_void, c_int};
 use std::panic;
@@ -33,8 +34,7 @@ pub struct ParityParams {
 }
 
 #[no_mangle]
-pub extern fn parity_config_from_cli(args: *const *const c_char, args_lens: *const usize, len: usize, output: *mut *mut c_void) -> c_int {
+pub unsafe extern fn parity_config_from_cli(args: *const *const c_char, args_lens: *const usize, len: usize, output: *mut *mut c_void) -> c_int {
-	unsafe {
 	panic::catch_unwind(|| {
 		*output = ptr::null_mut();
 
@@ -71,20 +71,16 @@ pub extern fn parity_config_from_cli(args: *const *const c_char, args_lens: *con
 		}
 	}).unwrap_or(1)
 }
-}
 
 #[no_mangle]
-pub extern fn parity_config_destroy(cfg: *mut c_void) {
+pub unsafe extern fn parity_config_destroy(cfg: *mut c_void) {
-	unsafe {
 	let _ = panic::catch_unwind(|| {
 		let _cfg = Box::from_raw(cfg as *mut parity_ethereum::Configuration);
 	});
 }
-}
 
 #[no_mangle]
-pub extern fn parity_start(cfg: *const ParityParams, output: *mut *mut c_void) -> c_int {
+pub unsafe extern fn parity_start(cfg: *const ParityParams, output: *mut *mut c_void) -> c_int {
-	unsafe {
 	panic::catch_unwind(|| {
 		*output = ptr::null_mut();
 		let cfg: &ParityParams = &*cfg;
@@ -92,18 +88,8 @@ pub extern fn parity_start(cfg: *const ParityParams, output: *mut *mut c_void) -
 		let config = Box::from_raw(cfg.configuration as *mut parity_ethereum::Configuration);
 
 		let on_client_restart_cb = {
-				struct Cb(Option<extern "C" fn(*mut c_void, *const c_char, usize)>, *mut c_void);
+			let cb = CallbackStr(cfg.on_client_restart_cb, cfg.on_client_restart_cb_custom);
-				unsafe impl Send for Cb {}
+			move |new_chain: String| { cb.call(&new_chain); }
-				unsafe impl Sync for Cb {}
-				impl Cb {
-					fn call(&self, new_chain: String) {
-						if let Some(ref cb) = self.0 {
-							cb(self.1, new_chain.as_bytes().as_ptr() as *const _, new_chain.len())
-						}
-					}
-				}
-				let cb = Cb(cfg.on_client_restart_cb, cfg.on_client_restart_cb_custom);
-				move |new_chain: String| { cb.call(new_chain); }
 		};
 
 		let action = match parity_ethereum::start(*config, on_client_restart_cb, || {}) {
@@ -121,21 +107,17 @@ pub extern fn parity_start(cfg: *const ParityParams, output: *mut *mut c_void) -
 		}
 	}).unwrap_or(1)
 }
-}
 
 #[no_mangle]
-pub extern fn parity_destroy(client: *mut c_void) {
+pub unsafe extern fn parity_destroy(client: *mut c_void) {
-	unsafe {
 	let _ = panic::catch_unwind(|| {
 		let client = Box::from_raw(client as *mut parity_ethereum::RunningClient);
 		client.shutdown();
 	});
 }
-}
 
 #[no_mangle]
-pub extern fn parity_rpc(client: *mut c_void, query: *const char, len: usize, out_str: *mut c_char, out_len: *mut usize) -> c_int {
+pub unsafe extern fn parity_rpc(client: *mut c_void, query: *const char, len: usize, out_str: *mut c_char, out_len: *mut usize) -> c_int {
-	unsafe {
 	panic::catch_unwind(|| {
 		let client: &mut parity_ethereum::RunningClient = &mut *(client as *mut parity_ethereum::RunningClient);
 
@@ -161,4 +143,23 @@ pub extern fn parity_rpc(client: *mut c_void, query: *const char, len: usize, ou
 		}
 	}).unwrap_or(1)
 }
+
+#[no_mangle]
+pub unsafe extern fn parity_set_panic_hook(callback: extern "C" fn(*mut c_void, *const c_char, usize), param: *mut c_void) {
+	let cb = CallbackStr(Some(callback), param);
+	panic_hook::set_with(move |panic_msg| {
+		cb.call(panic_msg);
+	});
+}
+
+// Internal structure for handling callbacks that get passed a string.
+struct CallbackStr(Option<extern "C" fn(*mut c_void, *const c_char, usize)>, *mut c_void);
+unsafe impl Send for CallbackStr {}
+unsafe impl Sync for CallbackStr {}
+impl CallbackStr {
+	fn call(&self, new_chain: &str) {
+		if let Some(ref cb) = self.0 {
+			cb(self.1, new_chain.as_bytes().as_ptr() as *const _, new_chain.len())
+		}
+	}
 }

parity/lib.rs

@@ -57,7 +57,6 @@ extern crate ethcore_transaction as transaction;
 extern crate ethereum_types;
 extern crate ethkey;
 extern crate kvdb;
-extern crate panic_hook;
 extern crate parity_hash_fetch as hash_fetch;
 extern crate parity_ipfs_api;
 extern crate parity_local_store as local_store;

parity/main.rs

@@ -253,7 +253,8 @@ fn main_direct(force_can_restart: bool) -> i32 {
 				panic_hook::set_with({
 					let e = exit.clone();
 					let exiting = exiting.clone();
-					move || {
+					move |panic_msg| {
+						let _ = stdio::stderr().write_all(panic_msg.as_bytes());
 						if !exiting.swap(true, Ordering::SeqCst) {
 							*e.0.lock() = ExitStatus {
 								panicking: true,

util/panic_hook/src/lib.rs

@@ -24,16 +24,26 @@ use std::thread;
 use std::process;
 use backtrace::Backtrace;
 
-/// Set the panic hook
+/// Set the panic hook to write to stderr and abort the process when a panic happens.
 pub fn set_abort() {
-	set_with(|| process::abort());
+	set_with(|msg| {
+		let _ = io::stderr().write_all(msg.as_bytes());
+		process::abort()
+	});
 }
 
-/// Set the panic hook with a closure to be called afterwards.
+/// Set the panic hook with a closure to be called. The closure receives the panic message.
-pub fn set_with<F: Fn() + Send + Sync + 'static>(f: F) {
+///
+/// Depending on how Parity was compiled, after the closure has been executed, either the process
+/// aborts or unwinding starts.
+///
+/// If you panic within the closure, a double panic happens and the process will stop.
+pub fn set_with<F>(f: F)
+where F: Fn(&str) + Send + Sync + 'static
+{
 	panic::set_hook(Box::new(move |info| {
-		panic_hook(info);
+		let msg = gen_panic_msg(info);
-		f();
+		f(&msg);
 	}));
 }
 
@@ -43,7 +53,7 @@ This is a bug. Please report it at:
     https://github.com/paritytech/parity-ethereum/issues/new
 ";
 
-fn panic_hook(info: &PanicInfo) {
+fn gen_panic_msg(info: &PanicInfo) -> String {
 	let location = info.location();
 	let file = location.as_ref().map(|l| l.file()).unwrap_or("<unknown>");
 	let line = location.as_ref().map(|l| l.line()).unwrap_or(0);
@@ -61,18 +71,13 @@ fn panic_hook(info: &PanicInfo) {
 
 	let backtrace = Backtrace::new();
 
-	let mut stderr = io::stderr();
+	format!(r#"
 
-	let _ = writeln!(stderr, "");
+====================
-	let _ = writeln!(stderr, "====================");
-	let _ = writeln!(stderr, "");
-	let _ = writeln!(stderr, "{:?}", backtrace);
-	let _ = writeln!(stderr, "");
-	let _ = writeln!(
-		stderr,
-		"Thread '{}' panicked at '{}', {}:{}",
-		name, msg, file, line
-	);
 
-	let _ = writeln!(stderr, "{}", ABOUT_PANIC);
+{backtrace:?}
+
+Thread '{name}' panicked at '{msg}', {file}:{line}
+{about}
+"#, backtrace = backtrace, name = name, msg = msg, file = file, line = line, about = ABOUT_PANIC)
 }