Uh oh!
There was an error while loading. Please reload this page.
Use the getentropy(2) syscall on OpenBSD - #30430
Conversation
rust-highfive
commented
Dec 17, 2015
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. |
There was a problem hiding this comment.
the panic message isn't very accurate: use getentropy name instead of getrandom
mmcco
commented
Dec 17, 2015
@semarie Thanks for the comments, I've addressed both with additional commits. |
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.
alexcrichton
commented
Dec 17, 2015
The man page for getentropy states
so should arc4random be used instead? |
mmcco
commented
Dec 17, 2015
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 |
alexcrichton
commented
Dec 17, 2015
Ah ok, makes sense to me! r=me once the bits have been added to libc (just let me know if you need help) |
mmcco
commented
Dec 17, 2015
You mean for the syscall() function addition, right? This PR is good to go as-is. |
alexcrichton
commented
Dec 17, 2015
Yes the |
mmcco
commented
Dec 17, 2015
In the meantime, can we add this? Considering that the existing getrandom(2) code for Linux does the same. |
alexcrichton
commented
Dec 17, 2015
When moving to the external liblibc I made a sweep of the standard library to excise all remaining |
mmcco
commented
Dec 19, 2015
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.
bors
commented
Dec 19, 2015
☔ The latest upstream changes (presumably #30381) made this pull request unmergeable. Please resolve the merge conflicts. |
mmcco
commented
Dec 19, 2015
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). |
semarie
commented
Dec 19, 2015
@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. |
mmcco
commented
Dec 19, 2015
@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. |
semarie
commented
Dec 19, 2015
@MMCo there are no problem. Eventually you would only spot the issue more early. I hope I have not been too rude. |
alexcrichton
commented
Dec 19, 2015
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.