Don't panic across FFI

Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes #354

Author: Kixunil

contrib/test.sh

@@ -80,7 +80,9 @@ if [ "$DO_ASAN" = true ]; then
 fi
 
 # Test if panic in C code aborts the process (either with a real panic or with SIGILL)
-cargo test -- --ignored --exact 'tests::test_panic_raw_ctx_should_terminate_abnormally' 2>&1 | tee /dev/stderr | grep "SIGILL\\|panicked at '\[libsecp256k1\]"
+cargo test -- --ignored --exact 'tests::test_panic_raw_ctx_should_terminate_abnormally' 2>&1 | tee /dev/stderr | grep "SIGABRT\\|SIGILL\\|panicked at '\[libsecp256k1\]"
+# Test custom handler
+cargo test -- --ignored --nocapture --exact 'tests::test_custom_abort_handler' 2>&1 | tee /dev/stderr | grep '^this is a custom abort handler:'
 
 # Bench
 if [ "$DO_BENCH" = true ]; then

secp256k1-sys/src/lib.rs

@@ -40,6 +40,7 @@ pub mod types;
 pub mod recovery;
 
 use core::{slice, ptr};
+use core::sync::atomic::AtomicPtr;
 use types::*;
 
 /// Flag for context to enable no precomputation
@@ -576,6 +577,91 @@ pub unsafe fn secp256k1_context_destroy(ctx: *mut Context) {
     rustsecp256k1_v0_4_1_context_destroy(ctx)
 }
 
+static ABORT_HANDLER: AtomicPtr<fn(&dyn core::fmt::Display) -> !> = AtomicPtr::new(core::ptr::null_mut());
+
+/// Registers custom abort handler globally.
+///
+/// libsecp256k1 may want to abort in case of invalid inputs. These are definitely bugs.
+/// The default handler aborts with `std` and **loops forever without `std`**.
+/// You can provide your own handler if you wish to override this behavior.
+///
+/// This function is `unsafe` because the supplied handler MUST NOT unwind
+/// since unwinding would cross FFI boundary.
+/// Double panic is *also* wrong!
+///
+/// Supplying panicking function may be safe if your program is compiled with `panic = "abort"`.
+/// It's **not** recommended to call this function from library crates - only binaries, somewhere
+/// near the beginning of `main()`.
+///
+/// The parameter passed to the handler is an error message that can be displayed (logged).
+///
+/// Note that the handler is a reference to function pointer rather than a function pointer itself
+/// because of [a missing Rust feature](https://github.com/rust-lang/rfcs/issues/2481).
+/// It's a bit tricky to use it, so here's an example:
+///
+/// ```
+/// fn custom_abort(message: &dyn std::fmt::Display) -> ! {
+///     eprintln!("this is a custom abort handler: {}", message);
+///     std::process::abort()
+/// }
+/// // We need to put the function pointer into a static variable because we need a 'static
+/// // reference to variable holding function pointer.
+/// static CUSTOM_ABORT: fn (&dyn std::fmt::Display) -> ! = custom_abort;
+///
+/// unsafe {
+///     secp256k1_sys::set_abort_handler(&CUSTOM_ABORT);
+/// }
+/// ```
+///
+/// The function does not guarantee any memory ordering so you MUST NOT abuse it for synchronization!
+/// Use some other synchronization primitive if you need to synchronize.
+pub unsafe fn set_abort_handler(handler: &'static fn(&dyn core::fmt::Display) -> !) {
+    ABORT_HANDLER.store(ptr_const_to_mut_cast(handler), core::sync::atomic::Ordering::Relaxed);
+}
+
+/// FFI-safe replacement for panic
+///
+/// Prints to stderr and aborts with `std`, loops forever without `std`.
+#[cfg_attr(not(feature = "std"), allow(unused))]
+fn abort_fallback(message: impl core::fmt::Display) -> ! {
+    #[cfg(feature = "std")]
+    {
+        eprintln!("[libsecp256k1] {}", message);
+        std::process::abort()
+    }
+    #[cfg(not(feature = "std"))]
+    {
+        // no better way to "abort" without std :(
+        loop {}
+    }
+}
+
+/// Ensures that types both sides of cast stay in sync and only the constness changes.
+///
+/// This elliminates the risk that if we change the type signature of abort handler the cast
+/// silently converts the types and causes UB.
+fn ptr_mut_to_const_cast<T>(ptr: *mut T) -> *const T {
+    ptr as _
+}
+
+/// Ensures that types both sides of cast stay in sync and only the constness changes.
+///
+/// This elliminates the risk that if we change the type signature of abort handler the cast
+/// silently converts the types and causes UB.
+fn ptr_const_to_mut_cast<T>(ptr: *const T) -> *mut T {
+    ptr as _
+}
+
+fn abort_with_message(message: impl core::fmt::Display) -> ! {
+    unsafe {
+        let handler = ptr_mut_to_const_cast(ABORT_HANDLER.load(core::sync::atomic::Ordering::Relaxed));
+        if !handler.is_null() {
+            (*handler)(&message)
+        } else {
+            abort_fallback(message)
+        }
+    }
+}
 
 /// **This function is an override for the C function, this is the an edited version of the original description:**
 ///
