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.

[real-or-random]

Oh I just sad in #358 that we should let it go.... But that was before I saw the comment here that the change got reverted. To be honest, I don't know what to do.

[Kixunil]

I see one, hopefully reasonable, way forward: have static AtomicPtr<fn(&str) -> !> storing the error handler initialized with null and a public function to set it.
The error handling code will load it and:

• call it if not null
• call eprintln!() + abort() on std
• loop forever in no_std

This behavior will be documented for no_std and consumers will be encouraged to provide their error handler.

[elichai]

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.

Wait, it's true that it got reverted, but only until extern "C-unwind" will be implemented, which it got implemented here: rust-lang/rust#76570, I'm pretty sure it is defined as abort right now, but let me try to get some official clarification here: rust-lang/rust#52652 (comment)

[apoelstra]

@elichai even if it is defined as of Rust 1.57 (or whatever) I think that's too far in the future (at least two years before that comes under our MSRV, maybe longer because post-Rust-2018 there's are far fewer essential features we're missing), and we need to handle this properly here.

I like @Kixunil's propasal.