Use the getentropy(2) syscall on OpenBSD #30430
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| for s in v.chunks_mut(256) { | ||
| unsafe { ret = syscall(7, s.as_mut_ptr(), s.len()); } | ||
| if ret == -1 { | ||
| panic!("unexpected getrandom error: {}", errno()); |
There was a problem hiding this comment.
the panic message isn't very accurate: use getentropy name instead of getrandom
|
@semarie Thanks for the comments, I've addressed both with additional commits. |
| } | ||
|
|
||
| extern "C" { | ||
| fn syscall(number: c_long, ...) -> c_long; |
There was a problem hiding this comment.
Could this be added to libc instead of being inlined here? It's likely to be useful for others as well! (same for the NR_GETENTROPY constant below)
There was a problem hiding this comment.
Yeah, I think that'd be useful, as long as it's portable across Unix platforms.
|
The man page for getentropy states
so should arc4random be used instead? |
I definitely don't think so. arc4random is a userland CSPRNG complete with a stirred pool etc. So, Rust would have a ChaCha20-based userland CSPRNG that backs up to a... ChaCha20-based userland CSPRNG that backs up to an entropy syscall. That sounds like a ton of extra overhead and complexity. Basically, arc4random is suggested over getentropy because it's a standard library CSPRNG rather than a straight syscall. But Rust implements its own standard library CSPRNG using the primitives we implement in |
|
Ah ok, makes sense to me! r=me once the bits have been added to libc (just let me know if you need help) |
You mean for the syscall() function addition, right? This PR is good to go as-is. |
|
Yes the |
|
In the meantime, can we add this? Considering that the existing getrandom(2) code for Linux does the same. |
|
When moving to the external liblibc I made a sweep of the standard library to excise all remaining |
|
Now that the necessary bits are in libc, the libc submodule will have to be bumped before this can be merged. |
Rust already supports Linux's getrandom(2), which is very similar and was based on getentropy(2). This is a pretty clean, simple addition that uses the same approach as the iOS randomness API support.
I copied it from the getrandom code but forgot to change the name. Reported by Sebastien Marie.
Reported by Sebastien Marie.
|
☔ The latest upstream changes (presumably #30381) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The libc update breaks other code. Specifically, I really think we should have merged the first commit and handled the rest elsewhere. This PR now involves updating a library's submodule and associated debugging, which aren't relevant to getentropy(2) support (particularly when the existing getrandom(2) support used a libc-less approach). |
|
@MMCo what was done is the good order for the workflow in rust. But it seems you don't have tested your PR on master before proposing it. OpenBSD and others BSD have poor testing infrastructure, so it is really important to test it locally before asking for merging. @alexcrichton I have a diff I am testing for the libc issue. As soon testing is ok, I will make a PR on libc. |
|
@semarie The first commit was well-tested. The rest have been in the process of making things work again after the various tweaks. I did get overconfident with a couple single-line changes. |
|
@MMCo there are no problem. Eventually you would only spot the issue more early. I hope I have not been too rude. |
Rust already supports Linux's getrandom(2), which is very similar and was based on getentropy(2). This is a pretty clean, simple addition that uses the same approach as the iOS randomness API support.
Rust already supports Linux's getrandom(2), which is very similar and
was based on getentropy(2). This is a pretty clean, simple addition that
uses the same approach as the iOS randomness API support.