search

tokio-rs / bytes

Utilities for working with bytes
MIT License
1.87k stars 278 forks source link

impl From for `&mut UninitSlice` to `&mut [MaybeUninit<u8>]` #548

Closed erickt closed 2 years ago

erickt commented 2 years ago

This adds an unsafe method to convert a &mut UninitSlice into a &mut [MaybeUninit<u8>]. This method is unsafe because some of the bytes in the slice may be initialized, and the caller should not overwrite them with uninitialized bytes.

This came about when auditing tokio-util's udp frame, where they want to pass the unitialized portion of a BytesMut to ReadBuf::uninit. They need to do this unsafe pointer casting in a few places, which complicates audits. This method lets us document the safety invariants the caller needs to maintain when doing this conversion.

Darksonn commented 2 years ago

Sorry, but this is not sound. You can reborrow a &mut UninitSlice to get a "subslice" to the same range. The subslice can then be converted to a &mut [MaybeUninit<u8>], but the original slice will still be usable once you stop using the subslice.

Darksonn commented 2 years ago

More generally, you need to be able to pass a &mut UninitSlice to untrusted code and trust that it wont deinitialize it. Even without reborrowing, this would break that.

erickt commented 2 years ago

Sorry, but this is not sound. You can reborrow a &mut UninitSlice to get a "subslice" to the same range. The subslice can then be converted to a &mut [MaybeUninit], but the original slice will still be usable once you stop using the subslice.

More generally, you need to be able to pass a &mut UninitSlice to untrusted code and trust that it wont deinitialize it. Even without reborrowing, this would break that.

Ah good point, I didn't consider reborrowing, yeah that would let us deinitialize UninitSlice. However, could you go into more detail about how this is unsound? I don't believe reborrowing would let us get overlapping &mut sections.

Would it be worthwhile to instead add a separate BytesMut::chunk_mut_slice(&mut self) -> &mut [MaybeUninit<u8>] (or something), since a lot of APIs seem to be using this type for working with uninitialized slices? Or would you rather hold off on that now, wait for something like https://doc.rust-lang.org/nightly/std/io/struct.ReadBuf.html to stabilize before considering new APIs?

Darksonn commented 2 years ago

I'm fine with adding an unsafe method for converting to &mut [MaybeUninit<u8>] if it's something we do often.

As for reborrowing, it really does give overlapping &mut sections. The reason it isn't UB is that the compiler will prevent you from using the original slice until after the last use of the reborrow. Since only one of them is usable at the time, the exclusivity is still upheld.

More fundamentally, the reason its unsound to do this is that the code that created the &mut UninitSlice knows that part of the slice is already initialized, and for safety reasons, it needs that part of the slice to remain initialized after giving out an &mut UninitSlice to the memory. Any way of deinitializing it given the &mut UninitSlice (or any reference or pointer derived from it) would cause issues.

Note that Tokio already has a stable variant of ReadBuf.

erickt commented 2 years ago

I'm fine with adding an unsafe method for converting to &mut [MaybeUninit] if it's something we do often. ... More fundamentally, the reason its unsound to do this is that the code that created the &mut UninitSlice knows that part of the slice is already initialized, and for safety reasons, it needs that part of the slice to remain initialized after giving out an &mut UninitSlice to the memory. Any way of deinitializing it given the &mut UninitSlice (or any reference or pointer derived from it) would cause issues.

Ah, that's great to know. It'd be great to document this, since it'd be important to check for this invariant during reviews. Would you be okay with the name UninitSlice::as_uninit_slice_mut? It looks like the standard library may stabilize on that naming scheme: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_uninit_slice_mut

Note that Tokio already has a stable variant of ReadBuf.

Oh sure, I just figured that when we stabilize std::io::ReadBuf, we might want to directly integrate support into the bytes crate. But we probably don't want to make the bytes crate depend on tokio for this API. We could lift tokio's ReadBuf into it's own crate, but I imagine that'd take some time to detangle it from everything else in tokio, so it might not be worth it.

erickt commented 2 years ago

Thanks for the review! I pushed up a new version that hopefully fixes your concerns.