Use getrandom for generating HashMap seed #80149

newpavlov · 2020-12-18T09:31:30Z

Closes: #62079

Note that in future versions of getrandom we plan to remove Windows XP support, thus std should not update it from v0.2.0 until rust-lang/compiler-team#378 gets resolved. Also ideally it should be done together with updating wasi to v0.10.

Since the rdrand feature is enabled unconditionally, RDRAND will be used not only on SGX targets, but also on all x86(-64) targets which are not supported in getrandom by default (e.g. UEFI and Hermit targets, see rust-random/getrandom#61).

As mentioned in the previous PR discussion, if it will be deemed necessary, we (getrandom maintainers) are ready to transfer getrandom to the rust-lang organization.

Blocked on m-ou-se/getrandom#1

rust-highfive · 2020-12-18T09:31:35Z

r? @shepmaster

(rust-highfive has picked a reviewer for you, use r? to override)

shepmaster · 2021-01-01T20:04:30Z

This is a bit too deep for me...

r? @joshtriplett

src/tools/tidy/src/pal.rs

library/std/src/sys/windows/pipe.rs

library/std/src/collections/hash/map.rs

josephlr · 2021-01-05T10:52:14Z

library/std/src/collections/hash/map.rs

@@ -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]> = {


Why the change to an array?

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).

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.

newpavlov · 2021-02-09T09:54:32Z

r? @sfackler

sfackler · 2021-02-10T00:57:39Z

@Mark-Simulacrum what are the procedures to pull in a new std dependency?

Mark-Simulacrum · 2021-02-10T01:03:13Z

I'll assign myself for now and get back to you soon.

Mark-Simulacrum · 2021-02-12T01:04:47Z

Can you clarify whether you mean the mechanics (i.e., infrastructure of adding deps in terms of toml files to edit and such) or policy questions? I think the first is likely working in this PR already, and the latter would need more time on my side to answer properly.

sfackler · 2021-02-12T01:08:13Z

Yeah I'm mostly wondering about the policy side, since CI should nail down the functionality.

bors · 2021-04-05T16:46:07Z

