Skip to content

Buf adapters can panic when custom Buf implementations break invariants #833

Description

@xc01

Summary

I ran into a few places where bytes adapter methods trust values returned by a custom Buf implementation and then index/slice without a local check.

I understand this may be considered a Buf contract issue. Still, some of these panics happen in fallible-looking APIs like try_get_u8, so it may be worth either documenting the assumptions closer to those methods or adding small defensive checks.

Tested on:

  • bytes 1.11.1

Findings

Area API / function Panic site Trigger
Fallible reads try_get_u8 src/buf/buf_impl.rs:1211 remaining() == 1, but chunk() is empty
Fallible reads try_get_i8 src/buf/buf_impl.rs:1246 same pattern
Iterator adapter IntoIter::next src/buf/iter.rs:115 has_remaining() is true, but chunk() is empty
Vectored adapter Take::chunks_vectored src/buf/take.rs:172 inner chunks_vectored returns a count larger than dst.len()
Vectored adapter Chain::chunks_vectored src/buf/chain.rs:168 same return-count trust issue

Example 1: try_get_u8 can panic instead of returning TryGetError

try_get_u8 checks remaining(), but then directly indexes chunk()[0]:

// src/buf/buf_impl.rs:1204-1213
fn try_get_u8(&mut self) -> Result<u8, TryGetError> {
    if self.remaining() < 1 {
        return Err(TryGetError {
            requested: 1,
            available: self.remaining(),
        });
    }
    let ret = self.chunk()[0];
    self.advance(1);
    Ok(ret)
}

If a custom Buf reports one remaining byte but returns an empty chunk, this panics.

Reproducer

use bytes::Buf;

struct InconsistentBuf;

impl Buf for InconsistentBuf {
    fn remaining(&self) -> usize {
        1
    }

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

    fn advance(&mut self, _cnt: usize) {}
}

#[test]
#[should_panic(expected = "index out of bounds")]
fn try_get_u8_can_panic_with_inconsistent_buf() {
    let mut buf = InconsistentBuf;
    let _ = buf.try_get_u8();
}

Observed:

src/buf/buf_impl.rs:1211:19:
index out of bounds: the len is 0 but the index is 0

try_get_i8 has the same pattern:

// src/buf/buf_impl.rs:1246
let ret = self.chunk()[0] as i8;

Expected:

Either document near try_get_u8 / try_get_i8 that these methods still rely on the remaining() / chunk() invariant, or use chunk().first() and return TryGetError if the implementation is inconsistent.

Example 2: vectored adapters trust inner chunks_vectored counts

Take::chunks_vectored trusts the count returned by the inner buffer:

// src/buf/take.rs:168-172
let cnt = self
    .inner
    .chunks_vectored(&mut slices[..dst.len().min(LEN)]);
let mut limit = self.limit;
for (i, (dst, slice)) in dst[..cnt].iter_mut().zip(slices.iter()).enumerate() {

If the inner Buf writes one entry but returns 2, this slices past the end of dst.

Reproducer

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

struct BadBuf;

impl Buf for BadBuf {
    fn remaining(&self) -> usize {
        1
    }

    fn chunk(&self) -> &[u8] {
        b"x"
    }

    fn advance(&mut self, _cnt: usize) {}

    fn chunks_vectored<'a>(&'a self, dst: &mut [IoSlice<'a>]) -> usize {
        if !dst.is_empty() {
            dst[0] = IoSlice::new(b"x");
        }
        2
    }
}

#[test]
#[should_panic]
fn take_chunks_vectored_can_panic_with_bad_inner_count() {
    let taken = BadBuf.take(1);
    let mut out = [IoSlice::new(&[]); 1];
    let _ = taken.chunks_vectored(&mut out);
}

Observed:

src/buf/take.rs:172:37:
range end index 2 out of range for slice of length 1

Chain::chunks_vectored has the same kind of trust boundary:

// src/buf/chain.rs:167-169
let mut n = self.a.chunks_vectored(dst);
n += self.b.chunks_vectored(&mut dst[n..]);
n

Expected:

It would help to document that chunks_vectored must never return a count larger than dst.len() and that the count must match entries actually written. Another option is to clamp/assert the returned count before slicing in adapters like Take and Chain.

Additional note: documented `BytesMut::reserve(usize::MAX)` overflow panic

BytesMut::reserve already documents capacity-overflow panics:

// src/bytes_mut.rs:587-589
/// # Panics
///
/// Panics if the new capacity overflows `usize`.

One public path that reaches this is reserving usize::MAX after creating shared backing storage:

use bytes::BytesMut;

#[test]
#[should_panic(expected = "overflow")]
fn reserve_inner_overflows_after_split_to() {
    let mut buf = BytesMut::from(&b"ab"[..]);

    let _prefix = buf.split_to(1);

    buf.reserve(usize::MAX);
}

Observed:

src/bytes_mut.rs:681:21:
overflow

Relevant source:

// src/bytes_mut.rs:678-682
let mut new_cap = match len.checked_add(additional) {
    Some(new_cap) => new_cap,
    None if !allocate => return false,
    None => panic!("overflow"),
};

This is already documented, so I am not presenting it as an undocumented panic. It may still be useful to consider a fallible reservation API for callers that want to avoid unwinding on capacity overflow.

Out of scope: documented panic behavior and unsafe precondition violations

I also saw several generated panic cases that I would not include as main findings:

  • Buf::advance / Take::advance with cnt > remaining(): documented to panic.
  • Buf::copy_to_bytes with len > remaining(): documented to panic.
  • Buf::get_int(9): documented to panic when nbytes > 8.
  • BufMut::advance_mut with cnt > remaining_mut(): unsafe API and documented to possibly panic.
  • BytesMut::spare_capacity_mut, try_reclaim, and reserve after unsafe set_len creates len > capacity: this violates the unsafe precondition.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions