search

rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
275 stars 180 forks source link

Increase MSRV to 1.57 & completely check null termination of C strings at compile time. #426

Closed briansmith closed 4 months ago

newpavlov commented 4 months ago

I don't think this is a good approach. It adds a whole bunch of code to just... introduce a CStr ersatz? We just should use &'static CStr. We can construct it using from_bytes_with_nul which is const since 1.72. Since we are likely to bump MSRV in v0.3 to 1.71, bumping it further to 1.72 is not a big deal. Alternatively, we could use unsafe from_bytes_with_nul_unchecked, which is const since 1.59. Ideally, we would use CStr literals, but they were stabilized only in Rust 1.77.

Also, in the latest version we use Weak only in the NetBSD backend. It may be worth to move it either to the NetBSD module, or to a separate module mounted only by the NetBSD code.

newpavlov commented 4 months ago

Also, could you please untie this PR from your commits in #425? It makes harder to review it.

briansmith commented 4 months ago

We just should use &'static CStr. We can construct it using from_bytes_with_nul which is const since 1.72. Since we are likely to bump MSRV in v0.3 to 1.71, bumping it further to 1.72 is not a big deal.

I doubt we will be able to increase MSRV beyond 1.63 any time soon because 1.63 is what Debian 8 uses, and many crates are trying to stay compatible with it through mid-2025 or so.

Alternatively, we could use unsafe from_bytes_with_nul_unchecked, which is const since 1.59.

CStr isn't in libcore until Rust 1.64, so we can't use it in a no_std crate until then.

briansmith commented 4 months ago

Branch briansmith:b/1.64-2 contains the MSRV 1.64 code that replaces cstr::Ref with &'static core::ffi::CStr.

briansmith commented 4 months ago

Also, could you please untie this PR from your commits in #425? It makes harder to review it.

It would be extra work. I would rather work together on #425 first. Note that this is just a draft PR to help understand PR #425.

You can see the difference between the two branches using the GitHub comparison tool, which is "live": https://github.com/briansmith/getrandom/compare/b/cast-1...briansmith:getrandom:b/cstr?expand=1

newpavlov commented 4 months ago

I don't think we need this anymore after #427 merge.