Skip to content

Use libc::getrandom on Solaris and update docs. #420

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

Merged
merged 1 commit into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/getentropy.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
//! Implementation using libc::getentropy
//! Implementation using getentropy(2)
//!
//! Available since:
//! - macOS 10.12
//! - OpenBSD 5.6
//! - Emscripten 2.0.5
//! - vita newlib since Dec 2021
//!
//! For these targets, we use getentropy(2) because getrandom(2) doesn't exist.
use crate::{util_libc::last_os_error, Error};
use core::mem::MaybeUninit;

Expand Down
11 changes: 8 additions & 3 deletions src/getrandom.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
//! Implementation using `libc::getrandom`.
//! Implementation using getrandom(2).
//!
//! Available since:
//! - Linux Kernel 3.17, Glibc 2.25, Musl 1.1.20
//! - Android API level 23 (Marshmallow)
//! - NetBSD 10.0
//! - FreeBSD 12.0
//! - Solaris 11.3
//! - Illumos since Dec 2018
//! - illumos since Dec 2018
//! - DragonFly 5.7
//! - Hurd Glibc 2.31
//! - shim-3ds since Feb 2022
//!
//! For these platforms, we always use the default pool and never set the
//! GRND_RANDOM flag to use the /dev/random pool. On Linux/Android/Hurd, using
//! GRND_RANDOM is not recommended. On NetBSD/FreeBSD/Dragonfly/3ds, it does
//! nothing. On illumos, the default pool is used to implement getentropy(2),
//! so we assume it is acceptable here.
use crate::{util_libc::sys_fill_exact, Error};
use core::mem::MaybeUninit;

Expand Down
10 changes: 6 additions & 4 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
//! | OpenBSD | `*‑openbsd` | [`getentropy`][7]
//! | NetBSD | `*‑netbsd` | [`getrandom`][16] if available, otherwise [`kern.arandom`][8]
//! | Dragonfly BSD | `*‑dragonfly` | [`getrandom`][9]
//! | Solaris | `*‑solaris` | [`getentropy`][11]
//! | Illumos | `*‑illumos` | [`getrandom`][12]
//! | Solaris | `*‑solaris` | [`getrandom`][11] (with `GRND_RANDOM`)
//! | illumos | `*‑illumos` | [`getrandom`][12]
//! | Fuchsia OS | `*‑fuchsia` | [`cprng_draw`]
//! | Redox | `*‑redox` | `/dev/urandom`
//! | Haiku | `*‑haiku` | `/dev/urandom` (identical to `/dev/random`)
Expand Down Expand Up @@ -173,7 +173,7 @@
//! [7]: https://man.openbsd.org/getentropy.2
//! [8]: https://man.netbsd.org/sysctl.7
//! [9]: https://leaf.dragonflybsd.org/cgi/web-man?command=getrandom
//! [11]: https://docs.oracle.com/cd/E88353_01/html/E37841/getentropy-2.html
//! [11]: https://docs.oracle.com/cd/E88353_01/html/E37841/getrandom-2.html
//! [12]: https://illumos.org/man/2/getrandom
//! [13]: https://github.com/emscripten-core/emscripten/pull/12240
//! [14]: https://www.qnx.com/developers/docs/7.1/index.html#com.qnx.doc.neutrino.utilities/topic/r/random.html
Expand Down Expand Up @@ -238,7 +238,6 @@ cfg_if! {
} else if #[cfg(any(
target_os = "macos",
target_os = "openbsd",
target_os = "solaris",
target_os = "vita",
target_os = "emscripten",
))] {
Expand Down Expand Up @@ -302,6 +301,9 @@ cfg_if! {
} else if #[cfg(any(target_os = "android", target_os = "linux"))] {
mod util_libc;
#[path = "linux_android.rs"] mod imp;
} else if #[cfg(target_os = "solaris")] {
mod util_libc;
#[path = "solaris.rs"] mod imp;
} else if #[cfg(target_os = "netbsd")] {
mod util_libc;
#[path = "netbsd.rs"] mod imp;
Expand Down
34 changes: 34 additions & 0 deletions src/solaris.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//! Solaris implementation using getrandom(2).
//!
//! While getrandom(2) has been available since Solaris 11.3, it has a few
//! quirks not present on other OSes. First, on Solaris 11.3, calls will always
//! fail if bufsz > 1024. Second, it will always either fail or completely fill
//! the buffer (returning bufsz). Third, error is indicated by returning 0,
//! rather than by returning -1. Finally, "if GRND_RANDOM is not specified
//! then getrandom(2) is always a non blocking call". This _might_ imply that
//! in early-boot scenarios with low entropy, getrandom(2) will not properly
//! block. To be safe, we set GRND_RANDOM, mirroring the man page examples.
//!
//! For more information, see the man page linked in lib.rs and this blog post:
//! https://blogs.oracle.com/solaris/post/solaris-new-system-calls-getentropy2-and-getrandom2
//! which also explains why this crate should not use getentropy(2).
use crate::{util_libc::last_os_error, Error};
use core::mem::MaybeUninit;

const MAX_BYTES: usize = 1024;

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
for chunk in dest.chunks_mut(MAX_BYTES) {
let ptr = chunk.as_mut_ptr() as *mut libc::c_void;
let ret = unsafe { libc::getrandom(ptr, chunk.len(), libc::GRND_RANDOM) };
// In case the man page has a typo, we also check for negative ret.
if ret <= 0 {
return Err(last_os_error());
}
Copy link
Member

@newpavlov newpavlov May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Member

@newpavlov newpavlov May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@josephlr josephlr May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Member

@newpavlov newpavlov May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Member Author

@josephlr josephlr May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@newpavlov newpavlov May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
    };
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@josephlr josephlr May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// If getrandom(2) succeeds, it should have completely filled chunk.
if (ret as usize) != chunk.len() {
return Err(Error::UNEXPECTED);
}
}
Ok(())
}