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

CStr::from_bytes_with_nul returns non-actionable error result #493

Closed
nyurik opened this issue Nov 21, 2024 · 8 comments
Closed

CStr::from_bytes_with_nul returns non-actionable error result #493

nyurik opened this issue Nov 21, 2024 · 8 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@nyurik
Copy link

nyurik commented Nov 21, 2024

Proposal

Problem statement

One of CStr constructors, CStr::from_bytes_with_nul(bytes: &[u8]) handles 3 cases:

  1. bytes has one NULL as the last value - creates CStr
  2. bytes has no NULL - error
  3. bytes has a NULL in some other position - error

The 3rd case is error that may require lossy conversion, but the 2nd case can easily be handled by the user code. Unfortunately, this function returns an opaque FromBytesWithNulError error in both 2nd and 3rd case, so the user cannot detect just the 2nd case - having to re-implement the entire function and bring in the memchr dependency.

Motivating examples or use cases

In this code, my FFI code needs to copy user's &[u8] into a C-allocated memory blob in a NUL-terminated CStr format. My code must first validate if &[u8] has a trailing NUL (case 1), no NUL (adds one on the fly - case 2), or NUL in the middle (3rd case - error). I had to re-implement from_bytes_with_nul and add memchrdependency just to handle the 2nd case.

Solution sketch

It may make sense to do one of these:

  • stabilize the kind of the error this function returns - so it can be examined
  • introduce a new function that returns validation result for all 3 cases
  • something else?

CStr::validate_bytes(value: &[u8]) -> CStrValidation -- returns an enum with valid, NotNulTerminated, InteriorNul

@nyurik nyurik added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Nov 21, 2024
@nyurik nyurik changed the title from_bytes_with_nul returns non-actionable error result CStr::from_bytes_with_nul returns non-actionable error result Nov 21, 2024
@kennytm
Copy link
Member

kennytm commented Nov 21, 2024

Exposing the kind() should be the most natural, as that kind private field is already there.

@Amanieu
Copy link
Member

Amanieu commented Dec 10, 2024

We discussed this in the libs-api meeting and we are in favor of adding this. However since there are no other possible ways for from_bytes_with_nul to fail, the error type itself should be turned into an enum rather than having a separate kind method.

@Amanieu Amanieu closed this as completed Dec 10, 2024
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Dec 10, 2024
@nyurik
Copy link
Author

nyurik commented Dec 10, 2024

Thx @Amanieu , are you saying that the enum FromBytesWithNulErrorKind should be renamed into enum FromBytesWithNulError , and the struct FromBytesWithNulError deleted?

@Amanieu
Copy link
Member

Amanieu commented Dec 10, 2024

Yes, essentially.

@nyurik
Copy link
Author

nyurik commented Dec 10, 2024

wouldn't this be a breaking change?

@cuviper
Copy link
Member

cuviper commented Dec 10, 2024

It's mostly opaque, but one way it breaks is that pattern FromBytesWithNulError { .. } works now with a struct, and won't work with an enum.

@nyurik
Copy link
Author

nyurik commented Dec 10, 2024

so i guess its no biggie. I can submit a PR in a sec

@nyurik
Copy link
Author

nyurik commented Dec 10, 2024

PR here rust-lang/rust#134143

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 5, 2025
Convert `struct FromBytesWithNulError` into enum

This PR renames the former `kind` enum from `FromBytesWithNulErrorKind` to `FromBytesWithNulError`, and removes the original struct.

See rust-lang/libs-team#493

## Possible Changes - TBD
* [x] should the new `enum FromBytesWithNulError` derive `Copy`?
* [ ] should there be any new/changed attributes?
* [x] add some more tests

## Problem

One of `CStr` constructors, `CStr::from_bytes_with_nul(bytes: &[u8])` handles 3 cases:
1. `bytes` has one NULL as the last value - creates CStr
2. `bytes` has no NULL - error
3. `bytes` has a NULL in some other position - error

The 3rd case is error that may require lossy conversion, but the 2nd case can easily be handled by the user code. Unfortunately, this function returns an opaque `FromBytesWithNulError` error in both 2nd and 3rd case, so the user cannot detect just the 2nd case - having to re-implement the entire function and bring in the `memchr` dependency.

## Motivating examples or use cases

In [this code](https://github.com/gquintard/varnish-rs/blob/f86d7a87683b08d2e634d63e77d9dc1d24ed4a13/varnish-sys/src/vcl/ws.rs#L158), my FFI code needs to copy user's `&[u8]` into a C-allocated memory blob in a NUL-terminated `CStr` format.  My code must first validate if `&[u8]` has a trailing NUL (case 1), no NUL (adds one on the fly - case 2), or NUL in the middle (3rd case - error). I had to re-implement `from_bytes_with_nul` and add `memchr`dependency just to handle the 2nd case.

r? `@Amanieu`
jhpratt added a commit to jhpratt/rust that referenced this issue Jan 15, 2025
Convert `struct FromBytesWithNulError` into enum

This PR renames the former `kind` enum from `FromBytesWithNulErrorKind` to `FromBytesWithNulError`, and removes the original struct.

See rust-lang/libs-team#493

## Possible Changes - TBD
* [x] should the new `enum FromBytesWithNulError` derive `Copy`?
* [ ] should there be any new/changed attributes?
* [x] add some more tests

## Problem

One of `CStr` constructors, `CStr::from_bytes_with_nul(bytes: &[u8])` handles 3 cases:
1. `bytes` has one NULL as the last value - creates CStr
2. `bytes` has no NULL - error
3. `bytes` has a NULL in some other position - error

The 3rd case is error that may require lossy conversion, but the 2nd case can easily be handled by the user code. Unfortunately, this function returns an opaque `FromBytesWithNulError` error in both 2nd and 3rd case, so the user cannot detect just the 2nd case - having to re-implement the entire function and bring in the `memchr` dependency.

## Motivating examples or use cases

In [this code](https://github.com/gquintard/varnish-rs/blob/f86d7a87683b08d2e634d63e77d9dc1d24ed4a13/varnish-sys/src/vcl/ws.rs#L158), my FFI code needs to copy user's `&[u8]` into a C-allocated memory blob in a NUL-terminated `CStr` format.  My code must first validate if `&[u8]` has a trailing NUL (case 1), no NUL (adds one on the fly - case 2), or NUL in the middle (3rd case - error). I had to re-implement `from_bytes_with_nul` and add `memchr`dependency just to handle the 2nd case.

r? `@Amanieu`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 15, 2025
Rollup merge of rust-lang#134143 - nyurik:err-nul, r=dtolnay

Convert `struct FromBytesWithNulError` into enum

This PR renames the former `kind` enum from `FromBytesWithNulErrorKind` to `FromBytesWithNulError`, and removes the original struct.

See rust-lang/libs-team#493

## Possible Changes - TBD
* [x] should the new `enum FromBytesWithNulError` derive `Copy`?
* [ ] should there be any new/changed attributes?
* [x] add some more tests

## Problem

One of `CStr` constructors, `CStr::from_bytes_with_nul(bytes: &[u8])` handles 3 cases:
1. `bytes` has one NULL as the last value - creates CStr
2. `bytes` has no NULL - error
3. `bytes` has a NULL in some other position - error

The 3rd case is error that may require lossy conversion, but the 2nd case can easily be handled by the user code. Unfortunately, this function returns an opaque `FromBytesWithNulError` error in both 2nd and 3rd case, so the user cannot detect just the 2nd case - having to re-implement the entire function and bring in the `memchr` dependency.

## Motivating examples or use cases

In [this code](https://github.com/gquintard/varnish-rs/blob/f86d7a87683b08d2e634d63e77d9dc1d24ed4a13/varnish-sys/src/vcl/ws.rs#L158), my FFI code needs to copy user's `&[u8]` into a C-allocated memory blob in a NUL-terminated `CStr` format.  My code must first validate if `&[u8]` has a trailing NUL (case 1), no NUL (adds one on the fly - case 2), or NUL in the middle (3rd case - error). I had to re-implement `from_bytes_with_nul` and add `memchr`dependency just to handle the 2nd case.

r? `@Amanieu`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants