Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
josephlr opened this issue May 29, 2024 · 3 comments
Open

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

josephlr opened this issue May 29, 2024 · 3 comments

Comments

@josephlr
Copy link
Member

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
Copy link
Member

newpavlov commented May 29, 2024

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
Copy link
Member Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants