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

std::sys::windows::rand::hashmap_random_keys() panics on some systems #108059

Closed
marti4d opened this issue Feb 14, 2023 · 4 comments · Fixed by #108060
Closed

std::sys::windows::rand::hashmap_random_keys() panics on some systems #108059

marti4d opened this issue Feb 14, 2023 · 4 comments · Fixed by #108060
Labels
C-bug Category: This is a bug.

Comments

@marti4d
Copy link
Contributor

marti4d commented Feb 14, 2023

Hi there,

In Issue #94098, Firefox was running into a problem where any use of std::collections::HashMap was causing a crash on the machines of some small subset of users.

The root cause is that Bcrypt.dll or bcryptprimitives.dll will fail to load on some machines, causing any attempt to generate random numbers to also fail, leading Rust std to panic. Note that this was a regression from previous std behavior, and was caused when std made the choice to move away from RtlGenRandom to BCrypt as its only source of randomness on Win32 in #84096.

Note that this issue is likely not specific to Firefox -- It's likely any program written in Rust will have this exact same behavior on this small set of machines.

At this time, I had made PR #96917 to re-introduce RtlGenRandom as a "fallback" in case BCrypt failed to load.

Since then, I see that PR #102044 attempted to re-remove RtlGenRandom and changed the fallback to use BCrypt as well. Starting with the release of this change, we are seeing crashes again in Firefox due to failing RNG on these machines. See this Bug on Bugzilla.

It appears that if BCrypt's primary provider is broken, the fallback will likely be broken as well.

My suggestion is that we should once-again re-introduce RtlGenRandom() as a fallback mechanism.

Thanks,
Chris

@marti4d marti4d added the C-bug Category: This is a bug. label Feb 14, 2023
@marti4d
Copy link
Contributor Author

marti4d commented Feb 14, 2023

@ChrisDenton FYI

@ChrisDenton
Copy link
Member

Oh, I'll put a revert asap.

@marti4d
Copy link
Contributor Author

marti4d commented Feb 14, 2023

@ChrisDenton Thanks! I wouldn't do a full revert -- I think a lot of your change is good and should stay (like using the system-preferred RNG seems like a good idea!) -- I would just perhaps get rid of the current fallback and change it back to RtlGenRandom -- It's almost-certain that if the system-preferred RNG fails, BCRYPT_RNG_ALGORITHM will fail as well.

@marti4d
Copy link
Contributor Author

marti4d commented Feb 14, 2023

Ah, I see the revert still keeps the "preferred RNG" logic. Good stuff! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants