Unsound BufWriter copy_to specialization with the unstable read_buf feature

Code

#![feature(read_buf)]

use std::io::{self, BufWriter, Read, ReadBuf};

struct BadReader;

impl Read for BadReader {
    fn read(&mut self, _buf: &mut [u8]) -> io::Result<usize> {
        unreachable!()
    }

    fn read_buf(&mut self, read_buf: &mut ReadBuf<'_>) -> io::Result<()> {
        let vec = vec![0; 2048];

        let mut buf = ReadBuf::new(vec.leak());
        buf.append(&[123; 2048]);
        *read_buf = buf;

        Ok(())
    }
}

fn main() {
    let mut buf = Vec::new();

    {
        let mut buf_ = BufWriter::with_capacity(1024, &mut buf);

        let mut input = BadReader.take(4096);
        io::copy(&mut input, &mut buf_).unwrap();
    }

    // read the memory to trigger miri to realize that the memory is uninitialized
    println!("buf[0]: {}", buf[0]);
}

Playground link: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=483f0b4f2f881519f459c3cd273a50f4 (run miri on it)

Unsound implementation

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

Lines 80 to 122 in 92ed874

    
           fn copy_to<R: Read + ?Sized>(reader: &mut R, writer: &mut Self) -> Result<u64> { 
        
               if writer.capacity() < DEFAULT_BUF_SIZE { 
        
                   return stack_buffer_copy(reader, writer); 
        
               } 
        
               let mut len = 0; 
        
               let mut init = 0; 
        
               loop { 
        
                   let buf = writer.buffer_mut(); 
        
                   let mut read_buf = ReadBuf::uninit(buf.spare_capacity_mut()); 
        
                   // SAFETY: init is either 0 or the initialized_len of the previous iteration 
        
                   unsafe { 
        
                       read_buf.assume_init(init); 
        
                   } 
        
                   if read_buf.capacity() >= DEFAULT_BUF_SIZE { 
        
                       match reader.read_buf(&mut read_buf) { 
        
                           Ok(()) => { 
        
                               let bytes_read = read_buf.filled_len(); 
        
                               if bytes_read == 0 { 
        
                                   return Ok(len); 
        
                               } 
        
                               init = read_buf.initialized_len() - bytes_read; 
        
                               // SAFETY: ReadBuf guarantees all of its filled bytes are init 
        
                               unsafe { buf.set_len(buf.len() + bytes_read) }; 
        
                               len += bytes_read as u64; 
        
                               // Read again if the buffer still has enough capacity, as BufWriter itself would do 
        
                               // This will occur if the reader returns short reads 
        
                               continue; 
        
                           } 
        
                           Err(ref e) if e.kind() == ErrorKind::Interrupted => continue, 
        
                           Err(e) => return Err(e), 
        
                       } 
        
                   } 
        
                   writer.flush_buf()?; 
        
               } 
        
           }

(we're basically tricking it into calling set_len with a length higher than what has actually been filled)

Documentation

Should this be considered a possible footgun when implementing something that calls read_buf? Should it be documented?

@drmeepster

This was already mention in the PR that added ReadBuf #81156 (comment)
cc @drmeepster @joshtriplett

There's two possible solutions I see here.

The easiest thing to do to fix this is just check if the ReadBuf points to the same buffer as before and return an error if it doesn't. This doesn't seem very "rusty" to me though, relying on a runtime check.
The other solution is to have a ReadBufRef type passed by value to read_buf instead of &mut ReadBuf. This solves the issue in general at compile time, but could make this api a lot more complicated to use.

I think another example of unsound code in std for a similar reason is in the read_buf implementation for Take.

I think another example of unsound code in std for a similar reason is in the read_buf implementation for Take.

Yes it seems like a similar attack could be done against it

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

Lines 2584 to 2601 in 18f32b7

    
           let mut sliced_buf = ReadBuf::uninit(ibuf); 
        
           // SAFETY: extra_init bytes of ibuf are known to be initialized 
        
           unsafe { 
        
               sliced_buf.assume_init(extra_init); 
        
           } 
        
           self.inner.read_buf(&mut sliced_buf)?; 
        
           let new_init = sliced_buf.initialized_len(); 
        
           let filled = sliced_buf.filled_len(); 
        
           // sliced_buf / ibuf must drop here 
        
           // SAFETY: new_init bytes of buf's unfilled buffer have been initialized 
        
           unsafe { 
        
               buf.assume_init(new_init); 
        
           }

Rollup merge of rust-lang#97015 - nrc:read-buf-cursor, r=Mark-Simulacrum

Navigation Menu

Repository navigation

Unsound `BufWriter` copy_to specialization with the unstable `read_buf` feature #93305

Code

Unsound implementation

Documentation

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	fn copy_to<R: Read + ?Sized>(reader: &mut R, writer: &mut Self) -> Result<u64> {
	if writer.capacity() < DEFAULT_BUF_SIZE {
	return stack_buffer_copy(reader, writer);
	}

	let mut len = 0;
	let mut init = 0;

	loop {
	let buf = writer.buffer_mut();
	let mut read_buf = ReadBuf::uninit(buf.spare_capacity_mut());

	// SAFETY: init is either 0 or the initialized_len of the previous iteration
	unsafe {
	read_buf.assume_init(init);
	}

	if read_buf.capacity() >= DEFAULT_BUF_SIZE {
	match reader.read_buf(&mut read_buf) {
	Ok(()) => {
	let bytes_read = read_buf.filled_len();

	if bytes_read == 0 {
	return Ok(len);
	}

	init = read_buf.initialized_len() - bytes_read;

	// SAFETY: ReadBuf guarantees all of its filled bytes are init
	unsafe { buf.set_len(buf.len() + bytes_read) };
	len += bytes_read as u64;
	// Read again if the buffer still has enough capacity, as BufWriter itself would do
	// This will occur if the reader returns short reads
	continue;
	}
	Err(ref e) if e.kind() == ErrorKind::Interrupted => continue,
	Err(e) => return Err(e),
	}
	}

	writer.flush_buf()?;
	}
	}

	let mut sliced_buf = ReadBuf::uninit(ibuf);

	// SAFETY: extra_init bytes of ibuf are known to be initialized
	unsafe {
	sliced_buf.assume_init(extra_init);
	}

	self.inner.read_buf(&mut sliced_buf)?;

	let new_init = sliced_buf.initialized_len();
	let filled = sliced_buf.filled_len();

	// sliced_buf / ibuf must drop here

	// SAFETY: new_init bytes of buf's unfilled buffer have been initialized
	unsafe {
	buf.assume_init(new_init);
	}

Unsound BufWriter copy_to specialization with the unstable read_buf feature #93305

Description

Code

Unsound implementation

Documentation

Activity

Urgau commented on Jan 25, 2022

beepster4096 commented on Jan 26, 2022

nrc commented on Apr 8, 2022

paolobarbolini commented on Apr 10, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Unsound `BufWriter` copy_to specialization with the unstable `read_buf` feature #93305