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 4aabbbd
Show file tree
Hide file tree
Showing 3 changed files with 117 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
92 changes: 89 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,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:**
///
Expand All @@ -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:**
Expand All @@ -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))]
Expand Down Expand Up @@ -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
}
Expand Down
27 changes: 25 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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()];
Expand Down

0 comments on commit 4aabbbd

Please sign in to comment.