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

MSRV 1.63: Use libstd instead of libc::open and instead of libpthreads's Mutex. #458

Closed
wants to merge 3 commits into from
Closed
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
2 changes: 1 addition & 1 deletion .clippy.toml
Original file line number Diff line number Diff line change
@@ -1 +1 @@
msrv = "1.60"
msrv = "1.63"
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-22.04, windows-2022]
toolchain: [nightly, beta, stable, "1.60"]
toolchain: [nightly, beta, stable, "1.63"]
# Only Test macOS on stable to reduce macOS CI jobs
include:
# x86_64-apple-darwin.
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/workspace.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ jobs:
- name: SOLID (solid.rs)
run: cargo clippy -Zbuild-std=core --target aarch64-kmc-solid_asp3
- name: Redox (use_file.rs)
run: cargo clippy -Zbuild-std=core --target x86_64-unknown-redox
run: cargo clippy -Zbuild-std=std --target x86_64-unknown-redox
- name: VxWorks (vxworks.rs)
run: cargo clippy -Zbuild-std=core --target x86_64-wrs-vxworks
- name: WASI (wasi.rs)
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "getrandom"
version = "0.2.15" # Also update html_root_url in lib.rs when bumping this
edition = "2021"
rust-version = "1.60" # Sync .clippy.toml, tests.yml, and README.md.
rust-version = "1.63" # Sync .clippy.toml, tests.yml, and README.md.
authors = ["The Rand Project Developers"]
license = "MIT OR Apache-2.0"
description = "A small cross-platform library for retrieving random data from system source"
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ crate features, WASM support and Custom RNGs see the

## Minimum Supported Rust Version

This crate requires Rust 1.60.0 or later.
This crate requires Rust 1.63.0 or later.

## Platform Support

Expand Down
3 changes: 3 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ impl Error {
pub const NODE_ES_MODULE: Error = internal_error(14);
/// Calling Windows ProcessPrng failed.
pub const WINDOWS_PROCESS_PRNG: Error = internal_error(15);
/// The mutex used when opening the random file was poisoned.
pub const UNEXPECTED_FILE_MUTEX_POISONED: Error = internal_error(16);

/// Codes below this point represent OS Errors (i.e. positive i32 values).
/// Codes at or above this point, but below [`Error::CUSTOM_START`] are
Expand Down Expand Up @@ -175,6 +177,7 @@ fn internal_desc(error: Error) -> Option<&'static str> {
Error::NODE_RANDOM_FILL_SYNC => Some("Calling Node.js API crypto.randomFillSync failed"),
Error::NODE_ES_MODULE => Some("Node.js ES modules are not directly supported, see https://docs.rs/getrandom#nodejs-es-module-support"),
Error::WINDOWS_PROCESS_PRNG => Some("ProcessPrng: Windows system function failure"),
Error::UNEXPECTED_FILE_MUTEX_POISONED => Some("File: Initialization panicked, poisoning the mutex"),
_ => None,
}
}
Expand Down
102 changes: 53 additions & 49 deletions src/use_file.rs
Original file line number Diff line number Diff line change
@@ -1,41 +1,47 @@
//! Implementations that just need to read from a file
use crate::{
util_libc::{open_readonly, sys_fill_exact},
Error,
};

extern crate std;

use crate::{util_libc::sys_fill_exact, Error};
use core::{
cell::UnsafeCell,
ffi::c_void,
mem::MaybeUninit,
sync::atomic::{AtomicI32, Ordering},
};
use std::{
fs,
io,
// TODO(MSRV 1.66): use `std::os::fd` instead of `std::unix::io`.
os::unix::io::{AsRawFd as _, BorrowedFd, IntoRawFd as _, RawFd},
sync::{Mutex, PoisonError},
};

/// For all platforms, we use `/dev/urandom` rather than `/dev/random`.
/// For more information see the linked man pages in lib.rs.
/// - On Linux, "/dev/urandom is preferred and sufficient in all use cases".
/// - On Redox, only /dev/urandom is provided.
/// - On AIX, /dev/urandom will "provide cryptographically secure output".
/// - On Haiku and QNX Neutrino they are identical.
const FILE_PATH: &[u8] = b"/dev/urandom\0";
const FILE_PATH: &str = "/dev/urandom";

// Do not inline this when it is the fallback implementation, but don't mark it
// `#[cold]` because it is hot when it is actually used.
#[cfg_attr(any(target_os = "android", target_os = "linux"), inline(never))]
pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
let fd = get_rng_fd()?;
sys_fill_exact(dest, |buf| unsafe {
libc::read(fd, buf.as_mut_ptr().cast::<c_void>(), buf.len())
libc::read(fd.as_raw_fd(), buf.as_mut_ptr().cast::<c_void>(), buf.len())
})
}

// Returns the file descriptor for the device file used to retrieve random
// bytes. The file will be opened exactly once. All subsequent calls will
// return the same file descriptor. This file descriptor is never closed.
fn get_rng_fd() -> Result<libc::c_int, Error> {
fn get_rng_fd() -> Result<BorrowedFd<'static>, Error> {
// std::os::fd::{BorrowedFd, OwnedFd} guarantee that -1 is not a valid file descriptor.
const FD_UNINIT: libc::c_int = -1;
const FD_UNINIT: RawFd = -1;

// In theory `libc::c_int` could be something other than `i32`, but for the
// In theory `RawFd` could be something other than `i32`, but for the
// targets we currently support that use `use_file`, it is always `i32`.
// If/when we add support for a target where that isn't the case, we may
// need to use a different atomic type or make other accomodations. The
Expand All @@ -49,27 +55,34 @@ fn get_rng_fd() -> Result<libc::c_int, Error> {
// process.) `get_fd_locked` stores into FD using `Ordering::Release` to
// ensure any such state is synchronized. `get_fd` loads from `FD` with
// `Ordering::Acquire` to synchronize with it.
//
// TODO(MSRV feature(once_cell_try)): Use `OnceLock::get_or_try_init`
// instead.
static FD: AtomicI32 = AtomicI32::new(FD_UNINIT);

fn get_fd() -> Option<libc::c_int> {
fn get_fd() -> Option<BorrowedFd<'static>> {
match FD.load(Ordering::Acquire) {
FD_UNINIT => None,
val => Some(val),
val => Some(unsafe { BorrowedFd::borrow_raw(val) }),
}
}

#[cold]
fn get_fd_locked() -> Result<libc::c_int, Error> {
fn get_fd_locked() -> Result<BorrowedFd<'static>, Error> {
// This mutex is used to prevent multiple threads from opening file
// descriptors concurrently, which could run into the limit on the
// number of open file descriptors. Our goal is to have no more than one
// file descriptor open, ever.
//
// SAFETY: We use the mutex only in this method, and we always unlock it
// before returning, making sure we don't violate the pthread_mutex_t API.
static MUTEX: Mutex = Mutex::new();
unsafe { MUTEX.lock() };
let _guard = DropGuard(|| unsafe { MUTEX.unlock() });
// We assume any call to `Mutex::lock` synchronizes-with
// (Ordering::Acquire) the preceding dropping of a `MutexGuard` that
// unlocks the mutex (Ordering::Release) and that `Mutex` doesn't have
// any special treatment for what's "inside" the mutex (the `T` in
// `Mutex<T>`). See `https://github.com/rust-lang/rust/issues/126239.
static MUTEX: Mutex<()> = Mutex::new(());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another possibility, ideally deferred to a future PR as it is riskier, would be to replace the double-checked locking with a simple RwLock<File>, on the assumption that acquiring the read lock is fast enough that avoiding doing it isn't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, maybe that can't be deferred. I was assuming that a locking a mutex has Ordering::Acquire semantics and unlocking it would have Ordering::Release semantics. However, Mutex doesn't guarantee Acquire/Release semantics at all; presumably it does have them for things inside the mutex, but FD isn't in the Mutex, only a () is. A Mutex<()> could theoretically have all its synchronization optimized away completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned elsewhere, I filed rust-lang/rust#126239 to get libstd to document the widely-assumed "fact" that Mutxes have acquire/release semantis.

let _guard = MUTEX
.lock()
.map_err(|_: PoisonError<_>| Error::UNEXPECTED_FILE_MUTEX_POISONED)?;

if let Some(fd) = get_fd() {
return Ok(fd);
Expand All @@ -79,11 +92,13 @@ fn get_rng_fd() -> Result<libc::c_int, Error> {
#[cfg(any(target_os = "android", target_os = "linux"))]
wait_until_rng_ready()?;

let fd = open_readonly(FILE_PATH)?;
let file = fs::File::open(FILE_PATH).map_err(map_io_error)?;

let fd = file.into_raw_fd();
debug_assert!(fd != FD_UNINIT);
FD.store(fd, Ordering::Release);

Ok(fd)
Ok(unsafe { BorrowedFd::borrow_raw(fd) })
}

// Use double-checked locking to avoid acquiring the lock if possible.
Expand Down Expand Up @@ -124,15 +139,12 @@ fn get_rng_fd() -> Result<libc::c_int, Error> {
// libsodium uses `libc::poll` similarly to this.
#[cfg(any(target_os = "android", target_os = "linux"))]
fn wait_until_rng_ready() -> Result<(), Error> {
let fd = open_readonly(b"/dev/random\0")?;
let file = fs::File::open("/dev/random").map_err(map_io_error)?;
let mut pfd = libc::pollfd {
fd,
fd: file.as_raw_fd(),
events: libc::POLLIN,
revents: 0,
};
let _guard = DropGuard(|| unsafe {
libc::close(fd);
});

loop {
// A negative timeout means an infinite timeout.
Expand All @@ -149,28 +161,20 @@ fn wait_until_rng_ready() -> Result<(), Error> {
}
}

struct Mutex(UnsafeCell<libc::pthread_mutex_t>);

impl Mutex {
const fn new() -> Self {
Self(UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER))
}
unsafe fn lock(&self) {
let r = libc::pthread_mutex_lock(self.0.get());
debug_assert_eq!(r, 0);
}
unsafe fn unlock(&self) {
let r = libc::pthread_mutex_unlock(self.0.get());
debug_assert_eq!(r, 0);
}
}

unsafe impl Sync for Mutex {}

struct DropGuard<F: FnMut()>(F);

impl<F: FnMut()> Drop for DropGuard<F> {
fn drop(&mut self) {
self.0()
}
fn map_io_error(err: io::Error) -> Error {
// TODO(MSRV feature(raw_os_error_ty)): Use `std::io::RawOsError`.
type RawOsError = i32;

err.raw_os_error()
.map_or(Error::UNEXPECTED, |errno: RawOsError| {
// RawOsError-to-u32 conversion is lossless for nonnegative values
// if they are the same size.
const _: () =
assert!(core::mem::size_of::<RawOsError>() == core::mem::size_of::<u32>());

match u32::try_from(errno) {
Ok(code) if code != 0 => Error::from_os_error(code),
_ => Error::ERRNO_NOT_POSITIVE,
}
})
}
27 changes: 0 additions & 27 deletions src/util_libc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,30 +72,3 @@ pub fn sys_fill_exact(
}
Ok(())
}

/// Open a file in read-only mode.
///
/// # Panics
/// If `path` does not contain any zeros.
// TODO: Move `path` to `CStr` and use `CStr::from_bytes_until_nul` (MSRV 1.69)
// or C-string literals (MSRV 1.77) for statics
#[inline(always)]
pub fn open_readonly(path: &[u8]) -> Result<libc::c_int, Error> {
assert!(path.iter().any(|&b| b == 0));
loop {
let fd = unsafe {
libc::open(
path.as_ptr().cast::<libc::c_char>(),
libc::O_RDONLY | libc::O_CLOEXEC,
)
};
if fd >= 0 {
return Ok(fd);
}
let err = last_os_error();
// We should try again if open() was interrupted.
if err.raw_os_error() != Some(libc::EINTR) {
return Err(err);
}
}
}