From 96ddf0715bc4e8043c0dc4c9c9d8e8ff2d457098 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 5 Jan 2022 16:54:22 +0100 Subject: [PATCH] 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 --- contrib/test.sh | 4 +- secp256k1-sys/src/lib.rs | 84 ++++++++++++++++++++++++++++++++++++++-- src/lib.rs | 27 ++++++++++++- 3 files changed, 109 insertions(+), 6 deletions(-) diff --git a/contrib/test.sh b/contrib/test.sh index dc936ab75..a52166ab6 100755 --- a/contrib/test.sh +++ b/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 diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 313e3e6a6..c42e655b9 100644 --- a/secp256k1-sys/src/lib.rs +++ b/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,83 @@ pub unsafe fn secp256k1_context_destroy(ctx: *mut Context) { rustsecp256k1_v0_4_1_context_destroy(ctx) } +static ABORT_HANDLER: AtomicPtr !> = 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(handler as *const _ as *mut _, core::sync::atomic::Ordering::Relaxed); +} + +/// FFI-safe replacement for panic +/// +/// Prints to stderr and aborts with `std`, double panics 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(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 +679,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 +702,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 +911,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 } diff --git a/src/lib.rs b/src/lib.rs index 441900690..ae6eaa4e3 100644 --- a/src/lib.rs +++ b/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,9 +581,9 @@ 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)}; @@ -588,6 +591,26 @@ mod tests { 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()];