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

Tracking Issue for CStr::from_bytes_until_nul #95027

Closed
1 of 3 tasks
ericseppanen opened this issue Mar 16, 2022 · 14 comments
Closed
1 of 3 tasks

Tracking Issue for CStr::from_bytes_until_nul #95027

ericseppanen opened this issue Mar 16, 2022 · 14 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ericseppanen
Copy link
Contributor

ericseppanen commented Mar 16, 2022

Feature gate: #![feature(cstr_from_bytes_until_nul)]

This is a tracking issue for adding member fn to CStr that convert from a byte slice, without the caller needing to know where the nul byte is located within the slice.

This is intended for use in FFI calls, where a foreign function wrote a string into a Rust-allocated buffer. The existing member fns fall short in this case:

  • CStr::from_ptr is unsafe and may read past the end of the buffer if no nul byte is found.
  • CStr::from_bytes_with_nul only works if there is exactly one nul byte at the end of the slice.

The proposed new member fn (tentatively named from_bytes_until_nul) is easier and can be used safely on any byte slice.

Public API

// std::ffi:

pub struct FromBytesUntilNulError(...);

impl CStr {
    pub fn from_bytes_until_nul(bytes: &[u8]) -> Result<&CStr, FromBytesUntilNulError>;
}

Steps / History

Unresolved Questions

@ericseppanen ericseppanen added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 16, 2022
@Stargateur
Copy link
Contributor

Stargateur commented Aug 1, 2022

CStr::from_bytes_with_nul only works if there is exactly one nul byte at the end of the slice.

That a better doc that real doc

ensuring that the byte slice is nul-terminated and does not contain any interior nul bytes.

for exemple, [97, 98, 99, 100, 0, 0, 0, 0] could be interpreted as no interior nul bytes unlike the example b"he\0llo\0".

@jimblandy
Copy link
Contributor

We could have avoided the unsafe block that led to gfx-rs/wgpu#3076 by using this function. I hope it gets stabilized.

@tgross35
Copy link
Contributor

Would it be possible to separately feature gate cstring_from_bytes_until_nul, instead of both CStr and CString being under cstr_from_bytes_until_nul? I ask because this CStr version is straightforward, has been around for almost a year, is very useful for FFI, and is already mentioned in the forums quite often - imho, probably ready for FCP.

However, the CString version hasn't even merged yet. I just don't think it's worth delaying the CStr stabilization for another year or so while waiting on CString::from_vec_until_nul to get merged & mature a while.

@tgross35
Copy link
Contributor

Hey @ericseppanen I hope you don't mind, but I've opened stabilization PR #107429 without any CString implementation. If you have any concerns just let me know and I can close / adjust it as needed

@ericseppanen
Copy link
Contributor Author

I'm not sure which issue to comment on, but I'm happy with whatever others want to do. I was never sure why the CString bit (#96186) stalled, but it's at least partially my own fault as I was too shy to beg for more reviews.

I'll try to find time to rebase that commit and poke the reviewers again.

@tgross35
Copy link
Contributor

Okiedoke, as long as you don't have any qualms about the cstr part going stable. Sorry to leapfrog you and start the stabilization, but I wasn't sure if you were still active on github.

Yeah, things have a tendency to stall a bit when they're close to the merge phase and there isn't really any open debate or strong need to drive discussion. Not your fault!

@m-ou-se m-ou-se changed the title Tracking Issue for CString/CStr::from_bytes_until_nul Tracking Issue for CStr::from_bytes_until_nul Jan 30, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Jan 30, 2023

Would it be possible to separately feature gate cstring_from_bytes_until_nul, instead of both CStr and CString being under cstr_from_bytes_until_nul?

Good idea. I've edited this tracking issue so it only covers CStr::from_bytes_until_nul.

compiler-errors added a commit to compiler-errors/rust that referenced this issue Feb 9, 2023
…bilization, r=dtolnay

Stabilize feature `cstr_from_bytes_until_nul`

This PR seeks to stabilize `cstr_from_bytes_until_nul`.

Partially addresses rust-lang#95027

This function has only been on nightly for about 10 months, but I think it is simple enough that there isn't harm discussing stabilization. It has also had at least a handful of mentions on both the user forum and the discord, so it seems like it's already in use or at least known.

This needs FCP still.

Comment on potential discussion points:
- eventual conversion of `CStr` to be a single thin pointer: this function will still be useful to provide a safe way to create a `CStr` after this change.
- should this return a length too, to address concerns about the `CStr` change? I don't see it as being particularly useful, and it seems less ergonomic (i.e. returning `Result<(&CStr, usize), FromBytesUntilNulError>`). I think users that also need this length without the additional `strlen` call are likely better off using a combination of other methods, but this is up for discussion
- `CString::from_vec_until_nul`: this is also useful, but it doesn't even have a nightly implementation merged yet. I propose feature gating that separately, as opposed to blocking this `CStr` implementation on that

Possible alternatives:

A user can use `from_bytes_with_nul` on a slice up to `my_slice[..my_slice.iter().find(|c| c == 0).unwrap()]`. However; that is significantly less ergonomic, and is a bit more work for the compiler to optimize compared the direct `memchr` call that this wraps.

## New stable API

```rs
// both in core::ffi

pub struct FromBytesUntilNulError(());

impl CStr {
    pub const fn from_bytes_until_nul(
        bytes: &[u8]
    ) -> Result<&CStr, FromBytesUntilNulError>
}
```

cc `@ericseppanen` original author, `@Mark-Simulacrum` original reviewer, `@m-ou-se` brought up some issues on the thin pointer CStr

`@rustbot` modify labels: +T-libs-api +needs-fcp
compiler-errors added a commit to compiler-errors/rust that referenced this issue Feb 9, 2023
…bilization, r=dtolnay

Stabilize feature `cstr_from_bytes_until_nul`

This PR seeks to stabilize `cstr_from_bytes_until_nul`.

Partially addresses rust-lang#95027

This function has only been on nightly for about 10 months, but I think it is simple enough that there isn't harm discussing stabilization. It has also had at least a handful of mentions on both the user forum and the discord, so it seems like it's already in use or at least known.

This needs FCP still.

Comment on potential discussion points:
- eventual conversion of `CStr` to be a single thin pointer: this function will still be useful to provide a safe way to create a `CStr` after this change.
- should this return a length too, to address concerns about the `CStr` change? I don't see it as being particularly useful, and it seems less ergonomic (i.e. returning `Result<(&CStr, usize), FromBytesUntilNulError>`). I think users that also need this length without the additional `strlen` call are likely better off using a combination of other methods, but this is up for discussion
- `CString::from_vec_until_nul`: this is also useful, but it doesn't even have a nightly implementation merged yet. I propose feature gating that separately, as opposed to blocking this `CStr` implementation on that

Possible alternatives:

A user can use `from_bytes_with_nul` on a slice up to `my_slice[..my_slice.iter().find(|c| c == 0).unwrap()]`. However; that is significantly less ergonomic, and is a bit more work for the compiler to optimize compared the direct `memchr` call that this wraps.

## New stable API

```rs
// both in core::ffi

pub struct FromBytesUntilNulError(());

impl CStr {
    pub const fn from_bytes_until_nul(
        bytes: &[u8]
    ) -> Result<&CStr, FromBytesUntilNulError>
}
```

cc ``@ericseppanen`` original author, ``@Mark-Simulacrum`` original reviewer, ``@m-ou-se`` brought up some issues on the thin pointer CStr

``@rustbot`` modify labels: +T-libs-api +needs-fcp
compiler-errors added a commit to compiler-errors/rust that referenced this issue Feb 9, 2023
…bilization, r=dtolnay

Stabilize feature `cstr_from_bytes_until_nul`

This PR seeks to stabilize `cstr_from_bytes_until_nul`.

Partially addresses rust-lang#95027

This function has only been on nightly for about 10 months, but I think it is simple enough that there isn't harm discussing stabilization. It has also had at least a handful of mentions on both the user forum and the discord, so it seems like it's already in use or at least known.

This needs FCP still.

Comment on potential discussion points:
- eventual conversion of `CStr` to be a single thin pointer: this function will still be useful to provide a safe way to create a `CStr` after this change.
- should this return a length too, to address concerns about the `CStr` change? I don't see it as being particularly useful, and it seems less ergonomic (i.e. returning `Result<(&CStr, usize), FromBytesUntilNulError>`). I think users that also need this length without the additional `strlen` call are likely better off using a combination of other methods, but this is up for discussion
- `CString::from_vec_until_nul`: this is also useful, but it doesn't even have a nightly implementation merged yet. I propose feature gating that separately, as opposed to blocking this `CStr` implementation on that

Possible alternatives:

A user can use `from_bytes_with_nul` on a slice up to `my_slice[..my_slice.iter().find(|c| c == 0).unwrap()]`. However; that is significantly less ergonomic, and is a bit more work for the compiler to optimize compared the direct `memchr` call that this wraps.

## New stable API

```rs
// both in core::ffi

pub struct FromBytesUntilNulError(());

impl CStr {
    pub const fn from_bytes_until_nul(
        bytes: &[u8]
    ) -> Result<&CStr, FromBytesUntilNulError>
}
```

cc ```@ericseppanen``` original author, ```@Mark-Simulacrum``` original reviewer, ```@m-ou-se``` brought up some issues on the thin pointer CStr

```@rustbot``` modify labels: +T-libs-api +needs-fcp
@tgross35
Copy link
Contributor

tgross35 commented Jul 1, 2023

Since cstr_from_bytes_until_nul has been stabilized (#107429 1.69), I think this issue could be closed. @ericseppanen do you perhaps want to start a new tracking issue for cstring_from_bytes_until_nul?

@ericseppanen
Copy link
Contributor Author

Sure, we can close this. I'm not sure if a new tracking issue is needed, since the CString::from_vec_until_nul PR (#96186) is not making progress. If someone wants to bring it back to life, feel free to persuade the reviewers.

@tgross35
Copy link
Contributor

tgross35 commented Jul 1, 2023

Fwiw that PR is labeled waiting on author, you can comment @rustbot ready to change it to waiting on review

@aswild
Copy link
Contributor

aswild commented Jul 13, 2023

Hi @ericseppanen,
Was it intentional to add FromBytesUntilNulError only in core::ffi and not re-export it in std::ffi?

I'm used to everything from core (and alloc) to be re-exported under the same name in std, so it was a bit of a surprise to see an error message when trying to use std::ffi::{CStr, FromBytesUntilNulError}.

@tgross35
Copy link
Contributor

tgross35 commented Jul 13, 2023

@aswild I don't think anybody would object to a reexport - are you interested in making a PR for that change?

It would need to go through FCP (as it changes public API) but I think it's small enough that that could happen on the PR.

@ericseppanen
Copy link
Contributor Author

Hi @ericseppanen, Was it intentional to add FromBytesUntilNulError only in core::ffi and not re-export it in std::ffi?

Not intentional; probably just me not knowing I was supposed to do it.

@aswild
Copy link
Contributor

aswild commented Jul 14, 2023

Sure, I can give making a PR a try! I haven't contributed to the standard library before so I apologize in advance if I get the #[stable] attribute wrong at first.

aswild added a commit to aswild/rust that referenced this issue Jul 14, 2023
Like the other CStr and CString error types, make a re-export for
std::ffi::FromBytesUntilNulError.

This seems to have slipped through the cracks in the
cstr_from_bytes_until_nul implementation and core_c_str migration.

Tracking Issue: rust-lang#95027
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 31, 2023
…r=dtolnay

Re-export core::ffi::FromBytesUntilNulError in std::ffi

Like the other CStr and CString error types, make a re-export for std::ffi::FromBytesUntilNulError.

This seems to have slipped through the cracks in the cstr_from_bytes_until_nul implementation and core_c_str migration.

Tracking Issue: rust-lang#95027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants