Closed josephlr closed 1 month ago
It looks like this won't be quite as easy as we would like. The main issue is that BCryptPrimitives.dll
doesn't have a corresponding library (.lib
file), so we can't just use link(name="bcryptprimitives")
. We are left with 4 options:
#[link(kind = "raw-dylib")]
dlltool
during build time (on *-windows-gnu
targets)windows-sys
crate
libc
so thats fine)LoadLibrary
and GetProcAddress
.lib
file like the windows
crate does
LoadLibrary
and similar functions..lib
files.The fourth option would be to include your own pre-built .lib
file (possibly generated by using raw-dylib
and storing the output or else copying the windows-rs lib generator code).
Adds call to dlltool during build time
Note that msvc targets don't require dlltool.
The fourth option would be to include your own pre-built
.lib
file (possibly generated by usingraw-dylib
and storing the output or else copying the windows-rs lib generator code).Adds call to dlltool during build time
Note that msvc targets don't require dlltool.
Thanks! Updated the list. I do think that maintaining a collection of .lib
files for each windows target is a lot more effort than just writing the ~20 lines of DLL loading code.
@ChrisDenton I have a question. If getrandom
lazily calls LoadLibraryA
/GetProcAddress
on first use, while libstd
uses raw-dylib
, will this cause any issues? When experimenting, it seems like LoadLibraryA
/GetProcAddress
always return the same handle/address when run in the same process, so I think there's some sort of caching?
@newpavlov @ChrisDenton I thinking of the following approach which would combine approaches (1) and (3).
build.rs
script to check the compiler version:
cargo::rustc-cfg=raw_dylib
if version >= 1.65 (See https://github.com/rust-lang/rust/pull/99916)cargo::rustc-cfg=raw_dylib_x86
if version >= 1.71 (See https://github.com/rust-lang/rust/pull/109677)cfg_if
block like:
cfg_if! {
if #[cfg(all(raw_dylib, not(target_arch = "x86")))] {
#[link(name = "bcryptprimitives", kind = "raw-dylib")]
extern "system" { fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; }
} else if #[cfg(all(raw_dylib_x86, target_arch = "x86"))] {
#[link(name = "bcryptprimitives", kind = "raw-dylib", import_name_type = "undecorated")]
extern "system" { fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; }
} else {
unsafe fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL {
// Do the lazy DLL loading
}
}
}
@ChrisDenton I have a question. If getrandom lazily calls LoadLibraryA/GetProcAddress on first use, while libstd uses raw-dylib, will this cause any issues? When experimenting, it seems like LoadLibraryA/GetProcAddress always return the same handle/address when run in the same process, so I think there's some sort of caching?
There's no problem with that. A DLL is only loaded once. Loading it more times indeed just returns the same module handle and increases the reference count.
@josephlr
combine approaches (1) and (3)
It would introduce a build script which will do nothing on all other targets.
We may consider delaying migration to ProcessPrng
until getrandom v0.3
, which can have a sufficiently high MSRV. Considering that rand
is in the process of preparing new v0.9 release, it may be a good time to cut v0.3 for us as well.
@newpavlov good idea. Looks like Rand 0.9's MSRV is 1.61, do you know if 1.71 would be too high?
I think regardless, we should move to ProcessPrng, and just always do the DLL loading (I can put up a draft PR to see how it looks).
I should add that the standard library is in general moving away from dynamic dll loading because it causes issues when used in some scenarios. E.g. attempting to load a library while the loader lock is already held can cause a deadlock or crash. This is especially important for std because HashMap
s are used everywhere and, by default, use RandomState
.
Thanks for the heads up @ChrisDenton getrandom
is also a transitive dependency of most crates in practice, so we also want to be sensitive to these concerns.
I should add that the standard library is in general moving away from dynamic dll loading because it causes issues when used in some scenarios. E.g. attempting to load a library while the loader lock is already held can cause a deadlock or crash. This is especially important for std because
HashMap
s are used everywhere and, by default, useRandomState
.
Do you know if there is any way to call ProcessPrng
on versions of Rust before 1.71 without loading a DLL? Or if there's a way to check if a DLL is already loaded (which it almost always will be) without calling LoadLibraryA
?
In general, I wouldn't think that the approach taken by Go and BoringSSL would break too many things in practice, but it might just be that Google doesn't run into those specific problems. It might be the case that moving to ProcessPRNG
with DLL loading isn't prefect, but fixes more problems than it creates.
Alternatively, we could just bump the MSRV to 1.71 on Windows, and have a lower MSRV on all other platforms.
Do you know if there is any way to call ProcessPrng on versions of Rust before 1.71 without loading a DLL?
The only way would be to use a pre-created lib file in one of the ways listed above (i.e. using the windows-targets crate or distributing a lib file with this crate).
Or if there's a way to check if a DLL is already loaded (which it almost always will be) without calling LoadLibraryA?
GetModuleHandleExA
can be used to get a module that's already loaded.
In general, I wouldn't think that the approach taken by Go and BoringSSL would break too many things in practice, but it might just be that Google doesn't run into those specific problems. It might be the case that moving to ProcessPRNG with DLL loading isn't prefect, but fixes more problems than it creates.
For sure. It's fine in the vast majority of cases. It's only really TLS constructors/destructors or pre/post C main that may be an issue and only then if the DLL is not already loaded. Rust itself does not have any built-in support for TLS constructors (that's done lazily) or for running code pre/post main. However, people do use crates that implement such things. One example was someone setting up a logger when a DLL is loaded where the logger used a HashMap
.
I imagine many sandboxes would block LoadLibrary
too. Also, some projects do try to avoid dynamic loading in general. It would be good to have a way to opt out of using LoadLibrary
, e.g. by having a msrv-1_71
feature.
@josephlr wrote:
@newpavlov good idea. Looks like Rand 0.9's MSRV is 1.61, do you know if 1.71 would be too high?
Many projects are sticking to MSRV <= 1.63 for the purpose of supporting Debian 8 which is out of mainstream support but which has some kind of extended (paid) support. Even some people who are infamous for their historical MSRV policies are at least trying to do so, so I imagine it is likely going to be difficult to move the MSRV beyond 1.63. I do support moving the MSRV to 1.63 though.
If we want to support Rust 1.63, then using windows-sys
probably will be the best option. I would prefer to not introduce a Windows-specific feature.
Would code based on windows-sys
have the same difficulties with sandboxes?
If we want to support Rust 1.63, then using windows-sys probably will be the best option. I would prefer to not introduce a Windows-specific feature.
I am using windows-sys
in ring, but I am considering removing the dependency in favor of my own extern "C"
bindings, because when MS updates windows-sys to a new version that isn't SemVer-compatible with the previous, then people feel the need to have ring updated with a new Cargo.toml that references the latest version. Otherwise, they end up having two versions of windows-sys in their dependency tree. This would be especially problematic if the newest version of windows-sys were to ever have an MSRV that is higher than ring's. It's probably less work for everybody to just avoid windows-sys for the small amounts of functionality that ring needs.
I expect that if getrandom
were to use windows-sys, we'd end up with the same situation as ring.
Build our own .lib file like the windows crate does
- Allows us to avoid calling LoadLibrary and similar functions.
- Requires maintaining libraries for the ~8 different windows targets
- Requires figuring out how to build .lib files.
It looks like windows-rs checks the .lib files into version control. I think we could have a script that uses libtool to extract the minimal info from the windows-rs .lib files into a corresponding tiny set of .lib files that we can then embed in getrandom. We could then just check in the minimal lib files into Git like windows-rs does. Because they will be tiny, it would be fine to have them all in one crate.
In the interim, why not add a "msrv_1_71" feature to getrandom
. When built with msrv_1_71
, we use #[link(kind = "raw-dylib")
to get ProcessPRNG
. By default, without the feature flag set, keep using RtlGenRandom
. When we have a better solution, msrv_1_71 can become a no-op.
ProcessPRNG was introduced in Windows 10, which is the minimum supported Windows for most Rust targets. Upstream change: https://github.com/rust-lang/rust/pull/121337
The minimum version of Windows supported in the latest version of Rust (actually, 1.78.0+) is Windows 10, so it makes sense to only use ProcessPRNG when building with Rust 1.78.0 and later. However, the minimum version of Windows supported by getrandom
is dictated by the MSRV of getrandom
, which is Windows 7. So, unless we're increasing our MSRV past 1.78 we still need to support Windows 7 by default.
In the interim, why not add a "msrv_1_71" feature to getrandom. When built with msrv_1_71, we use #[link(kind = "raw-dylib") to get ProcessPRNG. By default, without the feature flag set, keep using RtlGenRandom. When we have a better solution, msrv_1_71 can become a no-op.
Accordingly, I instead suggest having a msrv_1_78
feature, since we can't statically link with ProcessPRNG
until we know we're being built with Rust 1.78 or later.
However, the minimum version of Windows supported by getrandom is dictated by the MSRV of getrandom, which is Windows 7
Not quite. We reserve the right to bump platform requirements, if the latest stable version of Rust did it. For example, we have dropped Windows XP support, even though Rust 1.36 theoretically supports it.
Here would be my proposal (which would simplify our implementation and maintenance burden):
v0.2
getrandom
(either v0.3
or v1.0
)ProcessPRNG
on normal *-windows-*
targets with "raw-dylib"
*-windows-*
targets) to Windows 10RtlGenRandom
on the win7
targetsNot quite. We reserve the right to bump platform requirements, if the latest stable version of Rust did it. For example, we have dropped Windows XP support, even though Rust 1.36 theoretically supports it.
One advantage of getting 1.0 released, is that we can simplify our MSRV policy to be "we can drop platforms or increase the MSRV in a minor version release, but won't do so in a patch release"
I would prefer to not release 1.0 at the very least until the MSRV-aware resolver lands on stable and we are ready to bump crate's MSRV to encompass it. We also have some unresolved API questions like #365 and #281.
Bump the MSRV to 1.65 (noting that the MSRV is 1.71 if targeting x86 Windows)
This is still higher than 1.63 mentioned above and having a higher MSRV for a Tier 1 target is really undesirable.
- Release the next breaking change of getrandom (either v0.3 or v1.0)
I think we should avoid a SemVer-incompatible version upgrade if we can.
Just to clarify my above comments, I don't object to bumping the Windows minimum supported version to Windows 10. Just, we should clarify the communication as the initial comment seemed to imply that doing so follows from what Rust 1.78 does, rather than us thinking it's a good idea independently even for older versions of Rust.
Bump the MSRV to 1.65
What is the benefit of a MSRV 1.65 vs 1.63?
I don't see any reason to have an MSRV less than #426. I don't think there's much reason right now to have an MSRV greater than 1.57 except for this raw-dylib issue.
(noting that the MSRV is 1.71 if targeting x86 Windows)
I think we should explore @ChrisDenton's ideas of creating a MSRV-1.63-compatible way to statically link bcryptprimitives.dll/ProcessPRNG instead.
I don't know if it would actually be problematic to have an MSRV higher than 1.63 for non-Linux targets, but having different MSRVs for different targets seems likely to cause pain.
I think we should avoid a SemVer-incompatible version upgrade if we can.
I think we've considering bumping the MSRV at all in the past a breaking change (@newpavlov can confirm). If we stuck to that, any increase from the current MSRV 1.36 would be breaking. I'd be fine increasing the MSRV in a patch though. If you are using a rust version that old, you're almost certainly vendoring your deps as well.
Just to clarify my above comments, I don't object to bumping the Windows minimum supported version to Windows 10. Just, we should clarify the communication as the initial comment seemed to imply that doing so follows from what Rust 1.78 does, rather than us thinking it's a good idea independently even for older versions of Rust.
Makes sense. We should drop platforms because it makes sense. Windows 10 is ~9 years old, Windows 8.1 is EOL, and we would still have a way to target Windows 7 and 8, you would just need to specify a different target.
What is the benefit of a MSRV 1.65 vs 1.63?
I don't see any reason to have an MSRV less than #426. I don't think there's much reason right now to have an MSRV greater than 1.57 except for this raw-dylib issue.
I don't know if it would actually be problematic to have an MSRV higher than 1.63 for non-Linux targets, but having different MSRVs for different targets seems likely to cause pain.
1.65 stabilized raw-dylib
for non-x86 targets, but I think you are right here. If we want to implement this in terms of raw-dylib
, we should just increase the MSRV to 1.71.
I think we should explore @ChrisDenton's ideas of creating a MSRV-1.63-compatible way to statically link bcryptprimitives.dll/ProcessPRNG instead.
I think if we just depend on windows-targets
we could have an MSRV as low as 1.56.
OK I updated #415 to use windows-targets
and it seems to give a very clean and simple implementation. It would raise the MSRV to 1.56 though. windows-targets
uses edition=2021
. Looking at the implementation of that crate, it would still compile (with very minor changes) on edition=2018
all the way back to 1.31.
So we can either try to get windows-targets
to use edition=2018
, or we can bump our MSRV.
At this point I think doing LoadLibrary
shenanigans doesn't make a lot of sense. This leaves the options of:
windows-targets
crate (MSRV 1.56)"raw-dylib"
(MSRV 1.71, and other downsides noted in https://github.com/rust-random/getrandom/issues/414#issuecomment-2085017443)Personally I would prefer the first option (use windows-targets
), but I'd be interested in what y'all think.
I think we should avoid a SemVer-incompatible version upgrade if we can.
I think we've considering bumping the MSRV at all in the past a breaking change (@newpavlov can confirm). If we stuck to that, any increase from the current MSRV 1.36 would be breaking. I'd be fine increasing the MSRV in a patch though. If you are using a rust version that old, you're almost certainly vendoring your deps as well.
Treating conservative increases in MSRV as SemVer-breaking changes creates more problems than it solves. Realistically speaking, many, many projects I'm part of or am closely observing have a MSRV of 1.63. Including in particular cc-rs. So I think increasing the MSRV to 1.57 without bumping the SemVer minor version is the right thing to do. Note that Rust 1.57 was released Dec 2, 2021, about 2.5 years ago.
1.65 stabilized
raw-dylib
for non-x86 targets, but I think you are right here. If we want to implement this in terms ofraw-dylib
, we should just increase the MSRV to 1.71.
Using windows-sys/targets is much better than bumping MSRV to 1.64/1.71, regardless of how the version number of getrandom
is updated.
I think we should explore @ChrisDenton's ideas of creating a MSRV-1.63-compatible way to statically link bcryptprimitives.dll/ProcessPRNG instead.
I think if we just depend on
windows-targets
we could have an MSRV as low as 1.56.
I think that is fine. As I mentioned in the PR, there's no reason AFAICT to prefer windows-targets over windows-sys since they seem to do the version bumping in sync with each other. Using windows-targets directly seems worse.
So we can either try to get
windows-targets
to useedition=2018
, or we can bump our MSRV.
I don't think we should be bothering anybody with this. I feel like any effort towards that is would be wasting people's effort for no real benefit.
Treating conservative increases in MSRV as SemVer-breaking changes creates more problems than it solves. Realistically speaking, many, many projects I'm part of or am closely observing have a MSRV of 1.63. Including in particular cc-rs. So I think increasing the MSRV to 1.57 without bumping the SemVer minor version is the right thing to do. Note that Rust 1.57 was released Dec 2, 2021, about 2.5 years ago.
Using windows-sys/targets is much better than bumping MSRV to 1.64/1.71, regardless of how the version number of
getrandom
is updated.I don't think we should be bothering anybody with this. I feel like any effort towards that is would be wasting people's effort for no real benefit.
Agreed on all these.
I think that is fine. As I mentioned in the PR, there's no reason AFAICT to prefer windows-targets over windows-sys since they seem to do the version bumping in sync with each other. Using windows-targets directly seems worse.
I agree that they seem to have similar versions/MSRVs, and I initially looked at using windows-sys
. But it seems like:
windows-targets
allows us to have significantly lower compile times.
Win32_Foundation
(needed for ProcessPrng
) causes a huge number of items to be generated.windows-targets
seems to be done by "foundational" "low level" crates which need 1-2 simple Windows bindings.
Note that in those cases it's typical to use windows-bindgen
to generate the bindings. This can be done in a separate crate rather than a build script and the result committed to the repository.
Your comments regarding windows-targets
vs windows-sys
make sense to me. No objections.
Note that in those cases it's typical to use windows-bindgen to generate the bindings. This can be done in a separate crate rather than a build script and the result committed to the repository.
It's notably more work compared to just writing (or copy-pasting) the declarations. Hopefully there is not much that can go wrong in this case.
Fixed by #415
ProcessPRNG
was introduced in Windows 10, which is the minimum supported Windows for most Rust targets. Upstream change: https://github.com/rust-lang/rust/pull/121337BoringSSL now uses
ProcessPRNG
: https://github.com/google/boringssl/blob/2db0eb3f96a5756298dcd7f9319e56a98585bd10/crypto/rand_extra/windows.c#L64 Golang also switched toProcessPRNG
: https://go-review.googlesource.com/c/go/+/536235Should also help with the Chromium Sandbox: https://issues.chromium.org/issues/40277768