Unsoundness in BufReader with a broken inner Read impl

[conradludgate]

I tried this code:

(source: @davidzeng0 from the Community Discord server)
https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=198b86fbcfb8144e6fbb39d037f95f4e

use std::io::{Read, BufReader, Result};

pub struct MalformedRead {}

impl Read for MalformedRead {
    fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
        Ok(buf.len() * 20)
    }
}

fn main() {
    let mut reader = BufReader::new(MalformedRead {});
    let mut buf = [0u8; 1024];
    
    for _ in 0..8 {
        let read = reader.read(&mut buf).unwrap();
        dbg!(read);
    }

    reader.read(&mut buf).unwrap();
    dbg!(buf);
}

I expected to see this happen: panic, out of bounds read

Instead, this happened:

In debug: SEGMENTATION VIOLATION
In release: Heap buffer overflow
In miri: Undefined Behavior: out-of-bounds pointer use: alloc893 has size 8192, so pointer to 163840 bytes starting at offset 0 is out-of-bounds

Meta

Stable (1.75.0) and Nightly (1.77.0-nightly 2024-02-01 bf3c6c5) have the issue.

Issue

BufReader uses read_buf to read from the Reader, using the BorrowedBuf's len as the filled value.

rust/library/std/src/io/buffered/bufreader/buffer.rs

Lines 114 to 118 in bf3c6c5

	reader.read_buf(buf.unfilled())?;
	self.pos = 0;
	self.filled = buf.len();
	self.initialized = buf.init_len();

The default_read_buf function calls advance(n) with the dodgy read amount.

rust/library/std/src/io/mod.rs

Lines 580 to 584 in bf3c6c5

	let n = read(cursor.ensure_init().init_mut())?;
	unsafe {
	// SAFETY: we initialised using `ensure_init` so there is no uninit data to advance to.
	cursor.advance(n);
	}

BufReader uses the filled value as the upper bound of the read buffer.

rust/library/std/src/io/buffered/bufreader/buffer.rs

Line 42 in bf3c6c5

unsafe { MaybeUninit::slice_assume_init_ref(self.buf.get_unchecked(self.pos..self.filled)) }

[conradludgate]

It's unclear whether

• default_read_buf should be fixed to check that n is in bounds
• BufReader should be fixed to check that the buf.len() is in bounds

Reading the current safety comments, it appears that default_read_buf is in the wrong:

Safety
The caller must ensure that the first n bytes of the cursor have been properly initialised.

this is violated as only 8192 bytes are intiialised.

I would personally suggest that default_read_buf be updated to add a bounds check.

[conradludgate]

@rustbot claim