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

Remove lazy_static dependancy #52

Merged
merged 6 commits into from
Jul 4, 2019
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
7 changes: 4 additions & 3 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,14 @@ matrix:

- rust: nightly
env: DESCRIPTION="cross-platform build only"
# Redox: wait for /~https://github.com/rust-lang/rust/issues/60139
install:
- rustup target add x86_64-sun-solaris
- rustup target add x86_64-unknown-cloudabi
- rustup target add x86_64-unknown-freebsd
- rustup target add x86_64-fuchsia
- rustup target add x86_64-unknown-netbsd
- rustup target add x86_64-unknown-redox
# - rustup target add x86_64-unknown-redox
- rustup target add x86_64-fortanix-unknown-sgx
# For no_std targets
- rustup component add rust-src
Expand All @@ -119,7 +120,7 @@ matrix:
- cargo build --target=x86_64-unknown-freebsd --all-features
- cargo build --target=x86_64-fuchsia --all-features
- cargo build --target=x86_64-unknown-netbsd --all-features
- cargo build --target=x86_64-unknown-redox --all-features
# - cargo build --target=x86_64-unknown-redox --all-features
- cargo build --target=x86_64-fortanix-unknown-sgx --all-features
- cargo xbuild --target=x86_64-unknown-uefi
# also test minimum dependency versions are usable
Expand All @@ -129,7 +130,7 @@ matrix:
- cargo build --target=x86_64-unknown-freebsd --all-features
- cargo build --target=x86_64-fuchsia --all-features
- cargo build --target=x86_64-unknown-netbsd --all-features
- cargo build --target=x86_64-unknown-redox --all-features
# - cargo build --target=x86_64-unknown-redox --all-features
- cargo build --target=x86_64-fortanix-unknown-sgx --all-features
- cargo xbuild --target=x86_64-unknown-uefi

Expand Down
7 changes: 1 addition & 6 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,12 @@ members = ["tests/wasm_bindgen"]
[dependencies]
log = { version = "0.4", optional = true }

[target.'cfg(any(unix, target_os = "wasi"))'.dependencies]
[target.'cfg(any(unix, target_os = "redox", target_os = "wasi"))'.dependencies]
libc = "0.2.54"

# For holding file descriptors
[target.'cfg(any(unix, target_os = "redox"))'.dependencies]
lazy_static = "1.3.0"

[target.wasm32-unknown-unknown.dependencies]
wasm-bindgen = { version = "0.2.29", optional = true }
stdweb = { version = "0.4.9", optional = true }
lazy_static = "1.3.0"

[features]
std = []
6 changes: 3 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,11 @@ extern crate std;

mod error;
pub use crate::error::Error;

#[allow(dead_code)]
mod util;

// These targets need weak linkage to libc randomness functions.
#[cfg(any(target_os = "macos", target_os = "solaris", target_os = "illumos"))]
#[cfg(any(unix, target_os = "redox"))]
#[allow(dead_code)]
mod util_libc;

// System-specific implementations.
Expand Down
30 changes: 19 additions & 11 deletions src/use_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,15 @@
//! Implementations that just need to read from a file
extern crate std;

use crate::util_libc::LazyFd;
use crate::Error;
use core::mem::ManuallyDrop;
use core::num::NonZeroU32;
use lazy_static::lazy_static;
use std::{fs::File, io::Read};
use std::os::unix::io::{FromRawFd, IntoRawFd, RawFd};
use std::{
fs::File,
io::{self, Read},
};

#[cfg(target_os = "redox")]
const FILE_PATH: &str = "rand:";
Expand All @@ -29,28 +34,31 @@ const FILE_PATH: &str = "/dev/urandom";
const FILE_PATH: &str = "/dev/random";

pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
lazy_static! {
static ref FILE: Result<File, Error> = init_file();
}
let mut f = FILE.as_ref()?;
static FD: LazyFd = LazyFd::new();
let fd = FD.init(init_file).ok_or(io::Error::last_os_error())?;
let file = ManuallyDrop::new(unsafe { File::from_raw_fd(fd) });
let mut file_ref: &File = &file;

