Undefined behavior: the library panics across FFI boundary

[Kixunil]

Panicking across FFI boundary is UB but the library does it anyway in rustsecp256k1_v0_4_1_default_illegal_callback_fn. This has to be changed to abort. (infinite loop in non-std, I think)

[real-or-random]

See #290 and the first bullet in #288 (comment)

[Kixunil]

Forgot it's no longer UB. Relying on UB even with test doesn't sound good to me. Once could build the crate with a slightly different compiler or even in non-test mode could cause issues. We literally can't be sure, that's pretty much what "undefined" means.

[real-or-random]

Well yeah, it's not ideal for sure but I think we've explored all options in #288 and concluded this is the best. Or what would you suggest? You mentioned abort but it requires std.

[Kixunil]

#[cfg(feature = "std")]
{
    std::process::abort();
}
#[cfg(not(feature = "std"))]
{
    loop {}
}

[Kixunil]

Another no_std alternative:

struct PanicOnDrop;

impl Drop for PanicOnDrop {
    fn drop(&mut self) {
        panic!("force abort by double panic");
    }
}

let bomb = PanicOnDrop;
panic!("force abort by double panic");

However not sure how it translates on embedded and similar platforms.

[real-or-random]

I believe the double panic is indeed a way to access core::intrinsics::abort, which is otherwise not exposed in no_std. That may be a reasonable alternative I think.

I'm personally still too lazy to bother with this but feel free to open a PR.

[elichai]

I'm not sure there's anything to fix here.
right now panicking across FFI boundries is not UB.
Even when it was compiler devs explicitly allowed it to just pass through to support people doing longjumps, See: https://github.com/rust-lang/project-ffi-unwind/blob/master/periodic-summaries/October-2ndHalf.md#letting-extern-c-unwind and a lot of threads in Zulip about it

[apoelstra]

I tend to agree with @elichai.

I would ACK/accept a PR to use the double-panic bomb, since I think this behavior is still UB on the verison of rustc that mrustc supports (I think), and I appreciate that my "it's not UB on new stuff, and empirically the UB on old stuff has ok behavior" logic is not very sound. But I don't feel strongly enough about this that I would do the work myself.

[Kixunil]

No problem, submitted the PR.

[Kixunil]

Actually I just found out that panic across FFI is still UB - it got reverted and as far as I can see it wasn't reintroduced since then.

[apoelstra]

Wow!! Good find.