Skip to content

Minor incompatibility with bytes #1

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

Closed
AdamGS opened this issue Mar 17, 2025 · 2 comments
Closed

Minor incompatibility with bytes #1

AdamGS opened this issue Mar 17, 2025 · 2 comments

Comments

@AdamGS
Copy link

AdamGS commented Mar 17, 2025

First of all, this is an awesome crate! really cool to see people improving on some of the most popular crates in the ecosystem.
I tried arc-slice in Vortex, where we make very heavy use of Bytes. While it does seem to make a difference in some benchmarks, it also surfaced an issue where spare_capacity_mut is market unsafe here but isn't in bytes. I'm not if compatibility is a major goal here but I figured its worth reporting.

@wyfo
Copy link
Owner

wyfo commented Mar 18, 2025

You're right, spare_capacity_mut is unsafe in the port of bytes using arc-slice, and it was in fact done on purpose, as I suspect BytesMut::spare_capacity_mut to be unsound, and not having it unsafe for ArcSliceMut is definitely unsound.
Why is it unsound? Because spare_capacity_mut, combined with set_len, allows to write uninitialized memory to the buffer.
I'm currently asking if it is an issue for a Vec buffer (that BytesMut is using), but that is clearly an issue for arbitrary buffers, which ArcSliceMut supports. I may also open an issue on bytes repository, depending on the answer I get.

That being said, while in previous drafts I was also storing the Vec buffer directly in the Arc, I made a recent change to store only the pointer + capacity in case of Vec (if needs_drop::<T>() returns false). So technically, I'm protected about the unsoundness of writing uninitialized memory in my BytesMut reimplementation, and I can make BytesMut::spare_capacity_mut safe. I will try to push a new release tonight with this fix, making the API 100% compatible.

In any case, thank you a lot for your feedbacks, and for having tried arc-slice.

@wyfo
Copy link
Owner

wyfo commented Mar 18, 2025

0.1.0-alpha.2 has been published.

@wyfo wyfo closed this as completed Mar 18, 2025
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

2 participants