Use libc::getrandom on Solaris and update docs.

[josephlr]

#417 used getentropy(2) on Solaris, but after looking at the blog post introducing getrandom() and getentropy(), it seems like we should prefer using getrandom based on this quote:

On Solaris the output of getentropy(2) is entropy and should not be used where randomness is needed, in particular it must not be used where an IV or nonce is needed when calling a cryptographic operation. It is intended only for seeding a user space RBG (Random Bit Generator) system. More specifically the data returned by getentropy(2) has not had the required FIPS 140-2 processing for the DRBG applied to it.

I also updated some of the documentation explaining:

• Why we ever use getentropy(2)
• Why we set GRND_RANDOM on Solaris
• Why we don't ever set GRND_RANDOM on other platforms.

[newpavlov]

Left comment here just before this PR was created.

[josephlr]

Left comment here just before this PR was created.

Sounds good! I'll put Solaris in its own file.

[josephlr]

@newpavlov I moved Solaris stuff to its own file. I also changed the implementation to always set GRND_RANDOM.

Looking over the man pages, I think your comment in #417 (comment) (noting that all "correct" examples use GRND_RANDOM) is a good point. I don't think it hugely matters one way or another, and if we are doing a special implementation for Solaris, we might as well just mimic what they recommend in their man pages.

Let me know if you want me to change it back to using flags = 0 for consistency with the other uses of getrandom(2).

[newpavlov]

I am fine with using GRND_RANDOM to be extra safe. We can change it later, if someone more familiar with the OS will confirm that using zero flag performs like we want.

[newpavlov]

I think it's better to write the ret check like this:

// Upon successful completion, the `getrandom()` function returns the number of
// bytes written to `buf`. Otherwise, it returns 0 and sets `errno` to indicate the error.
if ret == 0 {
    return Err(last_os_error());
}
if ret as usize != chunk.len() {
    return Err(Error::UNEXPECTED);
}

[newpavlov]

It also may be worth to add a note that getrandom_inner never receives an empty buffer because of the check in lib.rs.

[josephlr]

Fixed, but I made the error check ret < 0, as it's negative values which indicates errors for getrandom. Then we also don't have to think about zero-length buffer handling here. (though i agree it's fine).

[josephlr]

Oh weird it seems like the docs contradict themselves:

"Upon successful completion, the getrandom() function returns the number of bytes written to buf. Otherwise, it returns 0 and sets errno to indicate the error."

"the function returns -1 and errno is set to EAGAIN."

[newpavlov]

No, the check should be ret == 0 as per the cited man quote (yeah, I was surprised as well...).

UPD: Maybe we should check both 0 and -1 to be extra safe? :)

[josephlr]

Updated to check for negative and zero values and to document the weirdness with the error return value.

[newpavlov]

It could be worth to slightly modify the check to have only one branch in the happy path:

if ret as usize != chunk.len() {
    return if ret <= 0 {
        last_os_error()
    } else {
        Err(Error::UNEXPECTED)
    };
}

[josephlr]

I think that is harder to read (and will probably get compiled to the same thing regardless).

[newpavlov]

You can tweak formatting to your liking.

will probably get compiled to the same thing regardless

It does not: https://rust.godbolt.org/z/G6d1zq4hs But it's a small enough difference, so I do not insist on this change.

[josephlr]

Oh interesting. I'll leave it as-is, but that's good to know.

I think only casting ret (which is ssize_t) to usize once we know it's not negative is more consistent with how we handle those sorts of values elsewhere.