Make HashMap fall back to RtlGenRandom if BCryptGenRandom fails

[marti4d]

With PR #84096, Rust std::collections::hash_map::RandomState changed from using RtlGenRandom() (msdn) to BCryptGenRandom() (msdn) as its source of secure randomness after much discussion (here, among other places).

Unfortunately, after that PR landed, Mozilla Firefox started experiencing fairly-rare crashes during startup while attempting to initialize the env_logger crate. (docs for env_logger) The root issue is that on some machines, BCryptGenRandom() will fail with an Access is denied. (os error 5) error message. (Bugzilla issue 1754490) (Discussion in issue #94098)

Note that this is happening upon startup of Firefox's unsandboxed Main Process, so this behavior is different and separate from previous issues (like this) where BCrypt DLLs were blocked by process sandboxing. In the case of sandboxing, we knew we were doing something abnormal and expected that we'd have to resort to abnormal measures to make it work.

However, in this case we are in a regular unsandboxed process just trying to initialize env_logger and getting a panic. We suspect that this may be caused by a virus scanner or some other security software blocking the loading of the BCrypt DLLs, but we're not completely sure as we haven't been able to replicate locally.

It is also possible that Firefox is not the only software affected by this; we just may be one of the pieces of Rust software that has the telemetry and crash reporting necessary to catch it.

I have read some of the historical discussion around using BCryptGenRandom() in Rust code, and I respect the decision that was made and agree that it was a good course of action, so I'm not trying to open a discussion about a return to RtlGenRandom(). Instead, I'd like to suggest that perhaps we use RtlGenRandom() as a "fallback RNG" in the case that BCrypt doesn't work.

This pull request implements this fallback behavior. I believe this would improve the robustness of this essential data structure within the standard library, and I see only 2 potential drawbacks:

1. Slight added overhead: It should be quite minimal though. The first call to sys::rand::hashmap_random_keys() will incur a bit of initialization overhead, and every call after will incur roughly 2 non-atomic global reads and 2 easily predictable branches. Both should be negligible compared to the actual cost of generating secure random numbers
2. RtlGenRandom() is deprecated by Microsoft: Technically true, but as mentioned in this comment on GoLang, this API is ubiquitous in Windows software and actually removing it would break lots of things. Also, Firefox uses it already in our C++ code, and Chromium uses it in their code as well (which transitively means that Microsoft uses it in their own web browser, Edge). If there did come a time when Microsoft truly removes this API, it should be easy enough for Rust to simply remove the fallback in the code I've added here

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

[ChrisDenton]

Ok, I think this is good to go.

I understand the concerns expressed in #94098 and that ideally this should not be necessary. However, I think @marti4d makes a strong case that a fallback is in fact needed at this time. It is causing issues for a small but significant number of users and while it would be great if the OS itself could fix or workaround whatever is causing the issue (possibly AVs), that's not happening at the moment. This decision could be revisited in the future if further information comes to light.

@bors r+

[bors]

📌 Commit 3de6c2c has been approved by ChrisDenton

[ChrisDenton]

@bors r- Improve error message

[ChrisDenton]

@bors r+

[ChrisDenton]

Oh, that didn't work. Let's try bors again.

@bors r+

[bors]

📌 Commit aba3454 has been approved by ChrisDenton

[bors]

⌛ Testing commit aba3454 with merge 72599de5af3d1060893084b6433650cde782a9fb...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

.......... (60/64)
...       (64/64)


/checkout/src/test/rustdoc-gui/search-tab-change-title-fn-sig.goml search-tab-change-title-fn-sig... FAILED
[ERROR] (line 6) TimeoutError: waiting for selector "#titles" failed: timeout 30000ms exceeded: for command `wait-for: "#titles"`
Build completed unsuccessfully in 0:00:45

[ChrisDenton]

That failure looks spurious to me. At least I don't think it could be caused by this PR. Let's try again.

@bors retry spurious src/test/rustdoc-gui/search-tab-change-title-fn-sig.goml