Unsoundness in BufReader with a broken inner Read impl

@davidzeng0

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

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)) }

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.

@rustbot

@rustbot claim

fix rust-lang#120603 by adding a check in default_read_buf

Rollup merge of rust-lang#120607 - conradludgate:fix-120603, r=dtolnay

Auto merge of rust-lang#120651 - matthiaskrgr:rollup-o5qdp0n, r=matth…

Rollup merge of rust-lang#120607 - conradludgate:fix-120603, r=dtolnay

Auto merge of rust-lang#120660 - matthiaskrgr:rollup-zuqljul, r=matth…

Unrolled build for rust-lang#120607

Navigation Menu

Repository navigation

Unsoundness in BufReader with a broken inner Read impl #120603

Meta

Issue

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	reader.read_buf(buf.unfilled())?;

	self.pos = 0;
	self.filled = buf.len();
	self.initialized = buf.init_len();

	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);
	}

Unsoundness in BufReader with a broken inner Read impl #120603

Description

Meta

Issue

Activity

conradludgate commented on Feb 3, 2024

conradludgate commented on Feb 3, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions