Skip to content

Commit 40919c2

Browse files
committed
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
1 parent 6911734 commit 40919c2

File tree

3 files changed

+83
-6
lines changed

3 files changed

+83
-6
lines changed

contrib/test.sh

+3-1
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,9 @@ if [ "$DO_ASAN" = true ]; then
8080
fi
8181

8282
# Test if panic in C code aborts the process (either with a real panic or with SIGILL)
83-
cargo test -- --ignored --exact 'tests::test_panic_raw_ctx_should_terminate_abnormally' 2>&1 | tee /dev/stderr | grep "SIGILL\\|panicked at '\[libsecp256k1\]"
83+
cargo test -- --ignored --exact 'tests::test_panic_raw_ctx_should_terminate_abnormally' 2>&1 | tee /dev/stderr | grep "SIGABRT\\|SIGILL\\|panicked at '\[libsecp256k1\]"
84+
# Test custom handler
85+
cargo test -- --ignored --nocapture --exact 'tests::test_custom_abort_handler' 2>&1 | tee /dev/stderr | grep '^this is a custom abort handler:'
8486

8587
# Bench
8688
if [ "$DO_BENCH" = true ]; then

secp256k1-sys/src/lib.rs

+55-3
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ pub mod types;
4040
pub mod recovery;
4141

4242
use core::{slice, ptr};
43+
use core::sync::atomic::AtomicPtr;
4344
use types::*;
4445

4546
/// Flag for context to enable no precomputation
@@ -576,6 +577,57 @@ pub unsafe fn secp256k1_context_destroy(ctx: *mut Context) {
576577
rustsecp256k1_v0_4_1_context_destroy(ctx)
577578
}
578579

580+
static ABORT_HANDLER: AtomicPtr<fn(&dyn core::fmt::Display) -> !> = AtomicPtr::new(core::ptr::null_mut());
581+
582+
/// Registers custom abort handler globally.
583+
///
584+
/// libsecp256k1 may want to abort in case of invalid inputs. These are definitely bugs.
585+
/// The default handler aborts with `std` and **loops forever without `std`**.
586+
/// You can provide your own handler if you wish to override this behavior.
587+
///
588+
/// This function is `unsafe` because the supplied handler MUST NOT unwind
589+
/// since unwinding would cross FFI boundary.
590+
/// Note that double panic is *also* wrong!
591+
///
592+
/// Supplying panicking function may be safe if your program is compiled with `panic = "abort"`.
593+
/// It's **not** recommended to call this function from library crates - only binaries, somewhere
594+
/// near the beginning of `main()`.
595+
///
596+
/// The parameter passed to the handler is an error message that can be displayed (logged).
597+
///
598+
/// Note that the handler is a reference to function pointer rather than a function pointer itself
599+
/// because of [a missing Rust feature](https://github.com/rust-lang/rfcs/issues/2481).
600+
pub unsafe fn set_abort_handler(handler: &'static fn(&dyn core::fmt::Display) -> !) {
601+
ABORT_HANDLER.store(handler as *const _ as *mut _, core::sync::atomic::Ordering::Relaxed);
602+
}
603+
604+
/// FFI-safe replacement for panic
605+
///
606+
/// Prints to stderr and aborts with `std`, double panics without `std`.
607+
#[cfg_attr(not(feature = "std"), allow(unused))]
608+
fn abort_fallback(message: impl core::fmt::Display) -> ! {
609+
#[cfg(feature = "std")]
610+
{
611+
eprintln!("[libsecp256k1] {}", message);
612+
std::process::abort()
613+
}
614+
#[cfg(not(feature = "std"))]
615+
{
616+
// no better way to "abort" without std :(
617+
loop {}
618+
}
619+
}
620+
621+
fn abort_with_message(message: impl core::fmt::Display) -> ! {
622+
unsafe {
623+
let handler = ABORT_HANDLER.load(core::sync::atomic::Ordering::Relaxed);
624+
if !handler.is_null() {
625+
(*handler)(&message)
626+
} else {
627+
abort_fallback(message)
628+
}
629+
}
630+
}
579631

580632
/// **This function is an override for the C function, this is the an edited version of the original description:**
581633
///
@@ -601,7 +653,7 @@ pub unsafe extern "C" fn rustsecp256k1_v0_4_1_default_illegal_callback_fn(messag
601653
use core::str;
602654
let msg_slice = slice::from_raw_parts(message as *const u8, strlen(message));
603655
let msg = str::from_utf8_unchecked(msg_slice);
604-
panic!("[libsecp256k1] illegal argument. {}", msg);
656+
abort_with_message(format_args!("illegal argument. {}", msg));
605657
}
606658

607659
/// **This function is an override for the C function, this is the an edited version of the original description:**
@@ -624,7 +676,7 @@ pub unsafe extern "C" fn rustsecp256k1_v0_4_1_default_error_callback_fn(message:
624676
use core::str;
625677
let msg_slice = slice::from_raw_parts(message as *const u8, strlen(message));
626678
let msg = str::from_utf8_unchecked(msg_slice);
627-
panic!("[libsecp256k1] internal consistency check failed {}", msg);
679+
abort_with_message(format_args!("internal consistency check failed {}", msg));
628680
}
629681

630682
#[cfg(not(rust_secp_no_symbol_renaming))]
@@ -833,7 +885,7 @@ mod fuzz_dummy {
833885
*output = 4;
834886
ptr::copy((*pk).0.as_ptr(), output.offset(1), 64);
835887
} else {
836-
panic!("Bad flags");
888+
abort_with_message(format_args!("Bad flags"));
837889
}
838890
1
839891
}

src/lib.rs

+25-2
Original file line numberDiff line numberDiff line change
@@ -578,16 +578,39 @@ mod tests {
578578
drop(ctx_vrfy);
579579
}
580580

581-
#[cfg(not(target_arch = "wasm32"))]
581+
#[cfg(all(not(target_arch = "wasm32"), feature = "std"))]
582582
#[test]
583-
#[ignore] // Panicking from C may trap (SIGILL) intentionally, so we test this manually.
583+
#[ignore] // Aborting intentionally, so we test this manually.
584584
fn test_panic_raw_ctx_should_terminate_abnormally() {
585585
let ctx_vrfy = Secp256k1::verification_only();
586586
let raw_ctx_verify_as_full = unsafe {Secp256k1::from_raw_all(ctx_vrfy.ctx)};
587587
// Generating a key pair in verify context will panic (ARG_CHECK).
588588
raw_ctx_verify_as_full.generate_keypair(&mut thread_rng());
589589
}
590590

591+
#[cfg(all(not(target_arch = "wasm32"), feature = "std"))]
592+
#[test]
593+
#[ignore] // Aborting intentionally, so we test this manually.
594+
fn test_custom_abort_handler() {
595+
fn custom_abort(message: &dyn std::fmt::Display) -> ! {
596+
use std::io::Write;
597+
598+
eprintln!("this is a custom abort handler: {}", message);
599+
let _ = std::io::stderr().flush();
600+
std::process::abort()
601+
}
602+
static CUSTOM_ABORT: fn (&dyn std::fmt::Display) -> ! = custom_abort;
603+
604+
unsafe {
605+
ffi::set_abort_handler(&CUSTOM_ABORT);
606+
}
607+
608+
let ctx_vrfy = Secp256k1::verification_only();
609+
let raw_ctx_verify_as_full = unsafe {Secp256k1::from_raw_all(ctx_vrfy.ctx)};
610+
// Generating a key pair in verify context will panic (ARG_CHECK).
611+
raw_ctx_verify_as_full.generate_keypair(&mut thread_rng());
612+
}
613+
591614
#[test]
592615
fn test_preallocation() {
593616
let mut buf_ful = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];

0 commit comments

Comments
 (0)