Skip to content

Commit

Permalink
Don't panic across FFI
Browse files Browse the repository at this point in the history
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 rust-bitcoin#354
  • Loading branch information
Kixunil committed Jan 7, 2022
1 parent 6911734 commit 40919c2
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 6 deletions.
4 changes: 3 additions & 1 deletion contrib/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
58 changes: 55 additions & 3 deletions secp256k1-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -576,6 +577,57 @@ 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.
/// Note that 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).
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 {}
}
}

fn abort_with_message(message: impl core::fmt::Display) -> ! {
unsafe {
let handler = 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:**
///
Expand All @@ -601,7 +653,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:**
Expand All @@ -624,7 +676,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))]
Expand Down Expand Up @@ -833,7 +885,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
}
Expand Down
27 changes: 25 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,16 +578,39 @@ 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) -> ! {
use std::io::Write;

eprintln!("this is a custom abort handler: {}", message);
let _ = std::io::stderr().flush();
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()];
Expand Down

0 comments on commit 40919c2

Please sign in to comment.