if cfg!(target_os = "emscripten") {
// `Crypto.getRandomValues` documents `dest` should be at most 65536 bytes.
for chunk in dest.chunks_mut(65536) {
f.read_exact(chunk)?;
file_ref.read_exact(chunk)?;
}
} else {
f.read_exact(dest)?;
file_ref.read_exact(dest)?;
}
Ok(())
}

fn init_file() -> Result<File, Error> {
fn init_file() -> Option<RawFd> {
if FILE_PATH == "/dev/urandom" {
// read one byte from "/dev/random" to ensure that OS RNG has initialized
File::open("/dev/random")?.read_exact(&mut [0u8; 1])?;
File::open("/dev/random")
.ok()?
.read_exact(&mut [0u8; 1])
.ok()?;
}
Ok(File::open(FILE_PATH)?)
Some(File::open(FILE_PATH).ok()?.into_raw_fd())
}

#[inline(always)]
Expand Down
52 changes: 46 additions & 6 deletions src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,26 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use core::sync::atomic::{AtomicUsize, Ordering};
use core::sync::atomic::{AtomicUsize, Ordering::Relaxed};

// This structure represents a laziliy initialized static usize value. Useful
// when it is perferable to just rerun initialization instead of locking.
// Both unsync_init and sync_init will invoke an init() function until it
// succeeds, then return the cached value for future calls.
//
// Both methods support 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 syncronization methods with be needed.
pub struct LazyUsize(AtomicUsize);

impl LazyUsize {
Expand All @@ -19,20 +35,44 @@ impl LazyUsize {

// The initialization is not completed.
pub const UNINIT: usize = usize::max_value();
// The initialization is currently running.
pub const ACTIVE: usize = usize::max_value() - 1;

// Runs the init() function at least once, returning the value of some run
// of init(). Unlike std::sync::Once, the init() function may be run
// multiple times. If init() returns UNINIT, future calls to unsync_init()
// will always retry. This makes UNINIT ideal for representing failure.
// of init(). Multiple callers can run their init() functions in parallel.
// init() should always return the same value, if it succeeds.
pub fn unsync_init(&self, init: impl FnOnce() -> usize) -> usize {
// Relaxed ordering is fine, as we only have a single atomic variable.

Choose a reason for hiding this comment

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

This assumes that a file descriptor consists only of that integer and no other user-space data. I suppose that is accurate on Unixes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup this is correct, libc::open always returns a positive int (link is to the Linux docs, but the function is a POSIX standard).

So a file descriptor will always fit in a positive int on unix

let mut val = self.0.load(Ordering::Relaxed);
let mut val = self.0.load(Relaxed);
if val == Self::UNINIT {
val = init();
self.0.store(val, Ordering::Relaxed);
self.0.store(val, Relaxed);
}
val
}

// Synchronously runs the init() function. Only one caller will have their
// init() function running at a time, and exactly one successful call will
// be run. The init() function should never return LazyUsize::ACTIVE.
pub fn sync_init(&self, init: impl FnOnce() -> usize, mut wait: impl FnMut()) -> usize {
// Common and fast path with no contention. Don't wast time on CAS.
match self.0.load(Relaxed) {
Self::UNINIT | Self::ACTIVE => {}
val => return val,
}
// Relaxed ordering is fine, as we only have a single atomic variable.
loop {
match self.0.compare_and_swap(Self::UNINIT, Self::ACTIVE, Relaxed) {
Self::UNINIT => {
let val = init();
self.0.store(val, Relaxed);

Choose a reason for hiding this comment

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

IMO it would be a good idea to assert! here that init did not return UNINIT or ACTIVE. This is not hot code, so always asserting seems fine.

Choose a reason for hiding this comment

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

Oh, seems like the function is actually allowed to return UNINIT to indicate "just try again"? I guess this is implied by the doc comment not ruling out that case, but might be worth calling out explicitly.

ACTIVE can still be asserted.

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, seems like the function is actually allowed to return UNINIT to indicate "just try again"? I guess this is implied by the doc comment not ruling out that case, but might be worth calling out explicitly.

This is called out, just in the docs for LazyUsize.

// Both methods support init() "failing". If the init() method returns UNINIT,
// that value will be returned as normal, but will not be cached.

ACTIVE can still be asserted.

Good idea. Thanks @RalfJung for taking a look at this stuff.

Copy link
Member

Choose a reason for hiding this comment

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

I think it may be better to write something like this:

let mut val = init();
if val == Self::ACTIVE { val = Self::UNINIT }
self.0.store(val, Relaxed);

And describe this behavior in docs.

return val;
}
Self::ACTIVE => wait(),
val => return val,
}
}
}
}

// Identical to LazyUsize except with bool instead of usize.
Expand Down
26 changes: 26 additions & 0 deletions src/util_libc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,29 @@ impl Weak {
NonNull::new(addr as *mut _)
}
}

pub struct LazyFd(LazyUsize);

impl LazyFd {
pub const fn new() -> Self {
Self(LazyUsize::new())
}

// If init() returns Some(x), x should be nonnegative.
pub fn init(&self, init: impl FnOnce() -> Option<libc::c_int>) -> Option<libc::c_int> {
let fd = self.0.sync_init(
|| match init() {
// OK as val >= 0 and val <= c_int::MAX < usize::MAX
Some(val) => val as usize,
None => LazyUsize::UNINIT,
},
|| unsafe {
libc::usleep(1000);

Choose a reason for hiding this comment

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

Wow that's a long sleep, isn't it? A comment seems always warranted with such a magic number.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya this sleep should probably be shorter, any recommendations? My guess is that we would want it to be longer than the normal latency for an open(2) syscall.

Copy link
Member

@newpavlov newpavlov Jul 11, 2019

Choose a reason for hiding this comment

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

On my laptop libc::open takes ~7µs and on playground ~8µs, so I think 10µs will be a good value. (it's a very coarse measurement, since it also includes time to retrieve current time)

Choose a reason for hiding this comment

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

I'm sorry this is outside my range of experience.^^ 1ms just seemed like a long time. /dev/random doesn't even do "real" I/O, so it shouldn't have huge latency.

Copy link
Member

Choose a reason for hiding this comment

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

It can be a huge latency if system entropy pool has not initialized, IIRC up to several minutes.

Choose a reason for hiding this comment

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

Sure, but that's not the common case.

Ideally wait would do something like double the wait time each time around the loop, starting at e.g. 1µs and maxing out at 1s or so. But that seems like overkill.^^

},
);
match fd {
LazyUsize::UNINIT => None,
val => Some(val as libc::c_int),
}
}
}
15 changes: 9 additions & 6 deletions src/wasm32_stdweb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use stdweb::web::error::Error as WebError;
use stdweb::{_js_impl, js};

use crate::Error;
use lazy_static::lazy_static;
use std::sync::Once;

#[derive(Clone, Copy, Debug)]
enum RngSource {
Expand All @@ -25,11 +25,14 @@ enum RngSource {

pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> {
assert_eq!(mem::size_of::<usize>(), 4);

lazy_static! {
static ref RNG_SOURCE: Result<RngSource, Error> = getrandom_init();
}
getrandom_fill((*RNG_SOURCE)?, dest)
static ONCE: Once = Once::new();
static mut RNG_SOURCE: Result<RngSource, Error> = Err(Error::UNAVAILABLE);

// SAFETY: RNG_SOURCE is only written once, before being read.
ONCE.call_once(|| unsafe {
RNG_SOURCE = getrandom_init();
});
getrandom_fill(unsafe { RNG_SOURCE }?, dest)
}

fn getrandom_init() -> Result<RngSource, Error> {
Expand Down