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 5, 2022
1 parent f531be3 commit 6ff72f8
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 4 deletions.
2 changes: 1 addition & 1 deletion contrib/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ 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\]"

# Bench
if [ "$DO_BENCH" = true ]; then
Expand Down
33 changes: 30 additions & 3 deletions secp256k1-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,33 @@ pub unsafe fn secp256k1_context_destroy(ctx: *mut Context) {
rustsecp256k1_v0_4_1_context_destroy(ctx)
}

/// FFI-safe replacement for panic
///
/// Prints to stderr and aborts with `std`, double panics without `std`.
#[cfg_attr(not(feature = "std"), allow(unused))]
fn ffi_abort(msg: impl core::fmt::Display) -> ! {
#[cfg(feature = "std")]
{
eprintln!("[libsecp256k1] {}", msg);
std::process::abort()
}
#[cfg(not(feature = "std"))]
{
use core::fmt::Display;

// Abort by double panic
struct PanicOnDrop<M: Display>(M);

impl<T: Display> Drop for PanicOnDrop<T> {
fn drop(&mut self) {
panic!("[libsecp256k1] {}", self.0);
}
}

let _bomb = PanicOnDrop(&msg);
panic!("[libsecp256k1] {}", &msg)
}
}

/// **This function is an override for the C function, this is the an edited version of the original description:**
///
Expand All @@ -594,7 +621,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);
ffi_abort(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 @@ -617,7 +644,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);
ffi_abort(format_args!("internal consistency check failed {}", msg));
}

#[cfg(not(rust_secp_no_symbol_renaming))]
Expand Down Expand Up @@ -826,7 +853,7 @@ mod fuzz_dummy {
*output = 4;
ptr::copy((*pk).0.as_ptr(), output.offset(1), 64);
} else {
panic!("Bad flags");
ffi_abort(format_args!("Bad flags"));
}
1
}
Expand Down

0 comments on commit 6ff72f8

Please sign in to comment.