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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 9 commits into from
Closed

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

wants to merge 9 commits into from

Conversation

briansmith
Copy link
Contributor

No description provided.

@briansmith
Increase MSRV to 1.38 so we can use safer pointer casts.
07d6edf
@briansmith
Use p.cast::<T>() instead of as as *mut T.
398b5d7
@briansmith
Clarify hazards in 'as *const _ casts of *const u8 to *const libc…
d77e000 
…::c_char`.
@briansmith
Minimize safety hazards in slice_as_uninit[_mut].
b5c89a6 
In each function, explicitly use the implicit conversion of
`&[T]` to `*const T`/`&mut [T]` to `* mut T` to eliminate one
`as *const T`/`as *mut T` respectively.

Instead of relying on the remaining cast in each function being
valid for all `T`, reduce the scope of the assumption to `u8`, and
add a TODO avoid eliminating the use of the assumption.
@briansmith
Increase MSRV to 1.42 so we can use slice patterns.
6873944
@briansmith briansmith changed the title Completely check null termination of C strings to compile time. Increase MSRV to 1.42 & completely check null termination of C strings to compile time. May 20, 2024
@briansmith briansmith changed the title Increase MSRV to 1.42 & completely check null termination of C strings to compile time. Increase MSRV to 1.42 & completely check null termination of C strings at compile time. May 20, 2024
@newpavlov
Copy link
Member

newpavlov commented May 20, 2024 •
edited
Loading

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.

@briansmith
Bump MSRV to 1.57 so we can panic! in const fn.
fd15a60
@briansmith
Improve the memory safety guarantees around open_readonly.
07071c7 
Do more complete NUL termination checking, at compile-time. Remove the
`unsafe` from the function as it is now memory-safe.
@briansmith briansmith force-pushed the b/cstr branch 2 times, most recently from bf7c4b1 to f557cbf Compare May 20, 2024 22:42
@briansmith briansmith changed the title Increase MSRV to 1.42 & completely check null termination of C strings at compile time. Increase MSRV to 1.57 & completely check null termination of C strings at compile time. May 20, 2024
@newpavlov
Copy link
Member

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

@briansmith
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@briansmith briansmith mentioned this pull request May 20, 2024
Merged
@newpavlov newpavlov mentioned this pull request May 20, 2024
Merged
@briansmith
Copy link
Contributor Author

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

@briansmith
Improve memory safety of Weak::new().
7119c43 
Do null terminator checking more completely and at compile time.
@briansmith briansmith mentioned this pull request May 21, 2024
Closed
@newpavlov
Copy link
Member

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

@newpavlov newpavlov closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@briansmith @newpavlov