tokio-rs / bytes

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

Buf::chunks_vectored() is wrong if chunk() isn't the whole buf #701

Open seanmonstar opened 4 months ago

seanmonstar commented 4 months ago

The design of Buf::chunks_vectored() does not account for an implementation that returns a partial slice in chunk() and doesn't override chunks_vectored() to be the full thing. It's assumed that if you have a composite buf, such as Chain, that you can just fill the IoSlices with both.

First reported in https://github.com/hyperium/hyper/issues/3650

Example

use bytes::Buf;
use std::io::IoSlice;

struct Trickle<'a>(&'a [u8]);

impl<'a> Buf for Trickle<'a> {
    fn remaining(&self) -> usize {
        self.0.len()
    }

    fn advance(&mut self, n: usize) {
        self.0 = &self.0[n..];
    }

    fn chunk(&self) -> &[u8] {
        &self.0[0..1]
    }
}

let ab = Trickle(b"hello").chain(&b"world"[..]);    

let mut io = [IoSlice::new(&[]); 16];
let n = ab.chunks_vectored(&mut io);

println!("{:?}", &io[..n]);
// [[104], [119, 111, 114, 108, 100]]
// eg [[h], [world]]

Interestingly, the documentation for chunks_vectored even declares a guarantee that likely cannot be upheld:

If chunk_vectored does not fill every entry in dst, then dst is guaranteed to contain all remaining slices in self.

Solutions

With the way the function signature is designed, I don't see a cheap way to check after a call if it really represents all of the buf.

An expensive solution would be to provide a helper function that compares remaining() with what was placed in the &mut [IoSlice], and only if true, pass the rest of the slices to the next buffer in the list.

Another option I can think of is to design a whole new function, chunks_vectored_done_right(), with arguments/return types that prevent making that mistake, deprecating the existing method, and probably changing the default to use the expensive solution anyways.

Darksonn commented 4 months ago

Yeah, this seems like a problem.

paolobarbolini commented 3 months ago

Turns out this was already discovered by #474

Darksonn commented 3 months ago

How about removing the guarantee about chunks_vectored being complete, and adding a new method chunks_vectored_is_complete that returns a boolean describing whether this type provides the guarantee that chunks_vectored always provides all slices.