Should __getrandom_custom use "Rust" ABI?

[josephlr]

Right now, the implementation of the custom feature uses a #[no_mangle] function named __getrandom_custom with signature: extern "C" fn(*mut u8, usize) -> u32.

When implementing this feature in #109, I initially used the extern "Rust" ABI, but after some discussion, we decided to use extern "C" due to issues around the stability of the "Rust" ABI.

However, @LegionMammal978 noted in #341 (comment) that panicking across an extern "C" function is unsafe, and it's very possible that a custom implementation could panic (certainly we do nothing to prevent that).

So should we switch to using the "Rust" ABI? If so, should we continue passing a raw pointer/length pair, or pass a slice?

[newpavlov]

Panic safety is a strong argument for using the "Rust" ABI. I think we can start with the minimal change of switching to it (i.e. we still will use fn(*mut u8, usize) -> u32 as signature). In hypothetical v0.3 we may expose __getrandom_custom with slice-based signature instead of relying on the macro.

[notgull]

In theory, you could also catch the panic and either abort on it or pass it around later as an error.

[LegionMammal978]

The ABI instability of extern "Rust" could be an issue here. Nothing's stopping __getrandom_custom() from being defined in a cdylib compiled with a different compiler version than the final program using getrandom_inner(). That would make it UB to call the extern "Rust" function across the boundary.

Aborting on panic is not all that difficult. It can be done most easily with a drop guard that causes a double panic:

extern "C" fn example() -> Output {
    struct PanicOnDrop;
    impl Drop for PanicOnDrop {
        fn drop(&mut self) {
            panic!("callback panicked");
        }
    }
    let guard = PanicOnDrop;
    let output = callback();
    core::mem::forget(guard);
    output
}

It would be a lot trickier to pass the full payload across the boundary, since, as before, neither a full Box<dyn Any + Send + Sync> nor a formatted String would be compatible across a cdylib. But just to continue the panic without the same payload would be feasible, by turning the panic into an error code and panicking again. (There are a few pitfalls in that, though.)

[newpavlov]

Nothing's stopping __getrandom_custom() from being defined in a cdylib compiled with a different compiler version than the final program using getrandom_inner().

I think A LOT of crates will break under such scenario, since doing so is not officially supported by Rust toolchain right now. We probably can safely ignore this concern.

Not catching potential panics will be the least surprising behavior for users, so I don't think we should do it unless absolutely necessary.