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

Bump MSRV to 1.57 & improve the memory safety guarantees around open_readonly. #432

Closed
wants to merge 2 commits into from

Conversation

briansmith
Copy link
Contributor

Do more complete NUL termination checking, at compile-time. Remove the unsafe from the function as it is now memory-safe.

Rust 1.42 introduced extended slice patterns.
Rust 1.46 introduced `if` in `const fn`.
Rust 1.57 introduced `panic!` in `const fn`.
Do more complete NUL termination checking, at compile-time. Remove the
`unsafe` from the function as it is now memory-safe.
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote in your other PR, I don't think it's worth to introduce the Ref type.

A better solution would be to use zero-terminated byte slices (i.e. const FILE_PATH: &[u8] = b"/dev/urandom\0";) for paths and add the following asserts to open_readonly:

assert_eq!(path[path.len() - 1], 0);
assert!(path[..path.len() - 1].iter().all(|&b| b != 0));

Since we use static paths, compiler should be able to optimize them out.

@josephlr
Copy link
Member

As I wrote in your other PR, I don't think it's worth to introduce the Ref type.

A better solution would be to use zero-terminated byte slices (i.e. const FILE_PATH: &[u8] = b"/dev/urandom\0";) for paths and add the following asserts to open_readonly:

I agree. This is a lot of complexity to just ensure that a byte slice passed to a function contains a zero byte (which is all we need for safety).

assert_eq!(path[path.len() - 1], 0);
assert!(path[..path.len() - 1].iter().all(|&b| b != 0));

Since we use static paths, compiler should be able to optimize them out.

I think the assert just needs to be:

assert!(path.iter().any(|&b| b == 0));

for the function to be safe. If our slice has bytes after the null-terminator, that's weird but not unsafe. This implementation has the advantage of optimizing out at even -C opt-level=1: https://rust.godbolt.org/z/h9EaPnaaY

@newpavlov
Copy link
Member

Closing in favor of #434.

@newpavlov newpavlov closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants