Docs: Buf::chunk should have more restrictions.

[vvvviiv]

This issue could also be a sub-issue of #178.

According to the docs of Buf::chunk:

bytes/src/buf/buf_impl.rs

Lines 142 to 150 in 8cc9407

	/// # Implementer notes
	///
	/// This function should never panic. Once the end of the buffer is reached,
	/// i.e., `Buf::remaining` returns 0, calls to `chunk()` should return an
	/// empty slice.
	// The `chunk` method was previously called `bytes`. This alias makes the rename
	// more easily discoverable.
	#[cfg_attr(docsrs, doc(alias = "bytes"))]
	fn chunk(&self) -> &[u8];

It only requires the implementer to return an empty slice when Buf::remaining returns 0, but a non-continuous implementation of Buf::chunk can return an empty slice even if Buf::remaining > 0.

For example:

pub struct BufVecDeque<B> {
    inner: VecDeque<B>
}

impl<B> Buf for BufVecDeque<B> where B: Buf {
    fn remaining(&self) -> usize {
        self.inner.iter().map(|b| b.remaining()).sum()
    }

    fn chunk(&self) -> &[u8] {
        self.inner.front().map_or(b"", |b| b.chunk())
    }

    fn advance(&mut self, mut cnt: usize) {
        while cnt > 0 {
            let Some(front) = self.inner.front_mut() else {
                return;
            };
            let min = std::cmp::min(front.remaining(), cnt);

            front.advance(min);
            cnt -= min;

            if !front.has_remaining() {
                self.inner.pop_front();
            }
        }
    }
}

impl<B> BufVecDeque<B> {
    pub fn new() -> Self {
        Self {
            inner: VecDeque::new(),
        }
    }

    pub fn push_back(&mut self, buf: B) {
        self.inner.push_back(buf)
    }
}

The problem is when you call some methods like Buf::copy_to_slice or Buf::copy_to_bytes, it will cause an infinite loop.

Let's see the default implementations of Buf::copy_to_slice:

bytes/src/buf/buf_impl.rs

Lines 268 to 282 in 8cc9407

	fn copy_to_slice(&mut self, mut dst: &mut [u8]) {
	if self.remaining() < dst.len() {
	panic_advance(dst.len(), self.remaining());
	}
	while !dst.is_empty() {
	let src = self.chunk();
	let cnt = usize::min(src.len(), dst.len());
	dst[..cnt].copy_from_slice(&src[..cnt]);
	dst = &mut dst[cnt..];
	self.advance(cnt);
	}
	}

The let src = self.chunk(); causes the problem, and its docs does not remind the implementer to override its behavior:

bytes/src/buf/buf_impl.rs

Lines 247 to 268 in 8cc9407

	/// Copies bytes from `self` into `dst`.
	///
	/// The cursor is advanced by the number of bytes copied. `self` must have
	/// enough remaining bytes to fill `dst`.
	///
	/// # Examples
	///
	/// ```
	/// use bytes::Buf;
	///
	/// let mut buf = &b"hello world"[..];
	/// let mut dst = [0; 5];
	///
	/// buf.copy_to_slice(&mut dst);
	/// assert_eq!(&b"hello"[..], &dst);
	/// assert_eq!(6, buf.remaining());
	/// ```
	///
	/// # Panics
	///
	/// This function panics if `self.remaining() < dst.len()`.
	fn copy_to_slice(&mut self, mut dst: &mut [u8]) {

The Buf::copy_to_bytes has the same problem.

As for some methods like Buf::get_u8, they panic:

bytes/src/buf/buf_impl.rs

Lines 300 to 307 in 8cc9407

	fn get_u8(&mut self) -> u8 {
	if self.remaining() < 1 {
	panic_advance(1, 0);
	}
	let ret = self.chunk()[0];
	self.advance(1);
	ret
	}

let ret = self.chunk()[0]; cause panic, and its docs only says: "This function panics if there is no more remaining data in self.":

bytes/src/buf/buf_impl.rs

Lines 284 to 300 in 8cc9407

	/// Gets an unsigned 8 bit integer from `self`.
	///
	/// The current position is advanced by 1.
	///
	/// # Examples
	///
	/// ```
	/// use bytes::Buf;
	///
	/// let mut buf = &b"\x08 hello"[..];
	/// assert_eq!(8, buf.get_u8());
	/// ```
	///
	/// # Panics
	///
	/// This function panics if there is no more remaining data in `self`.
	fn get_u8(&mut self) -> u8 {

But it also panics when Buf::remaining > 0.

In fact, the problem is already mentioned in #178 (comment).

These are all because the Buf::chunk can return an empty slice even if Buf::remaining > 0.

Solutions

A quick fix is restricting the Buf::chunk to return an empty slice if and only if Buf::remaining returns 0, but it also changes the semantics of Buf::chunk, should it be considered a breaking change? I can open a PR if the change is acceptable.

Any more ideas?

[Darksonn]

I think we already de-facto have this requirement, if it's not already specified.

[vvvviiv]

Oh, I just found the BufMut::chunk_mut already had the restrictions.

bytes/src/buf/buf_mut.rs

Lines 166 to 179 in 8cc9407

	/// # Implementer notes
	///
	/// This function should never panic. `chunk_mut` should return an empty
	/// slice **if and only if** `remaining_mut()` returns 0. In other words,
	/// `chunk_mut()` returning an empty slice implies that `remaining_mut()` will
	/// return 0 and `remaining_mut()` returning 0 implies that `chunk_mut()` will
	/// return an empty slice.
	///
	/// This function may trigger an out-of-memory abort if it tries to allocate
	/// memory and fails to do so.
	// The `chunk_mut` method was previously called `bytes_mut`. This alias makes the
	// rename more easily discoverable.
	#[cfg_attr(docsrs, doc(alias = "bytes_mut"))]
	fn chunk_mut(&mut self) -> &mut UninitSlice;

I will open a PR to fix this issue.