remove panic from C invalid-argument callbacks

[apoelstra]

The upstream C functions will return false after calling these callbacks, at which point we will (usually) panic in the Rust code.

We are not allowed to panic from the callbacks themselves, since the result would be a panic crossing FFI boundaries, which is UB (and results in segfaults in Rust nightly as of 2021-03-11).

[elichai]

There'e already previous discussion on this subject on: #228
I think the reason for the segfault is the merging of this PR: rust-lang/rust#76570.
I need to read the C-unwind carefully again, as I remember it said that unwinding through extern "C" should abort the process.

[real-or-random]

I don't understand this. Doesn't this say that https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md#abi-boundaries-and-unforced-unwinding that calling panic!() even from "C" is never UB?

edit: I mean of course only in the recent versions that implement this but that's where we see the segfault. Maybe because the RFC is not yet fully implemented? rust-lang/rust#74990

[elichai]

I don't understand this. Doesn't this say that https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md#abi-boundaries-and-unforced-unwinding that calling panic!() even from "C" is never UB?

edit: I mean of course only in the recent versions that implement this but that's where we see the segfault. Maybe because the RFC is not yet fully implemented? rust-lang/rust#74990

It looks like it doesn't segfault, it SIGILL, which is apparently is used by LLVM to abort:
rust-lang/rust#52633

So the nightly is doing exactly what the RFC says

[real-or-random]

Ah okay! Maybe we should simply call https://doc.rust-lang.org/std/process/fn.abort.html . I wasn't aware of this function. The process "crashes" cleanly and without UB. Isn't that exactly what we want?

edit: ah this was metnioned in #228, and it requires std of course... rust-lang/rfcs#2512 (comment)

[real-or-random]

Ok, better idea: Can't we write the callback in C and call __builtin_trap()? This even works on wasm:

https://github.com/WebAssembly/wasi-libc/blob/5ccfab77b097a5d0184f91184952158aa5904c8d/libc-bottom-half/sources/abort.c

[apoelstra]

We cannot use std::process::abort because it's not available with nostd.

This __builtin_trap solution seems pretty nice though!

[apoelstra]

There's also an argument that we could just leave the existing panic in place and just remove the unit test, since we cannot test a SIGILL.

[real-or-random]

Ok so I think two by far best solutions for us are:

• Stick with panic!(). For Rust versions that implement RFC 2945, this is exactly what we want (with the minor caveat that the SIGILL is not elegant). For older versions we have technically UB, but we've never seen a compiler exploiting this in bad ways, and future versions will implement RFC 2945.
• Write the handler in C and use __builtin_trap. This is never UB. The minor caveat is that this requires gcc or clang. As far as I understand, cc could use MSVC in some of our builds?! If yes, that's not nice anyway (for performance) but we could implement an alternative abort for MSVC. As long as we don't need to specialize this for 20 different compilers, this is doable.

[apoelstra]

I think we should just leave the panic in place.

[elichai]

I think this can be closed now that #290 is merged? (it is technically still UB on stable rust, but
A. this path should never take in production sound code.
B. in practice it currently works correctly because there's a lot of code out there that relies on that behavior (which is the motivation for the c-unwind RFC))

[real-or-random]

Yeah, I think this can be closed for now. I heard that the change in behavior may be reverted soon :( . But then we can still reconsider this.