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

Switch to safe CStr::from_bytes_until_nul on sized c_char array wrapper #746

Merged
merged 1 commit into from
Dec 2, 2023

Conversation

MarijnS95
Copy link
Collaborator

@MarijnS95 MarijnS95 commented May 1, 2023

Depends on #831

Certain structs contain sized character arrays that are converted to CStr for convenient accss to the user and our Debug implementation using unsafe CStr::from_ptr(...as_ptr()). There is no need to round-trip to a pointer and possibly read out of bounds if the NUL-terminator index (string length) is instead searched for by the newly stabilized CStr::from_bytes_until_nul() fn since Rust 1.69 (which panics if no NUL-terminator is found before the end of the slice).

Unfortunately unsafe is still needed to cast the array from a c_char (i8 on most platforms) to u8, which is what from_bytes_until_nul() accepts.


Draft because this is a rather eager MSRV bump that we should probably give more time to be adopted by the community, and because the unsafe slice casts don't look all that pretty.

@Ralith
Copy link
Collaborator

Ralith commented May 1, 2023

I think this would look a lot better wrapped up in a helper function.

@MarijnS95
Copy link
Collaborator Author

@Ralith indeed, because of the unfortunate slice cast. I wish there were from_chararray_until_nul or similar functions, taking a c_char. The cast from u8 to c_char is anyway safe according to CStr::from_ptr().

@MarijnS95 MarijnS95 force-pushed the cstr-from-bytes-until-nul branch 2 times, most recently from 3b120f0 to bda23f5 Compare May 3, 2023 19:35
ash/src/vk/prelude.rs Outdated Show resolved Hide resolved
ash/src/vk/prelude.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the cstr-from-bytes-until-nul branch 2 times, most recently from 9229dc7 to 7489a7e Compare October 11, 2023 10:37
ash/src/vk/prelude.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Collaborator Author

@Ralith taking this PR one step further, what is your thought on extending these structs with a fn <field_name>_as_cstr() for convenience to users?

That would imply the same MSRV bump unless we decide to implement it with the current from_ptr(...as_ptr()) code already used in the Debug formatters.

@Ralith
Copy link
Collaborator

Ralith commented Nov 25, 2023

That sounds nice to me! Any trivial operation that ~every user will have to do to make use of a field is a good candidate for wrapping.

@MarijnS95 MarijnS95 changed the title Switch to CStr::from_bytes_until_nul on sized char arrays in structs Switch to safe CStr::from_bytes_until_nul on sized c_char wrapper Nov 25, 2023
@MarijnS95 MarijnS95 changed the title Switch to safe CStr::from_bytes_until_nul on sized c_char wrapper Switch to safe CStr::from_bytes_until_nul on sized c_char array wrapper Nov 25, 2023
@MarijnS95
Copy link
Collaborator Author

Done in #831 without breaking MSRV, and rebased this PR to turn the unsafe function into a Result with appropriate error type.

(We could also make the current function be safe and return a Result by reimplementing that function, but I'd rather bump MSRV at that point...)

@MarijnS95
Copy link
Collaborator Author

That sounds nice to me! Any trivial operation that ~every user will have to do to make use of a field is a good candidate for wrapping.

On this note, do we also have cases where we might want to wrap a pointer+count in a slice?

In most cases it's probably not useful as the user already has access to their slice:

let mut my_data = Vec![...];
let mut vk_struct = vk::Something::builder().data(&mut my_data);
device.get_something(&mut vk_struct);
dbg!(my_data);

But it might be worth it for writing layers in Rust with ash (though not something that we're close to supporting yet). And it could also be used to work around #815 (comment) (though I'd still rather fix the builder fn).

@MarijnS95
Copy link
Collaborator Author

@Ralith if you're okay with this, Rust 1.69 is now 7 months old and helps us fix the existing soundness issues discussed in #831 (comment) in a trivial way. I'd say we merge this and bump the MSRV, and also unblock the no_std PR.

@MarijnS95 MarijnS95 marked this pull request as ready for review December 2, 2023 09:40
…rapper

Certain structs contain sized character arrays that are converted to
`CStr` for convenient accss to the user and our `Debug` implementation
using unsafe `CStr::from_ptr(...as_ptr())`.  There is no need to
round-trip to a pointer and possibly read out of bounds if the
NUL-terminator index (string length) is instead searched for by the
newly stabilized `CStr::from_bytes_until_nul()` fn since Rust 1.69
(which panics if no NUL-terminator is found before the end of the
slice).

Unfortunately `unsafe` is still needed to cast the array from a `c_char`
(`i8` on most platforms) to `u8`, which is what `from_bytes_until_nul()`
accepts.
@Ralith
Copy link
Collaborator

Ralith commented Dec 2, 2023

SGTM. I don't think there's much benefit to a conservative MSRV here, especially if you don't want one.

@MarijnS95 MarijnS95 merged commit befb8cd into master Dec 2, 2023
20 checks passed
@MarijnS95 MarijnS95 deleted the cstr-from-bytes-until-nul branch December 2, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants