bytes icon indicating copy to clipboard operation
bytes copied to clipboard

Docs: `Buf::chunk` should have more restrictions.

Open vvvviiv opened this issue 1 month ago • 2 comments

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

According to the docs of Buf::chunk:

https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L142-L150

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:

https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L268-L282

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

https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L247-L268

The Buf::copy_to_bytes has the same problem.

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

https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L300-L307

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

https://github.com/tokio-rs/bytes/blob/8cc940779fd6a489a2d7ca53fbbc44f84210083e/src/buf/buf_impl.rs#L284-L300

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

In fact, the problem is already mentioned in https://github.com/tokio-rs/bytes/issues/178#issuecomment-458213222.

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?

vvvviiv avatar Jul 02 '24 16:07 vvvviiv