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

[paolobarbolini]

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?

[Urgau]

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

[beepster4096]

There's two possible solutions I see here.

1. 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.
2. 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.

[nrc]

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

[paolobarbolini]

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