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

Requesting Feedback: WIP changes to rand.rs #846

Closed
wants to merge 2 commits into from

Conversation

josephlr
Copy link
Contributor

This PR is a collection of all the potential improvements to rand.rs. This won't be merged as-is, and will need to be split off into smaller PRs like #839. Not all of the ideas here will end up being merged. I'm just putting this up to get feedback from @briansmith @sconybeare @myfreeweb @oherrala @tatsuya6502 and anyone else who interested.

This was motivated by the work I've been doing for the getrandom crate.

Things this patch does:

  • Organizes code based on how we try to get randomness:
    • sysrand: get randomness via a syscall or system function
    • file: get randomness from a file
    • some implementations choose between sysrand and file
  • Unifies the Linux and Android implementations
  • Read 1 byte from /dev/random before reading from /dev/urandom
  • Removes dev_urandom_fallback feature, as it is no longer needed for security. If you want to force Linux to use getrandom just don't enable the use_heap feature.
  • Linux: use libc::SYS_getrandom (Use libc::SYS_getrandom #839)
  • Linux: use GRND_NONBLOCK for getrandom() detection
  • Redox: Adds support back
  • NetBSD: Adds support (easy as it just does the same thing as Linux)
  • Windows: Simplifies implementation
  • Minor formatting and naming fixes
  • Adds more implementations for error::Unspecified to make error handling easier.
  • Documentation update

This has been in libc for a while, so we don't need to increment the
libc version in Cargo.toml. Note that we cannot use the `getrandom()`
function from libc, as it isn't avalible on Android.
@briansmith briansmith closed this Jun 15, 2019
@briansmith
Copy link
Owner

Sorry, I don't think it makes sense to mix so many changes at once, even for a conversation. Each of the changes you suggest deserve their own thread, and many of these topics already have their own thread.
Please discuss in the appropriate issue. For example, checking whether /dev/urandom is initialized is issue #558, which I found through this search: https://github.com/briansmith/ring/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+rand.

(Also, because CI is so slow due to the number of configurations tested, and because I'm in a hurry to make some particular changes today, I've canceled the CI jobs for these PRs. Don't interpret this as an implicit rejection of your PRs. However, I think it would be better for us to agree on what to do through a discussion in an issue rather than through PRs if there is any potential for disagreement.)

@josephlr josephlr deleted the fixup branch September 23, 2020 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants