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

Suggestion: BufMut::advance_mut should make cnt > self.remaining_mut() unsound #760

Open
paolobarbolini opened this issue Jan 24, 2025 · 4 comments

Comments

@paolobarbolini
Copy link
Contributor

paolobarbolini commented Jan 24, 2025

The current documentation for BufMut::advance_mut suggests that the implementation should handle cases where cnt > self.remaining_mut(). However, since this function is already unsafe and requires the caller to ensure that the declared length has been properly initialized, it seems contradictory to also suggest the implementation handle out-of-bounds lengths.
This requirement was added in #70, and did not exist before. It looks like an artifact of the pre-MaybeUninit days.

The suggestion should instead be to add debug_assert! to catch obviously unsound implementations while avoiding unnecessary runtime overhead in release builds.

@paolobarbolini paolobarbolini changed the title Suggestion: BufMut::advance_mut should not require cnt to be in bounds Suggestion: BufMut::advance_mut should make cnt > self.remaining_mut() unsound Jan 24, 2025
@Darksonn
Copy link
Contributor

I'm not convinced this is possible due to the semver hack. Bytes v1 will re-export bytes_v2::BufMut, so we cannot make any change we want to due to backwards compat.

@Kixunil
Copy link

Kixunil commented Jan 25, 2025

I agree it's impossible but I also agree that an already unsafe function required to handle invalid inputs is unfortunate.

@paolobarbolini
Copy link
Contributor Author

I'm not convinced this is possible due to the semver hack. Bytes v1 will re-export bytes_v2::BufMut, so we cannot make any change we want to due to backwards compat.

Could we change the docs to say something along the lines of:

  • Implementors: keep doing what you're doing
  • Callers: assume it's unsound to have cnt > self.remaining_mut()

@Kixunil
Copy link

Kixunil commented Jan 26, 2025

That would be a nice way to transition into it becoming unsound at some point (with breaking change).

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

No branches or pull requests

3 participants