From 001220b8d3900f0cf4f211cc61257cc75be46ead Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Wed, 5 Jun 2024 15:15:55 -0700 Subject: [PATCH 1/3] use_file: Use `std::File` instead of `DropGuard`. For now, still use `libc::{poll,read}`. But use `File::open` to open the files, instead of using `DropGuard`. While doing this, switch to the `RawFd` type alias from `libc::c_int`. --- .github/workflows/workspace.yml | 2 +- src/use_file.rs | 55 +++++++++++++++++++++++---------- src/util_libc.rs | 27 ---------------- 3 files changed, 40 insertions(+), 44 deletions(-) diff --git a/.github/workflows/workspace.yml b/.github/workflows/workspace.yml index 5c029aad..989a5b0d 100644 --- a/.github/workflows/workspace.yml +++ b/.github/workflows/workspace.yml @@ -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) diff --git a/src/use_file.rs b/src/use_file.rs index 4fdbe3d7..ea542bd0 100644 --- a/src/use_file.rs +++ b/src/use_file.rs @@ -1,14 +1,18 @@ //! 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, + os::fd::{IntoRawFd as _, RawFd}, +}; /// For all platforms, we use `/dev/urandom` rather than `/dev/random`. /// For more information see the linked man pages in lib.rs. @@ -16,7 +20,7 @@ use core::{ /// - 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. @@ -31,11 +35,11 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit]) -> Result<(), Error> { // 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 { +fn get_rng_fd() -> Result { // 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 @@ -51,7 +55,7 @@ fn get_rng_fd() -> Result { // `Ordering::Acquire` to synchronize with it. static FD: AtomicI32 = AtomicI32::new(FD_UNINIT); - fn get_fd() -> Option { + fn get_fd() -> Option { match FD.load(Ordering::Acquire) { FD_UNINIT => None, val => Some(val), @@ -59,7 +63,7 @@ fn get_rng_fd() -> Result { } #[cold] - fn get_fd_locked() -> Result { + fn get_fd_locked() -> Result { // 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 @@ -79,7 +83,9 @@ fn get_rng_fd() -> Result { #[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); @@ -124,15 +130,14 @@ fn get_rng_fd() -> Result { // 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")?; + use std::os::unix::io::AsRawFd as _; + + 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. @@ -149,6 +154,24 @@ fn wait_until_rng_ready() -> Result<(), Error> { } } +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::() == core::mem::size_of::()); + + match u32::try_from(errno) { + Ok(code) if code != 0 => Error::from_os_error(code), + _ => Error::ERRNO_NOT_POSITIVE, + } + }) +} + struct Mutex(UnsafeCell); impl Mutex { diff --git a/src/util_libc.rs b/src/util_libc.rs index 7708bfcc..95c36958 100644 --- a/src/util_libc.rs +++ b/src/util_libc.rs @@ -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 { - assert!(path.iter().any(|&b| b == 0)); - loop { - let fd = unsafe { - libc::open( - path.as_ptr().cast::(), - 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); - } - } -} From 7ac1473f7167e8b5949d496309c8cc567f517c46 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Mon, 17 Jun 2024 10:58:10 -0700 Subject: [PATCH 2/3] MSRV 1.63: use_file: Clarify I/O safety using `BorrowedFd`. --- .clippy.toml | 2 +- .github/workflows/tests.yml | 2 +- Cargo.toml | 2 +- README.md | 2 +- src/use_file.rs | 23 ++++++++++------------- 5 files changed, 14 insertions(+), 17 deletions(-) diff --git a/.clippy.toml b/.clippy.toml index 13f202e9..550d4759 100644 --- a/.clippy.toml +++ b/.clippy.toml @@ -1 +1 @@ -msrv = "1.60" +msrv = "1.63" diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 1d86ee27..46dd784a 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -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. diff --git a/Cargo.toml b/Cargo.toml index 47c7fd28..f2b7445b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/README.md b/README.md index ef8a6ce2..bbab4f28 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/use_file.rs b/src/use_file.rs index ea542bd0..4505c0d1 100644 --- a/src/use_file.rs +++ b/src/use_file.rs @@ -10,8 +10,10 @@ use core::{ sync::atomic::{AtomicI32, Ordering}, }; use std::{ - fs, io, - os::fd::{IntoRawFd as _, RawFd}, + fs, + io, + // TODO(MSRV 1.66): use `std::os::fd` instead of `std::unix::io`. + os::unix::io::{AsRawFd as _, BorrowedFd, IntoRawFd as _, RawFd}, }; /// For all platforms, we use `/dev/urandom` rather than `/dev/random`. @@ -28,14 +30,14 @@ const FILE_PATH: &str = "/dev/urandom"; pub fn getrandom_inner(dest: &mut [MaybeUninit]) -> Result<(), Error> { let fd = get_rng_fd()?; sys_fill_exact(dest, |buf| unsafe { - libc::read(fd, buf.as_mut_ptr().cast::(), buf.len()) + libc::read(fd.as_raw_fd(), buf.as_mut_ptr().cast::(), 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 { +fn get_rng_fd() -> Result, Error> { // std::os::fd::{BorrowedFd, OwnedFd} guarantee that -1 is not a valid file descriptor. const FD_UNINIT: RawFd = -1; @@ -55,22 +57,19 @@ fn get_rng_fd() -> Result { // `Ordering::Acquire` to synchronize with it. static FD: AtomicI32 = AtomicI32::new(FD_UNINIT); - fn get_fd() -> Option { + fn get_fd() -> Option> { match FD.load(Ordering::Acquire) { FD_UNINIT => None, - val => Some(val), + val => Some(unsafe { BorrowedFd::borrow_raw(val) }), } } #[cold] - fn get_fd_locked() -> Result { + fn get_fd_locked() -> Result, 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() }); @@ -89,7 +88,7 @@ fn get_rng_fd() -> Result { 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. @@ -130,8 +129,6 @@ fn get_rng_fd() -> Result { // libsodium uses `libc::poll` similarly to this. #[cfg(any(target_os = "android", target_os = "linux"))] fn wait_until_rng_ready() -> Result<(), Error> { - use std::os::unix::io::AsRawFd as _; - let file = fs::File::open("/dev/random").map_err(map_io_error)?; let mut pfd = libc::pollfd { fd: file.as_raw_fd(), From 2d9f884226510e7372e6c196adde7d7edb50f886 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 4 Jun 2024 22:55:19 -0700 Subject: [PATCH 3/3] use_file: `std::sync::Mutex`, dropping all libpthread use. pthreads mutexes are not safe to move. While it is very unlikely that the mutex we create will ever be moved, we don't actively do anything to actively prevent it from being moved. (libstd, when it used/uses pthreads mutexes, would box them to prevent them from being moved.) Also, now on Linux and Android (and many other targets for which we don't use use_std), libstd uses futexes instead of pthreads mutexes. Thus using libstd's Mutex will be more efficient and avoid adding an often-otherwise-unnecessary libpthreads dependency on these targets. * Linux, Android: Futex [1]. * Haiku, Redox, NTO, AIX: pthreads [2]. * others: not using `use_file`. This will not affect our plans for *-*-linux-none, since we don't plan to use `use_file` for it. This breaks 32-bit x86 QNX Neutrino, which doesn't have libstd because the target itself is abandoned [3]. the other QNX Neutrino targets didn't get libstd support until Rust 1.69, so this effectively raises the MSRV for them to 1.69. I tried to use `Once` to avoid the MSRV increase but it doesn't support fallible initialization. `OnceLock` wasn't added until 1.70 but even then, `OnceLock::get_or_try_init` is still unstable. On x86_64 Linux, this change removes all libpthreads dependencies: ```diff - pthread_mutex_lock - pthread_mutex_unlock ``` and adds these libstd dependencies: ```diff + std::panicking::panic_count::GLOBAL_PANIC_COUNT + std::panicking::panic_count::is_zero_slow_path + std::sys::sync::mutex::futex::Mutex::lock_contended + std::sys::sync::mutex::futex::Mutex::wake ``` as measured using `cargo asm`. [1] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L4-L10 [2] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L17-L20 [3] https://github.com/rust-random/getrandom/issues/453#issuecomment-2148124364 --- src/error.rs | 3 +++ src/use_file.rs | 44 ++++++++++++++------------------------------ 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/src/error.rs b/src/error.rs index 5eff99eb..5c4f1c9e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -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 @@ -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, } } diff --git a/src/use_file.rs b/src/use_file.rs index 4505c0d1..54d98d0a 100644 --- a/src/use_file.rs +++ b/src/use_file.rs @@ -4,7 +4,6 @@ extern crate std; use crate::{util_libc::sys_fill_exact, Error}; use core::{ - cell::UnsafeCell, ffi::c_void, mem::MaybeUninit, sync::atomic::{AtomicI32, Ordering}, @@ -14,6 +13,7 @@ use std::{ 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`. @@ -55,6 +55,9 @@ fn get_rng_fd() -> Result, 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> { @@ -70,9 +73,16 @@ fn get_rng_fd() -> Result, Error> { // 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. - 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`). See `https://github.com/rust-lang/rust/issues/126239. + static MUTEX: Mutex<()> = Mutex::new(()); + let _guard = MUTEX + .lock() + .map_err(|_: PoisonError<_>| Error::UNEXPECTED_FILE_MUTEX_POISONED)?; if let Some(fd) = get_fd() { return Ok(fd); @@ -168,29 +178,3 @@ fn map_io_error(err: io::Error) -> Error { } }) } - -struct Mutex(UnsafeCell); - -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); - -impl Drop for DropGuard { - fn drop(&mut self) { - self.0() - } -}