Windows: Use ProcessPrng for random keys by ChrisDenton · Pull Request #121337 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation41 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Windows 10 introduced ProcessPrng for random number generation. This allows us to replace the overly complicated (and prone to failure) BCryptGenRandom with a documented function.
For the tier 3 Windows 7 target, we simply use the older RtlGenRandom, which is undocumented. It should be fine even on modern systems (for comparability reasons) as it's just a wrapper for ProcessPrng. However, it does require loading an extra intermediary DLL which we can avoid when we know we have Windows 10+.
rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Operating system: Windows
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
cc @bjorn3, I've marked this as draft because this would introduce a single use of raw-dylib which isn't yet supported by cranelift. Would you be ok temporarily carrying a small patch until the raw-dylib work is done? Something like the following so as to use the older rng function we use on win7:
diff
diff --git a/library/std/src/sys/pal/windows/c.rs b/library/std/src/sys/pal/windows/c.rs index c773ede6341..cee8a74eee9 100644 --- a/library/std/src/sys/pal/windows/c.rs +++ b/library/std/src/sys/pal/windows/c.rs @@ -323,7 +323,7 @@ pub unsafe fn NtWriteFile(
// Use raw-dylib to import ProcessPrng as we can't rely on there being an import library. cfg_if::cfg_if! { -if #[cfg(not(target_vendor = "win7"))] { +if #[cfg(any())] { #[cfg(target_arch = "x86")] #[link(name = "bcryptprimitives", kind = "raw-dylib", import_name_type = "undecorated")] extern "system" { diff --git a/library/std/src/sys/pal/windows/rand.rs b/library/std/src/sys/pal/windows/rand.rs index 74f26c28dd0..a755c08682e 100644 --- a/library/std/src/sys/pal/windows/rand.rs +++ b/library/std/src/sys/pal/windows/rand.rs @@ -1,7 +1,7 @@ use crate::mem; use crate::sys::c;
-#[cfg(not(target_vendor = "win7"))] +#[cfg(any())] #[inline] pub fn hashmap_random_keys() -> (u64, u64) { let mut v = (0, 0); @@ -12,7 +12,6 @@ pub fn hashmap_random_keys() -> (u64, u64) { v }
-#[cfg(target_vendor = "win7")] pub fn hashmap_random_keys() -> (u64, u64) { use crate::ffi::c_void; use crate::io;
This comment has been minimized.
Do we include the generated import library in the rlib for the crate that uses raw-dylib? Or do we always generate it as part of invoking the linker? If the latter, this would break cg_clif when using an unpatched LLVM compiled stdlib, like would be done for the rustc-codegen-cranelift-preview rustup component. If the former, I did be fine with carrying a patch for now.
I'd have to check up on that. I know if you compile a staticlib it gets included rather than being a separate import lib so I'd assumed rlibs worked similarly but I don't actually know.
Ok, I confirmed that the imports are generated and included in the std rlib. I also tested to make sure cranelift builds with it. With the additional patch, both --sysroot clif and --sysroot llvm work.
This comment has been minimized.
Marking as ready for review. This might possibly ping everyone...
The miri changes just add a new shim function for ProcessPrng, which is essentially the same as RtlGenRandom but with slightly different argument types.
The Miri subtree was changed
cc @rust-lang/miri
Some changes occurred in compiler/rustc_codegen_cranelift
cc @bjorn3
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me on the Miri part.
Should we keep support for SystemFunction036 and BCryptGenRandom or get rid of it? They are likely untested now.
Should we keep support for SystemFunction036 and BCryptGenRandom or get rid of it? They are likely untested now.
I would suggest keeping them for awhile as one or the other are likely to still be used outside of std for some time to come.
📌 Commit 292b4a3 has been approved by Mark-Simulacrum
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
This is essentially the same as SystemFunction036 (aka RtlGenRandom) except that the given length is a usize instead of a u32
This PR was just broken by the switch to using addr_of_mut! instead of casting a reference. Easy enough to rebase on.
@bors r=Mark-Simulacrum
📌 Commit 843eaf2 has been approved by Mark-Simulacrum
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
@klensy most relevant to Rust is that BCryptGenRandom was failing often enough to be a top a cause of statup crashes in Firefox (hence the need for a fallback). On a more minor note it's also an overly complex function for our needs.
Finished benchmarking commit (34aab62): comparison URL.
Overall result: ❌ regressions - no action needed
@rustbot label: -perf-regression
Instruction count
This is a highly reliable metric that was used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 0.5% | [0.5%, 0.5%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Max RSS (memory usage)
This benchmark run did not return any relevant results for this metric.
Cycles
This benchmark run did not return any relevant results for this metric.
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 651.624s -> 651.457s (-0.03%)
Artifact size: 311.12 MiB -> 311.11 MiB (-0.00%)
No problem! This is something I wanted for years but it took a lot of things coming together before I could and even then there were issues. I should say also say thanks to chromium as my PR adding ProcessPrng to Wine didn't get merged until chromium broke without it.
The issue at hand revolves around the static import of ProcessPrng creating compatibility problems. While Windows 7 and Windows 8 are not officially supported, the resulting compiled program, without further modifications, fails to run on these versions of Windows as well as on Wine. This situation is distinct from typical compatibility issues, as it effectively discontinues support, forcing users to produce multiple versions of the program. Similar to how ARM platform users must generate separate executables, users would need to create multiple .exe files and let end-users choose which one to download.
A more conventional approach would involve the operating system dynamically selecting and loading functions based on its version. For instance, for versions below Windows 10, RtlGenRandom could be used, while ProcessPrng could be utilized for versions above. This method would at least allow the compiled executables to potentially run on products with Windows versions lower than 10. The responsibility of assessing the performance of these executables on different Windows versions would then fall to the developers. If static import is maintained, the compiled executables would simply fail to execute.
Have you read https://blog.rust-lang.org/2024/02/26/Windows-7.html?
For Windows 7 the x86_64-win7-windows-msvc tier 3 target has been added. As I understand it the aim is to make them tier 2 in the future. We can't make it tier 1 as Github Actions simply doesn't provide Windows 7 VM's anymore and thus there is no way for us to actually test on Windows 7, which is the main requirement that distinguishes tier 1 from tier 2.
as well as on Wine.
The following shim works on Wine while it doesn't support ProcessPrng yet: https://github.com/rust-lang/rustc_codegen_cranelift/blob/master/patches/bcryptprimitives.rs See https://github.com/rust-lang/rustc_codegen_cranelift/blob/f7cc528deb35a8cd92ab438f62414866c200a848/.github/workflows/main.yml#L115-L117 for how to build and use it.
Yes, I've learned that Windows is already a third-tier target, but not yet. The key is that there is a difference between the problems caused by direct static import of this function and the compatibility issues. Why not change it to compat_fn_with_fallback to ensure that the most obvious difference is that the exe compiled by rust cannot run under Windows 10, instead of an error. If If something goes wrong, developers can still handle it themselves.
Just like the wine mentioned, if during this time period, the user needs to use the latest version of rust, and wine is not quickly integrated into it, plus the time difference between various releases, it will be very uncomfortable to be stuck. You should ensure that It is basically usable. As for whether there are any functional problems, this can be bypassed by other methods.
users would need to create multiple .exe files and let end-users choose which one to download.
Correct. That is a consequence of splitting Windows 7 into a separate target. That's an expected part of the outcome here. It's not great for Windows 7 users, but there's a trade-off here between supporting users on ancient unsupported OS versions vs making Rust work well and be easily maintainable on modern OS versions, and the Rust project decided that this is the way they want to resolve the trade-off.
What you are asking for would be equivalent to keeping Windows 7 support in the tier 1 target, which was explicitly rejected as an alternative as it is considered to cause an undue maintenance burden, and because it raises false expectations about how well Rust is going to work on Windows 7.
We're also talking about an OS that receives no more security updates! It is a bad idea to provide any sort of encouragement for such an OS to remain in productive use. You can keep using old Rust versions if you really need to; they are unsupported but then so is the OS these programs are apparently meant to run on.
It's very sad to see things like this happen. It's not about not actively maintaining/testing previous versions, it's "deliberate sabotage" - with no apparent benefit. Even though it's in Microsoft's interest to kill off "deprecated" OSes, it should not be in anyone else's interest. So, why this eagerness to "help" them?
There are many reasons why people can't or won't upgrade. The lack of security updates often isn't very relevant, it all depends on what the computer is used for. But, the fact that a programming language makes it very difficult to make software that will work under such circumstances, when it would have been a minimal burden to make most things work as they used to, really disqualifies it as a programming language in my opinion. Rust is not alone in this, but that doesn't really make the matter any less serious.
with no apparent benefit
There is a benefit. It is spelled out in the decision process. See rust-lang/compiler-team#651 for some more background. There is ofc also a downside. These decisions are not taken lightly.
a minimal burden
This does not reflect reality. It's hard to see from the outside how much work goes into supporting a platform. The burden got so big that t-libs decided too much is too much, they'd rather spend their limited resources on making Rust better for modern platforms that are actually still supported than to keep legacy platforms alive. Anyway discussion on a closed PR is not really helpful.
Labels
This PR was explicitly merged by bors.
Operating system: Windows
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.