rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
264 stars 166 forks source link

Document and Test that `getrandom()` never panics. #435

Open josephlr opened 1 month ago

josephlr commented 1 month ago

Spun off of the review of #434

We should have a way to verify in the CI that our implementation (at least on easy-to-test architectures) does not panic. This could be done via the no-panic crate as a dev-dependancy.

newpavlov commented 1 month ago

A slightly easier approach could be to check whether generated rlib file contains any strings with panic. It will not catch any potential panics in uninlined functions from our dependencies (I think no-panic is similar in this regard) and may break if in future we will add an item with "panic" in it (e.g. an error description), but it may be a bit friendlier to cross-compilation.

josephlr commented 1 month ago

A slightly easier approach could be to check whether generated rlib file contains any strings with panic.

One thing to look out for is that our current implementation will emit references to panic_in_cleanup due to our use of DropGuard. This is because the compiler can't statically determine that libc::close and libc::pthread_mutex_unlock will not panic. Replacing them with Rust stubs removes the panic_in_cleanup references.

In general panic_in_cleanup is fine, as it will never create a panic, it just aborts the process.

briansmith commented 1 month ago

Eventually it would be good to eliminate all uses of the panic infrastructure (and runtime library in general), but use_file and the fallback logic is the least urgent, IMO.