@@ -601,7 +687,7 @@ pub unsafe extern "C" fn rustsecp256k1_v0_4_1_default_illegal_callback_fn(messag
     use core::str;
     let msg_slice = slice::from_raw_parts(message as *const u8, strlen(message));
     let msg = str::from_utf8_unchecked(msg_slice);
-    panic!("[libsecp256k1] illegal argument. {}", msg);
+    abort_with_message(format_args!("illegal argument. {}", msg));
 }
 
 /// **This function is an override for the C function, this is the an edited version of the original description:**
@@ -624,7 +710,7 @@ pub unsafe extern "C" fn rustsecp256k1_v0_4_1_default_error_callback_fn(message:
     use core::str;
     let msg_slice = slice::from_raw_parts(message as *const u8, strlen(message));
     let msg = str::from_utf8_unchecked(msg_slice);
-    panic!("[libsecp256k1] internal consistency check failed {}", msg);
+    abort_with_message(format_args!("internal consistency check failed {}", msg));
 }
 
 #[cfg(not(rust_secp_no_symbol_renaming))]
@@ -833,7 +919,7 @@ mod fuzz_dummy {
             *output = 4;
             ptr::copy((*pk).0.as_ptr(), output.offset(1), 64);
         } else {
-            panic!("Bad flags");
+            abort_with_message(format_args!("Bad flags"));
         }
         1
      }

src/lib.rs

@@ -112,6 +112,9 @@
 //!
 //! ## Crate features/optional dependencies
 //!
+//! This crate supports `no_std` however **you should check the documentation of
+//! [`ffi::set_abort_handler`] if you intend to use it without `std`!**
+//!
 //! This crate provides the following opt-in Cargo features:
 //!
 //! * `std` - use standard Rust library, enabled by default.
@@ -578,16 +581,36 @@ mod tests {
         drop(ctx_vrfy);
     }
 
-    #[cfg(not(target_arch = "wasm32"))]
+    #[cfg(all(not(target_arch = "wasm32"), feature = "std"))]
     #[test]
-    #[ignore] // Panicking from C may trap (SIGILL) intentionally, so we test this manually.
+    #[ignore] // Aborting intentionally, so we test this manually.
     fn test_panic_raw_ctx_should_terminate_abnormally() {
         let ctx_vrfy = Secp256k1::verification_only();
         let raw_ctx_verify_as_full = unsafe {Secp256k1::from_raw_all(ctx_vrfy.ctx)};
         // Generating a key pair in verify context will panic (ARG_CHECK).
         raw_ctx_verify_as_full.generate_keypair(&mut thread_rng());
     }
 
+    #[cfg(all(not(target_arch = "wasm32"), feature = "std"))]
+    #[test]
+    #[ignore] // Aborting intentionally, so we test this manually.
+    fn test_custom_abort_handler() {
+        fn custom_abort(message: &dyn std::fmt::Display) -> ! {
+            eprintln!("this is a custom abort handler: {}", message);
+            std::process::abort()
+        }
+        static CUSTOM_ABORT: fn (&dyn std::fmt::Display) -> ! = custom_abort;
+
+        unsafe {
+            ffi::set_abort_handler(&CUSTOM_ABORT);
+        }
+
+        let ctx_vrfy = Secp256k1::verification_only();
+        let raw_ctx_verify_as_full = unsafe {Secp256k1::from_raw_all(ctx_vrfy.ctx)};
+        // Generating a key pair in verify context will panic (ARG_CHECK).
+        raw_ctx_verify_as_full.generate_keypair(&mut thread_rng());
+    }
+
     #[test]
     fn test_preallocation() {
         let mut buf_ful = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];