From 5d8bbced82c8fd003ae47da5a6827bf8c5ad3091 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Wed, 5 Jun 2024 15:15:55 -0700 Subject: [PATCH 1/2] 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 6e6f903a652b26a1c6687fc65f1f8b85a6562834 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Mon, 17 Jun 2024 10:58:10 -0700 Subject: [PATCH 2/2] use_file: Use `once_cell::sync::OnceCell` instead of libpthreads. 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 `std::sync::Once` to avoid adding an external dependency but it doesn't support fallible initialization. `OnceLock` wasn't added until 1.70, and even then `OnceLock::get_or_try_init` is still unstable. --- Cargo.toml | 3 ++ src/use_file.rs | 139 ++++++++++++------------------------------------ 2 files changed, 37 insertions(+), 105 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 47c7fd28..8f5f8e65 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,9 @@ wasi = { version = "0.11", default-features = false } [target.'cfg(all(windows, not(target_vendor = "win7")))'.dependencies] windows-targets = "0.52" +[target.'cfg(any(target_os = "aix", target_os = "android", target_os = "haiku", target_os = "linux", target_os = "nto", target_os = "redox"))'.dependencies] +once_cell = { version = "1.19.0", default-features = false, features = ["std"]} + [target.'cfg(all(any(target_arch = "wasm32", target_arch = "wasm64"), target_os = "unknown"))'.dependencies] wasm-bindgen = { version = "0.2.62", default-features = false, optional = true } js-sys = { version = "0.3", optional = true } diff --git a/src/use_file.rs b/src/use_file.rs index ea542bd0..85d93c75 100644 --- a/src/use_file.rs +++ b/src/use_file.rs @@ -3,16 +3,10 @@ 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}, -}; +use core::{ffi::c_void, mem::MaybeUninit}; +use std::{fs::File, io, os::unix::io::AsRawFd as _}; +// TODO(MSRV feature(once_cell_try)): Use std::sync::OnceLock instead. +use once_cell::sync::OnceCell as OnceLock; /// For all platforms, we use `/dev/urandom` rather than `/dev/random`. /// For more information see the linked man pages in lib.rs. @@ -26,78 +20,41 @@ const FILE_PATH: &str = "/dev/urandom"; // `#[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]) -> Result<(), Error> { - let fd = get_rng_fd()?; - sys_fill_exact(dest, |buf| unsafe { - libc::read(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 { - // std::os::fd::{BorrowedFd, OwnedFd} guarantee that -1 is not a valid file descriptor. - const FD_UNINIT: RawFd = -1; - - // 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 - // compiler will let us know if/when that is the case, because the - // `FD.store(fd)` would fail to compile. - // // The opening of the file, by libc/libstd/etc. may write some unknown // state into in-process memory. (Such state may include some sanitizer // bookkeeping, or we might be operating in a unikernal-like environment // where all the "kernel" file descriptor bookkeeping is done in our - // 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. - static FD: AtomicI32 = AtomicI32::new(FD_UNINIT); - - fn get_fd() -> Option { - match FD.load(Ordering::Acquire) { - FD_UNINIT => None, - val => Some(val), - } - } - - #[cold] - 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 - // 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() }); - - if let Some(fd) = get_fd() { - return Ok(fd); - } - - // On Linux, /dev/urandom might return insecure values. - #[cfg(any(target_os = "android", target_os = "linux"))] - wait_until_rng_ready()?; - - 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); + // process.) Thus we avoid using (relaxed) atomics like we use in other + // parts of the library. + // + // We 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. + // + // We assume any call to `OnceLock::get_or_try_init` synchronizes-with + // (Ordering::Acquire) the preceding call to `OnceLock::get_or_try_init` + // after `init()` returns an `Ok` result (Ordering::Release). See + // https://github.com/rust-lang/rust/issues/126239. + static FILE: OnceLock = OnceLock::new(); + let file = FILE.get_or_try_init(init)?; + + // TODO(MSRV feature(read_buf)): Use `std::io::Read::read_buf` + sys_fill_exact(dest, |buf| unsafe { + libc::read( + file.as_raw_fd(), + buf.as_mut_ptr().cast::(), + buf.len(), + ) + }) +} - Ok(fd) - } +#[cold] +fn init() -> Result { + // On Linux, /dev/urandom might return insecure values. + #[cfg(any(target_os = "android", target_os = "linux"))] + wait_until_rng_ready()?; - // Use double-checked locking to avoid acquiring the lock if possible. - if let Some(fd) = get_fd() { - Ok(fd) - } else { - get_fd_locked() - } + File::open(FILE_PATH).map_err(map_io_error) } // Polls /dev/random to make sure it is ok to read from /dev/urandom. @@ -130,9 +87,7 @@ 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 file = File::open("/dev/random").map_err(map_io_error)?; let mut pfd = libc::pollfd { fd: file.as_raw_fd(), events: libc::POLLIN, @@ -171,29 +126,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() - } -}