☔ The latest upstream changes (presumably #83890) made this pull request unmergeable. Please resolve the merge conflicts.

newpavlov · 2021-04-05T20:29:43Z

@m-ou-se
Any updates?

m-ou-se · 2021-04-11T12:30:28Z

@newpavlov I reviewed the getrandom crate. See my comments here: m-ou-se/getrandom#1

Overall the quality looks good, and I'd be happy to see this under rust-lang/getrandom.

There's a few important differences in behaviour if we switch std to use getrandom, which are important do discuss before merging this PR.

Most importantly, std does not block and just falls back to urandom on Linux even if it's not yet properly seeded. (See m-ou-se/getrandom#1 (comment).) I'm not saying that's a good thing, but this will be a noticable change in behaviour.

Also, std currently simply picks (1, 2) as the random keys for hashmaps on the unsupported platforms. This PR keeps that behaviour only for specifically wasm32-unknown-unknown and aarch64-hermit, whereas right now that behaviour is selected for any unknown platform. This means that any unsupported platform other than these two will stop compiling.

m-ou-se · 2021-04-11T12:35:55Z

Is there any desire to change the API? Hopefully not significantly since it has already undergone several revisions, and since rand is somewhat dependent on it (especially the error type).

I think there's one change/addition to the API that could be somewhat useful: a way to read into uninitialized buffers. E.g. by accepting a &mut [MaybeUninit<u8>] or something like proposed in rfc 2930.

m-ou-se · 2021-04-11T13:01:17Z

library/std/src/collections/hash/map.rs

+                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");


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.

bors · 2021-04-22T07:53:25Z

☔ The latest upstream changes (presumably #84411) made this pull request unmergeable. Please resolve the merge conflicts.

crlf0710 · 2021-05-07T16:30:26Z

Marking this as S-blocked.
@newpavlov The next steps would be dealing with m-ou-se/getrandom#1 , i guess?

gilescope · 2021-10-18T21:33:58Z

"Use BCryptGenRandom instead of RtlGenRandom on Windows." merged. nice - that's great news. It's getting there...

cameronelliott · 2022-11-09T01:01:15Z

Will this change the availability of HashMap with regards to no_std ?
I just submitted a PR for the embedded book to clear up some confusion regarding that.
rust-embedded/book#274
rust-embedded/book#333

bjorn3 · 2022-11-09T10:27:44Z

No, getting randomness needs interaction with an OS on every architecture other than newish x86 systems (through rdrand). #![no_std] does not assume the presence of any particular OS or even any OS at all.

therealprof · 2022-11-09T16:27:10Z

No, getting randomness needs interaction with an OS on every architecture other than newish x86 systems (through rdrand). #![no_std] does not assume the presence of any particular OS or even any OS at all.

So in what way does the HashMap implementation in std differ from hashbrown which it has been based on (and which only requires alloc to work)?

bjorn3 · 2022-11-09T16:37:02Z

libstd's HashMap is a wrapper around hashbrown that uses a hasher by default using randomness to ensure hashdos resistence.

therealprof · 2022-11-09T16:40:37Z

libstd's HashMap is a wrapper around hashbrown that uses a hasher by default using randomness to ensure hashdos resistence.

Makes sense, but why can't alloc export the HashMap (and HashSet) and std only adds the wrapper on top of that?

bjorn3 · 2022-11-09T17:06:36Z

That would make alloc::collections::HashMap and std::collections::HashMap permanently incompatible with each other. This is bound to give a lot of confusion and would prevent merging them in the future if we find a way to support this. For example by allowing the user to specify the rng using a mechanism similar to #[global_allocator].

therealprof · 2022-11-09T17:36:07Z

Fair point, OTOH this is adding a bit of confusion because people can't just opt into an alloc crate to get the same collections as std provides but instead have to use an external crate which describes itself as the default HashMap of Rust. Can we add additional API to provide raw access to the hashbrown methods which and simply remove the wrapper on no_std targets?

bjorn3 · 2022-11-09T17:48:44Z

Can we add additional API to provide raw access to the hashbrown methods which and simply remove the wrapper on no_std targets?

Hashbrown doesn't have the same stability guarantees as rust and it exports a lot of methods that are unstable or non-existent in the libstd version. Hiding those cq giving them an #[unstable] stability attribute and making sure that hashbrown api changes don't affect libstd users is one of the main purposes of the wrapper. If you keep a wrapper, but require the user to specify a hasher rather than using the hashdos resistent default hasher of std::collections::HashMap I don't see the difference with providing a separate alloc::collections::HashMap and std::collections::HashMap type.

scottmcm · 2023-01-08T20:54:29Z

Ideally we'd be able to move the wrapper to alloc and have the std be a type alias that sets the S = RandomState, so you could still write hasher-generic code against just alloc.

AFAIK that doesn't work right now, though, because of some coherence restrictions.

newpavlov · 2023-04-02T03:27:07Z

This PR is significantly outdated, so I will close it. I will create a new PR after we address the @m-ou-se's review.

rust-highfive assigned shepmaster Dec 18, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2020

This comment has been minimized.

Sign in to view

newpavlov force-pushed the getrandom2 branch from a336d08 to 25309a4 Compare December 18, 2020 09:39

This comment has been minimized.

Sign in to view

Use getrandom for generating HashMap seed

688ba4b

newpavlov force-pushed the getrandom2 branch from 25309a4 to 688ba4b Compare December 18, 2020 09:56

This comment has been minimized.

Sign in to view

fix cfg

7d0be77

This comment has been minimized.

Sign in to view

replace TODO with FIXME

636bbf4

newpavlov mentioned this pull request Dec 19, 2020

Use getrandom crate #62082

Closed

rust-highfive assigned joshtriplett and unassigned shepmaster Jan 1, 2021

josephlr reviewed Jan 5, 2021

View reviewed changes

newpavlov added 3 commits January 8, 2021 20:52

Change u64 to usize as per @josephlr's proposal

0117047

fix path

528fb0d

link RFC 2991 instead of the feature request issue

582566d

rust-highfive assigned sfackler and unassigned joshtriplett Feb 9, 2021

Mark-Simulacrum self-assigned this Feb 10, 2021

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 12, 2021

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 5, 2021

Merge branch 'master' into getrandom2

2011fe5

m-ou-se reviewed Apr 11, 2021

View reviewed changes

josephlr mentioned this pull request Apr 29, 2021

Hermit is not currently supported by getrandom rust-random/getrandom#199

Closed

crlf0710 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 7, 2021

This was referenced Oct 15, 2021

Towards rand 1.0 rust-random/getrandom#232

Open

Use BCryptGenRandom instead of RtlGenRandom on Windows. #84096

Merged

commial mentioned this pull request Jan 6, 2022

Update rand requirement from 0.7 to 0.8 ANSSI-FR/MLA#71

Closed

briansmith mentioned this pull request Oct 6, 2022

Add function which returns random array? rust-random/getrandom#281

Closed

joboet mentioned this pull request Jan 8, 2023

Provide access to the system's random number generator rust-lang/libs-team#159

Open

newpavlov closed this Apr 2, 2023

newpavlov deleted the getrandom2 branch April 2, 2023 03:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getrandom for generating HashMap seed #80149

Use getrandom for generating HashMap seed #80149

newpavlov commented Dec 18, 2020 •

edited by Dylan-DPC

Loading

rust-highfive commented Dec 18, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shepmaster commented Jan 1, 2021

josephlr Jan 5, 2021

newpavlov Jan 8, 2021

josephlr Jan 14, 2021

newpavlov commented Feb 9, 2021

sfackler commented Feb 10, 2021

Mark-Simulacrum commented Feb 10, 2021

Mark-Simulacrum commented Feb 12, 2021

sfackler commented Feb 12, 2021

bors commented Apr 5, 2021

newpavlov commented Apr 5, 2021

m-ou-se commented Apr 11, 2021

m-ou-se commented Apr 11, 2021

m-ou-se Apr 11, 2021

bors commented Apr 22, 2021

crlf0710 commented May 7, 2021

gilescope commented Oct 18, 2021

cameronelliott commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

therealprof commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

therealprof commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

therealprof commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

scottmcm commented Jan 8, 2023

newpavlov commented Apr 2, 2023

Use getrandom for generating HashMap seed #80149

Use getrandom for generating HashMap seed #80149

Conversation

newpavlov commented Dec 18, 2020 • edited by Dylan-DPC Loading

rust-highfive commented Dec 18, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shepmaster commented Jan 1, 2021

josephlr Jan 5, 2021

Choose a reason for hiding this comment

newpavlov Jan 8, 2021

Choose a reason for hiding this comment

josephlr Jan 14, 2021

Choose a reason for hiding this comment

newpavlov commented Feb 9, 2021

sfackler commented Feb 10, 2021

Mark-Simulacrum commented Feb 10, 2021

Mark-Simulacrum commented Feb 12, 2021

sfackler commented Feb 12, 2021

bors commented Apr 5, 2021

newpavlov commented Apr 5, 2021

m-ou-se commented Apr 11, 2021

m-ou-se commented Apr 11, 2021

m-ou-se Apr 11, 2021

Choose a reason for hiding this comment

bors commented Apr 22, 2021

crlf0710 commented May 7, 2021

gilescope commented Oct 18, 2021

cameronelliott commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

therealprof commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

therealprof commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

therealprof commented Nov 9, 2022

bjorn3 commented Nov 9, 2022

scottmcm commented Jan 8, 2023

newpavlov commented Apr 2, 2023

newpavlov commented Dec 18, 2020 •

edited by Dylan-DPC

Loading