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

Switch BufMut::bytes_mut from MaybeUninit to &mut UninitSlice, a type ownec by bytes for this purpose #433

Merged
merged 12 commits into from Oct 19, 2020
53 changes: 21 additions & 32 deletions src/buf/buf_mut.rs
@@ -1,12 +1,8 @@
use crate::buf::{limit, Chain, Limit};
use crate::buf::{limit, Chain, Limit, UninitSlice};
#[cfg(feature = "std")]
use crate::buf::{writer, Writer};

use core::{
cmp,
mem::{self, MaybeUninit},
ptr, usize,
};
use core::{cmp, mem, ptr, usize};

use alloc::{boxed::Box, vec::Vec};

Expand Down Expand Up @@ -73,19 +69,14 @@ pub unsafe trait BufMut {
///
/// let mut buf = Vec::with_capacity(16);
///
/// unsafe {
/// // MaybeUninit::as_mut_ptr
/// buf.bytes_mut()[0].as_mut_ptr().write(b'h');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'e');
/// // Write some data
/// buf.bytes_mut()[0..2].copy_from_slice(b"he");
/// unsafe { buf.advance_mut(2) };
///
/// buf.advance_mut(2);
/// // write more bytes
/// buf.bytes_mut()[0..3].copy_from_slice(b"llo");
///
/// buf.bytes_mut()[0].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[2].as_mut_ptr().write(b'o');
///
/// buf.advance_mut(3);
/// }
/// unsafe { buf.advance_mut(3); }
///
/// assert_eq!(5, buf.len());
/// assert_eq!(buf, b"hello");
Expand Down Expand Up @@ -144,14 +135,14 @@ pub unsafe trait BufMut {
///
/// unsafe {
/// // MaybeUninit::as_mut_ptr
/// buf.bytes_mut()[0].as_mut_ptr().write(b'h');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'e');
/// buf.bytes_mut()[0..].as_mut_ptr().write(b'h');
/// buf.bytes_mut()[1..].as_mut_ptr().write(b'e');
///
/// buf.advance_mut(2);
///
/// buf.bytes_mut()[0].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[1].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[2].as_mut_ptr().write(b'o');
/// buf.bytes_mut()[0..].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[1..].as_mut_ptr().write(b'l');
/// buf.bytes_mut()[2..].as_mut_ptr().write(b'o');
///
/// buf.advance_mut(3);
/// }
Expand All @@ -167,7 +158,7 @@ pub unsafe trait BufMut {
/// `bytes_mut` returning an empty slice implies that `remaining_mut` will
/// return 0 and `remaining_mut` returning 0 implies that `bytes_mut` will
/// return an empty slice.
fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>];
fn bytes_mut(&mut self) -> &mut UninitSlice;

/// Transfer bytes into `self` from `src` and advance the cursor by the
/// number of bytes written.
Expand Down Expand Up @@ -922,7 +913,7 @@ macro_rules! deref_forward_bufmut {
(**self).remaining_mut()
}

fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
(**self).bytes_mut()
}

Expand Down Expand Up @@ -1007,9 +998,9 @@ unsafe impl BufMut for &mut [u8] {
}

#[inline]
fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
// MaybeUninit is repr(transparent), so safe to transmute
unsafe { mem::transmute(&mut **self) }
fn bytes_mut(&mut self) -> &mut UninitSlice {
// UninitSlice is repr(transparent), so safe to transmute
unsafe { &mut *(*self as *mut [u8] as *mut _) }
}

#[inline]
Expand Down Expand Up @@ -1042,18 +1033,16 @@ unsafe impl BufMut for Vec<u8> {
}

#[inline]
fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
use core::slice;

fn bytes_mut(&mut self) -> &mut UninitSlice {
if self.capacity() == self.len() {
self.reserve(64); // Grow the vec
}

let cap = self.capacity();
let len = self.len();

let ptr = self.as_mut_ptr() as *mut MaybeUninit<u8>;
unsafe { &mut slice::from_raw_parts_mut(ptr, cap)[len..] }
let ptr = self.as_mut_ptr();
unsafe { &mut UninitSlice::from_raw_parts_mut(ptr, cap)[len..] }
}

// Specialize these methods so they can skip checking `remaining_mut`
Expand Down
6 changes: 2 additions & 4 deletions src/buf/chain.rs
@@ -1,8 +1,6 @@
use crate::buf::IntoIter;
use crate::buf::{IntoIter, UninitSlice};
use crate::{Buf, BufMut};

use core::mem::MaybeUninit;

#[cfg(feature = "std")]
use std::io::IoSlice;

Expand Down Expand Up @@ -183,7 +181,7 @@ where
self.a.remaining_mut() + self.b.remaining_mut()
}

fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
if self.a.has_remaining_mut() {
self.a.bytes_mut()
} else {
Expand Down
5 changes: 3 additions & 2 deletions src/buf/limit.rs
@@ -1,6 +1,7 @@
use crate::buf::UninitSlice;
use crate::BufMut;

use core::{cmp, mem::MaybeUninit};
use core::cmp;

/// A `BufMut` adapter which limits the amount of bytes that can be written
/// to an underlying buffer.
Expand Down Expand Up @@ -60,7 +61,7 @@ unsafe impl<T: BufMut> BufMut for Limit<T> {
cmp::min(self.inner.remaining_mut(), self.limit)
}

fn bytes_mut(&mut self) -> &mut [MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
let bytes = self.inner.bytes_mut();
let end = cmp::min(bytes.len(), self.limit);
&mut bytes[..end]
Expand Down
2 changes: 2 additions & 0 deletions src/buf/mod.rs
Expand Up @@ -24,6 +24,7 @@ mod limit;
#[cfg(feature = "std")]
mod reader;
mod take;
mod uninit_slice;
mod vec_deque;
#[cfg(feature = "std")]
mod writer;
Expand All @@ -34,6 +35,7 @@ pub use self::chain::Chain;
pub use self::iter::IntoIter;
pub use self::limit::Limit;
pub use self::take::Take;
pub use self::uninit_slice::UninitSlice;

#[cfg(feature = "std")]
pub use self::{reader::Reader, writer::Writer};
176 changes: 176 additions & 0 deletions src/buf/uninit_slice.rs
@@ -0,0 +1,176 @@
use core::fmt;
use core::mem::MaybeUninit;
use core::ops::{
Index, IndexMut, Range, RangeFrom, RangeFull, RangeInclusive, RangeTo, RangeToInclusive,
};

/// Uninitialized byte slice.
///
/// Returned by `BufMut::bytes_mut()`, the referenced byte slice may be
/// uninitialized. The wrapper provides safe access without introducing
/// undefined behavior.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to say what the concrete invariant/protocol for this type is, and in particular how it differs from MaybeUninit.

if I understand correctly, the key difference is that the contents may be uninitialized, but the API ensures that once they are initialized, they will never be de-initialized again. Is that correct?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct. Especially it allows constructing UninitSlice from already-initialized slice (&mut [u8]).

///
/// The safety invariants of this wrapper are:
///
/// 1. Reading from an `UninitSlice` is undefined behavior.
/// 2. Writing uninitialized bytes to an `UninitSlice` is undefined behavior.
///
/// The difference between `&mut UninitSlice` and `&mut [MaybeUninit<u8>]` is
/// that it is possible in safe code to write uninitialized bytes to an
/// `&mut [MaybeUninit<u8>]`, which this type prohibits.
#[repr(transparent)]
pub struct UninitSlice([MaybeUninit<u8>]);

impl UninitSlice {
/// Create a `&mut UninitSlice` from a pointer and a length.
///
/// # Safety
///
/// The caller must ensure that `ptr` references a valid memory region owned
/// by the caller representing a byte slice for the duration of `'a`.
///
/// # Examples
///
/// ```
/// use bytes::buf::UninitSlice;
///
/// let bytes = b"hello world".to_vec();
/// let ptr = bytes.as_ptr() as *mut _;
/// let len = bytes.len();
///
/// let slice = unsafe { UninitSlice::from_raw_parts_mut(ptr, len) };
/// ```
pub unsafe fn from_raw_parts_mut<'a>(ptr: *mut u8, len: usize) -> &'a mut UninitSlice {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a constructor that takes an &mut [u8]?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe (also) impl<'a> From<&'a mut [u8]> for &'a mut UninitSlice

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both sounds good to me. I think we can do that in a follow up PR.

let maybe_init: &mut [MaybeUninit<u8>] =
core::slice::from_raw_parts_mut(ptr as *mut _, len);
&mut *(maybe_init as *mut [MaybeUninit<u8>] as *mut UninitSlice)
}

/// Write a single byte at the specified offset.
///
/// # Panics
///
/// The function panics if `index` is out of bounds.
///
/// # Examples
///
/// ```
/// use bytes::buf::UninitSlice;
///
/// let mut data = [b'f', b'o', b'o'];
/// let slice = unsafe { UninitSlice::from_raw_parts_mut(data.as_mut_ptr(), 3) };
///
/// slice.write_byte(0, b'b');
///
/// assert_eq!(b"boo", &data[..]);
/// ```
pub fn write_byte(&mut self, index: usize, byte: u8) {
assert!(index < self.len());

unsafe { self[index..].as_mut_ptr().write(byte) }
}

/// Copies bytes from `src` into `self`.
///
/// The length of `src` must be the same as `self`.
///
/// # Panics
///
/// The function panics if `src` has a different length than `self`.
///
/// # Examples
///
/// ```
/// use bytes::buf::UninitSlice;
///
/// let mut data = [b'f', b'o', b'o'];
/// let slice = unsafe { UninitSlice::from_raw_parts_mut(data.as_mut_ptr(), 3) };
///
/// slice.copy_from_slice(b"bar");
///
/// assert_eq!(b"bar", &data[..]);
/// ```
pub fn copy_from_slice(&mut self, src: &[u8]) {
use core::ptr;

assert_eq!(self.len(), src.len());

unsafe {
ptr::copy_nonoverlapping(src.as_ptr(), self.as_mut_ptr(), self.len());
}
}

/// Return a raw pointer to the slice's buffer.
///
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest documenting safety:

  • The consumer must not read from the memory behind the pointer
  • The consumer must not write uninit bytes into memory behind the pointer

(also aliasing rules, but that should be obvious)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, writing uninit bytes into the memory should be fine if the caller does not advance accordingly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not if UninitSlice was constructed from &mut [u8] - same issue why there was a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right! Way subtle

/// # Safety
///
/// The caller **must not** read from the referenced memory and **must not**
/// write **uninitialized** bytes to the slice either.
///
/// # Examples
///
/// ```
/// use bytes::BufMut;
///
/// let mut data = [0, 1, 2];
/// let mut slice = &mut data[..];
/// let ptr = BufMut::bytes_mut(&mut slice).as_mut_ptr();
/// ```
pub fn as_mut_ptr(&mut self) -> *mut u8 {
self.0.as_mut_ptr() as *mut _
}

/// Returns the number of bytes in the slice.
///
/// # Examples
///
/// ```
/// use bytes::BufMut;
///
/// let mut data = [0, 1, 2];
/// let mut slice = &mut data[..];
/// let len = BufMut::bytes_mut(&mut slice).len();
///
/// assert_eq!(len, 3);
/// ```
pub fn len(&self) -> usize {
self.0.len()
}
}

impl fmt::Debug for UninitSlice {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_struct("UninitSlice[...]").finish()
}
}

macro_rules! impl_index {
($($t:ty),*) => {
$(
impl Index<$t> for UninitSlice {
type Output = UninitSlice;

fn index(&self, index: $t) -> &UninitSlice {
let maybe_uninit: &[MaybeUninit<u8>] = &self.0[index];
unsafe { &*(maybe_uninit as *const [MaybeUninit<u8>] as *const UninitSlice) }
}
}

impl IndexMut<$t> for UninitSlice {
fn index_mut(&mut self, index: $t) -> &mut UninitSlice {
let maybe_uninit: &mut [MaybeUninit<u8>] = &mut self.0[index];
unsafe { &mut *(maybe_uninit as *mut [MaybeUninit<u8>] as *mut UninitSlice) }
}
}
)*
};
}

impl_index!(
Range<usize>,
RangeFrom<usize>,
RangeFull,
RangeInclusive<usize>,
RangeTo<usize>,
RangeToInclusive<usize>
);
12 changes: 6 additions & 6 deletions src/bytes_mut.rs
Expand Up @@ -11,7 +11,7 @@ use alloc::{
vec::Vec,
};

use crate::buf::IntoIter;
use crate::buf::{IntoIter, UninitSlice};
use crate::bytes::Vtable;
#[allow(unused)]
use crate::loom::sync::atomic::AtomicMut;
Expand Down Expand Up @@ -684,7 +684,7 @@ impl BytesMut {
self.reserve(cnt);

unsafe {
let dst = self.maybe_uninit_bytes();
let dst = self.uninit_slice();
// Reserved above
debug_assert!(dst.len() >= cnt);

Expand Down Expand Up @@ -910,12 +910,12 @@ impl BytesMut {
}

#[inline]
fn maybe_uninit_bytes(&mut self) -> &mut [mem::MaybeUninit<u8>] {
fn uninit_slice(&mut self) -> &mut UninitSlice {
unsafe {
let ptr = self.ptr.as_ptr().offset(self.len as isize);
let len = self.cap - self.len;

slice::from_raw_parts_mut(ptr as *mut mem::MaybeUninit<u8>, len)
UninitSlice::from_raw_parts_mut(ptr, len)
}
}
}
Expand Down Expand Up @@ -985,11 +985,11 @@ unsafe impl BufMut for BytesMut {
}

#[inline]
fn bytes_mut(&mut self) -> &mut [mem::MaybeUninit<u8>] {
fn bytes_mut(&mut self) -> &mut UninitSlice {
if self.capacity() == self.len() {
self.reserve(64);
}
self.maybe_uninit_bytes()
self.uninit_slice()
}

// Specialize these methods so they can skip checking `remaining_mut`
Expand Down