From 7b2d78d190ab8848c3bace87459140b6e1370b48 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 18 Jun 2024 11:58:54 -0700 Subject: [PATCH] Android/Linux/rdrand: Use once_cell::race::OnceBool instead of LazyBool. Remove src/lazy.rs. `lazy::LazyBool` had "Last to win the race" semantics; when multiple threads see an uninitialized LazyBool, all of them will calculate a value. As they finish, each one will overwrite the value set by the previous thread. If two threads calculate different values for the boolean, then the value of the boolean can change during the period where the threads are racing. This doesn't seem to be a huge issue with the way it is currently used, but it is hard to reason about. `once_cell::race::OnceBool` has "first to win the race" semantics. When multiple threads see an uninitialized OnceBool, all of them will calculate a vlaue. The first one to finish will write its value; the rest will have their work ignored. Thus there is never any change in the stored value at any point. This is much easier to reason about. The different semantics come down to the fact that once_cell uses `AtomicUsize::compare_exchange` whereas lazy.rs was using `AtomicUsize::store`. --- Cargo.toml | 10 ++++- src/lazy.rs | 66 ------------------------------ src/lib.rs | 1 - src/linux_android_with_fallback.rs | 7 ++-- src/rdrand.rs | 7 ++-- tests/rdrand.rs | 2 - 6 files changed, 17 insertions(+), 76 deletions(-) delete mode 100644 src/lazy.rs diff --git a/Cargo.toml b/Cargo.toml index 47c7fd28..adccf53e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,6 +13,10 @@ exclude = [".*"] [dependencies] cfg-if = "1" +once_cell = { version = "1.19.0", default-features = false, optional = true } + +[target.'cfg(any(target_os = "android", target_os = "linux", all(target_arch = "x86_64", target_env = "sgx")))'.dependencies] +once_cell = { version = "1.19.0", default-features = false, features = ["race"] } # When built as part of libstd compiler_builtins = { version = "0.1", optional = true } @@ -30,6 +34,10 @@ windows-targets = "0.52" [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 } + +[target.'cfg(any(target_arch = "x86", target_arch = "x86_64"))'.dev-dependencies] +once_cell = { version = "1.19.0", default-features = false, features = ["race"] } + [target.'cfg(all(any(target_arch = "wasm32", target_arch = "wasm64"), target_os = "unknown"))'.dev-dependencies] wasm-bindgen-test = "0.3.18" @@ -40,7 +48,7 @@ std = [] # Bumps minimum supported Linux kernel version to 3.17 and Android API level to 23 (Marshmallow). linux_disable_fallback = [] # Feature to enable fallback RDRAND-based implementation on x86/x86_64 -rdrand = [] +rdrand = ["once_cell/race"] # Feature to enable JavaScript bindings on wasm*-unknown-unknown js = ["wasm-bindgen", "js-sys"] # Feature to enable custom RNG implementations diff --git a/src/lazy.rs b/src/lazy.rs deleted file mode 100644 index 0b4730e6..00000000 --- a/src/lazy.rs +++ /dev/null @@ -1,66 +0,0 @@ -//! Helpers built around pointer-sized atomics. -#![cfg(target_has_atomic = "ptr")] -#![allow(dead_code)] -use core::sync::atomic::{AtomicUsize, Ordering}; - -// This structure represents a lazily initialized static usize value. Useful -// when it is preferable to just rerun initialization instead of locking. -// unsync_init will invoke an init() function until it succeeds, then return the -// cached value for future calls. -// -// unsync_init supports init() "failing". If the init() method returns UNINIT, -// that value will be returned as normal, but will not be cached. -// -// Users should only depend on the _value_ returned by init() functions. -// Specifically, for the following init() function: -// fn init() -> usize { -// a(); -// let v = b(); -// c(); -// v -// } -// the effects of c() or writes to shared memory will not necessarily be -// observed and additional synchronization methods may be needed. -struct LazyUsize(AtomicUsize); - -impl LazyUsize { - // The initialization is not completed. - const UNINIT: usize = usize::MAX; - - const fn new() -> Self { - Self(AtomicUsize::new(Self::UNINIT)) - } - - // Runs the init() function at most once, returning the value of some run of - // init(). Multiple callers can run their init() functions in parallel. - // init() should always return the same value, if it succeeds. - fn unsync_init(&self, init: impl FnOnce() -> usize) -> usize { - #[cold] - fn do_init(this: &LazyUsize, init: impl FnOnce() -> usize) -> usize { - let val = init(); - this.0.store(val, Ordering::Relaxed); - val - } - - // Relaxed ordering is fine, as we only have a single atomic variable. - let val = self.0.load(Ordering::Relaxed); - if val != Self::UNINIT { - val - } else { - do_init(self, init) - } - } -} - -// Identical to LazyUsize except with bool instead of usize. -pub(crate) struct LazyBool(LazyUsize); - -impl LazyBool { - pub const fn new() -> Self { - Self(LazyUsize::new()) - } - - pub fn unsync_init(&self, init: impl FnOnce() -> bool) -> bool { - self.0.unsync_init(|| usize::from(init())) != 0 - } -} diff --git a/src/lib.rs b/src/lib.rs index f5e13c36..30be30b3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -216,7 +216,6 @@ use crate::util::{slice_as_uninit_mut, slice_assume_init_mut}; use core::mem::MaybeUninit; mod error; -mod lazy; mod util; // To prevent a breaking change when targets are added, we always export the // register_custom_getrandom macro, so old Custom RNG crates continue to build. diff --git a/src/linux_android_with_fallback.rs b/src/linux_android_with_fallback.rs index 98fa15e8..3efc5f3d 100644 --- a/src/linux_android_with_fallback.rs +++ b/src/linux_android_with_fallback.rs @@ -1,11 +1,12 @@ //! Implementation for Linux / Android with `/dev/urandom` fallback -use crate::{lazy::LazyBool, linux_android, use_file, util_libc::last_os_error, Error}; +use crate::{linux_android, use_file, util_libc::last_os_error, Error}; use core::mem::MaybeUninit; +use once_cell::race::OnceBool; pub fn getrandom_inner(dest: &mut [MaybeUninit]) -> Result<(), Error> { // getrandom(2) was introduced in Linux 3.17 - static HAS_GETRANDOM: LazyBool = LazyBool::new(); - if HAS_GETRANDOM.unsync_init(is_getrandom_available) { + static HAS_GETRANDOM: OnceBool = OnceBool::new(); + if HAS_GETRANDOM.get_or_init(is_getrandom_available) { linux_android::getrandom_inner(dest) } else { use_file::getrandom_inner(dest) diff --git a/src/rdrand.rs b/src/rdrand.rs index f4c593bd..deb3651d 100644 --- a/src/rdrand.rs +++ b/src/rdrand.rs @@ -1,6 +1,7 @@ //! RDRAND backend for x86(-64) targets -use crate::{lazy::LazyBool, util::slice_as_uninit, Error}; +use crate::{util::slice_as_uninit, Error}; use core::mem::{size_of, MaybeUninit}; +use once_cell::race::OnceBool; cfg_if! { if #[cfg(target_arch = "x86_64")] { @@ -94,8 +95,8 @@ fn is_rdrand_good() -> bool { } pub fn getrandom_inner(dest: &mut [MaybeUninit]) -> Result<(), Error> { - static RDRAND_GOOD: LazyBool = LazyBool::new(); - if !RDRAND_GOOD.unsync_init(is_rdrand_good) { + static RDRAND_GOOD: OnceBool = OnceBool::new(); + if !RDRAND_GOOD.get_or_init(is_rdrand_good) { return Err(Error::NO_RDRAND); } // SAFETY: After this point, we know rdrand is supported. diff --git a/tests/rdrand.rs b/tests/rdrand.rs index a355c31e..25678683 100644 --- a/tests/rdrand.rs +++ b/tests/rdrand.rs @@ -6,8 +6,6 @@ use getrandom::Error; #[macro_use] extern crate cfg_if; -#[path = "../src/lazy.rs"] -mod lazy; #[path = "../src/rdrand.rs"] mod rdrand; #[path = "../src/util.rs"]