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

Use getrandom for generating HashMap seed #80149

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 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
3 changes: 3 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,9 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ee8025cf36f917e6a52cce185b7c7177689b838b7ec138364e50cc2277a56cf4"
dependencies = [
"cfg-if 0.1.10",
"compiler_builtins",
"libc",
"rustc-std-workspace-core",
"wasi",
]

Expand Down Expand Up @@ -4692,6 +4694,7 @@ dependencies = [
"core",
"dlmalloc",
"fortanix-sgx-abi",
"getrandom 0.2.0",
"hashbrown",
"hermit-abi",
"libc",
Expand Down
5 changes: 5 additions & 0 deletions library/std/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ compiler_builtins = { version = "0.1.35" }
profiler_builtins = { path = "../profiler_builtins", optional = true }
unwind = { path = "../unwind" }
hashbrown = { version = "0.9.0", default-features = false, features = ['rustc-dep-of-std'] }
# RDRAND feature is needed for SGX targets, but will work on other x86(-64) targets
# unsupported in `getrandom` by default
bjorn3 marked this conversation as resolved.
Show resolved Hide resolved
# FIXME: remove the Hermit part after its support will be added to `getrandom`
[target.'cfg(not(any(all(target_arch = "wasm32", target_vendor = "unknown", target_os = "unknown"), all(target_arch = "aarch64", target_os = "hermit"))))'.dependencies]
getrandom = { version = "0.2.0", features = ['rustc-dep-of-std', 'rdrand'] }

# Dependencies of the `backtrace` crate
addr2line = { version = "0.14.0", optional = true, default-features = false }
Expand Down
20 changes: 15 additions & 5 deletions library/std/src/collections/hash/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use crate::fmt::{self, Debug};
use crate::hash::{BuildHasher, Hash, Hasher, SipHasher13};
use crate::iter::{FromIterator, FusedIterator};
use crate::ops::Index;
use crate::sys;

/// A hash map implemented with quadratic probing and SIMD lookup.
///
Expand Down Expand Up @@ -2783,13 +2782,24 @@ impl RandomState {
// iteration order allows a form of DOS attack. To counter that we
// increment one of the seeds on every RandomState creation, giving
// every corresponding HashMap a different iteration order.
thread_local!(static KEYS: Cell<(u64, u64)> = {
Cell::new(sys::hashmap_random_keys())
thread_local!(static KEYS: Cell<[u64; 2]> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change to an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was copied from the previous PR. I think at the time I introduced this change because [u64; 2] is slightly more idiomatic than (u64, u64).

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think (u64, u64) makes more sense because it's used to fill out a RandomState which consists of two separate u64s rather than an array, but it's not a huge deal.

let mut buf = [0u8; 16];
// Use a constant seed on `wasm32-unknown-unknown` and Hermit targets.
// FIXME: remove the Hermit part after its support will be added to `getrandom`
// FIXME: replace the WASM part with `cfg(target = "..")`:
newpavlov marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/rust-lang/rfcs/pull/2991
#[cfg(not(any(
all(target_arch = "wasm32", target_vendor = "unknown", target_os = "unknown"),
all(target_arch = "aarch64", target_os = "hermit"),
)))]
getrandom::getrandom(&mut buf).expect("failed to get system entropy");
Copy link
Member

Choose a reason for hiding this comment

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

It might be good to convert the error to an std::io::Error first before panicking on it. That way, it uses the Debug implementation of std's Error and we don't pull in any of getrandoms display/debug code.

let n = u128::from_ne_bytes(buf);
Cell::new([n as u64, (n >> 64) as u64])
});

KEYS.with(|keys| {
let (k0, k1) = keys.get();
keys.set((k0.wrapping_add(1), k1));
let [k0, k1] = keys.get();
keys.set([k0.wrapping_add(1), k1]);
RandomState { k0, k1 }
})
}
Expand Down
5 changes: 0 additions & 5 deletions library/std/src/sys/hermit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,6 @@ pub fn abort_internal() -> ! {
}
}

// FIXME: just a workaround to test the system
pub fn hashmap_random_keys() -> (u64, u64) {
(1, 2)
}

// This function is needed by the panic runtime. The symbol is named in
// pre-link args for the target specification, so keep that in sync.
#[cfg(not(test))]
Expand Down
18 changes: 0 additions & 18 deletions library/std/src/sys/sgx/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,24 +142,6 @@ pub extern "C" fn __rust_abort() {
abort_internal();
}

pub mod rand {
pub fn rdrand64() -> u64 {
unsafe {
let mut ret: u64 = 0;
for _ in 0..10 {
if crate::arch::x86_64::_rdrand64_step(&mut ret) == 1 {
return ret;
}
}
rtabort!("Failed to obtain random data");
}
}
}

pub fn hashmap_random_keys() -> (u64, u64) {
(self::rand::rdrand64(), self::rand::rdrand64())
}

pub use crate::sys_common::{AsInner, FromInner, IntoInner};

pub trait TryIntoInner<Inner>: Sized {
Expand Down
2 changes: 0 additions & 2 deletions library/std/src/sys/unix/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ pub use crate::os::redox as platform;
#[cfg(all(not(doc), target_os = "solaris"))]
pub use crate::os::solaris as platform;

pub use self::rand::hashmap_random_keys;
pub use libc::strlen;

#[macro_use]
Expand Down Expand Up @@ -65,7 +64,6 @@ pub mod os;
pub mod path;
pub mod pipe;
pub mod process;
pub mod rand;
pub mod rwlock;
pub mod stack_overflow;
pub mod stdio;
Expand Down
239 changes: 0 additions & 239 deletions library/std/src/sys/unix/rand.rs

This file was deleted.

4 changes: 0 additions & 4 deletions library/std/src/sys/unsupported/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ pub fn abort_internal() -> ! {
core::intrinsics::abort();
}

pub fn hashmap_random_keys() -> (u64, u64) {
(1, 2)
}

// This enum is used as the storage for a bunch of types which can't actually
// exist.
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
Expand Down
2 changes: 0 additions & 2 deletions library/std/src/sys/vxworks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

use crate::io::ErrorKind;

pub use self::rand::hashmap_random_keys;
pub use crate::os::vxworks as platform;
pub use libc::strlen;

Expand Down Expand Up @@ -41,7 +40,6 @@ pub mod path;
#[path = "../unix/pipe.rs"]
pub mod pipe;
pub mod process;
pub mod rand;
#[path = "../unix/rwlock.rs"]
pub mod rwlock;
#[path = "../unix/stack_overflow.rs"]
Expand Down
Loading