RFC: Reading into uninitialized buffers by sfackler · Pull Request #2930 · rust-...

Member

text/0000-read-buf.md

Outdated

Show resolved

sfackler commented on May 19

Yeah, this should be applicable to AsyncRead without any real issue.

One related thing to think about is how well this proposal would mesh with async reads where the kernel is the buffer owner, not the caller. See https://vorner.github.io/2019/11/03/io-uring-mental-experiments.html for some reasoning.

sfackler commented on May 20

I believe this is basically orthogonal to an io_uring interface.

burdges commented on May 20 •

Is there an existing crate to develop ReadBuf and friends outside std for people who want to use this right now? I'm thinking like other "std v2" projects like futures before and concurrently no_std Error?

joshtriplett commented on May 20

I think it'd be valuable to specify a feature (even if for internal use only) that allows a trait to specify a list of lists of methods that constitute a minimum implementation, such that an impl that doesn't provide at least one of the minimum implementations will get a warning.

sfackler commented on May 20

@burdges I have some of the code in https://github.com/sfackler/read-buf, but it currently uses an older version of ReadBuf that doesn't track the written cursor internally.

rpjohnst commented on May 20

Regarding the unresolved question of whether to include the written field... what sort of performance impact would we expect there, from the two choices?

With the written field included in ReadBuf, there can be fewer asserts, but the struct itself is larger, on top of becoming a by-reference argument where there once were direct by-value argument and return value. These are probably both mitigated by #[inline], but then there's the question of the impact on compile times.

Without the written field, the struct is smaller, but there are more asserts checking impl correctness. How easy is it to optimize out those asserts? Are there idioms people will learn similar to those that make bounds checks more optimizable?

Or is this difference lost in the noise because we're doing I/O? Are there cases where this might not be true?

djc commented on May 20

It might be nice if there was a safe interface for writing to the uninitialized part of a ReadBuf from an already existing slice of buffered data (that is, is there a useful intersection of "safe interface" and "no need to unnecessarily initialize").

I did not read the Dropbox Paper document now (and I think I would feel slightly better if the RFC didn't rely on linking to it). Does it cover instead having a ReadBuf trait, with the Read impl taking that type as an argument (with default)? This could potentially allow some abstraction over things that could be wrapped, as some others have mentioned upthread.

It might be nice if there was a safe interface for writing to the uninitialized part of a ReadBuf from an already existing slice of buffered data (that is, is there a useful intersection of "safe interface" and "no need to unnecessarily initialize").

Seems like you want ReadBuf::append, which is already included.

Amanieu commented on May 21

Have you considered renaming written to filled? I found written a bit confusing at first glance since this type is used when reading data from a file, not writing.

sfackler commented on May 21

Ooh yeah, filled is much cleaner than written! I will update with that and @RalfJung's method name suggestions tonight.

rfcbot commented on Sep 8

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

sfackler commented on Sep 8 •

While preparing the PR to implement the new APIs in std, I came across a bit of a footgun in the current design: a malicious Read implementation can replace the ReadBuf passed to it with a different one. If the code using the reader is just looking at the number of bytes filled/initialized in the ReadBuf (e.g. the implementation of read_to_end in the RFC text), this can cause soundness problems as the initialized count no longer refers to the buffer that the code assumes it is:

impl Read for BadReader {
    fn read_buf(&mut self, buf: &mut ReadBuf<'_>) -> io::Result<()> {
        *buf = ReadBuf::new(Box::leak(vec![0; buf.capacity()].into_boxed_slice()));
        buf.add_filled(buf.remaining());
        Ok(())
    }
}

To the best of my knowledge this can only be done by leaking a Vec<u8> like in the example above, so this should only be a problem with actively malicious implementations. It can be worked around in the calling code by checking that the buffer address after the call to read_buf matches the expected value, but that's IMO a pretty sketchy thing to have to remember to do.

I think the fix is to prevent the Read implementations from replacing the buffer. @Amanieu suggested using Pin, but that feels like a bit of an abuse of the Pin interface - ReadBuf is always movable after all! We just need to prevent replacement in this one place. The alternative is to essentially wrap the mutable reference in another type which is passed by value into read_buf:

pub struct ReadBuf<'a> { ... }

impl<'a> ReadBuf<'a> {
    pub fn as_ref(&mut self) -> ReadBufRef<'_> { ... }
}

pub struct ReadBufRef<'a> { ... }

pub trait Read {
    fn read_buf(&mut self, buf: ReadBufRef<'_>) -> io::Result<()> { ... }
}

This unfortunately results in two types that will have very similar APIs. I think I'm going to land the initial PR with the API proposed by the RFC text, but this issue will need to be fixed before stabilization.

Diggsey commented on Sep 8 •

a malicious Read implementation can replace the ReadBuf passed to it with a different one.

Doesn't the lifetime parameter make this difficult? Could it be solved by making it invariant so you can't just coerce a ReadBuf with a static lifeitme?

RalfJung commented on Sep 8 •

Difficult but not impossible if you have a 'static buffer, like via Box::leak.

sfackler commented on Sep 8

It does not - Box::leak will produce a reference with any lifetime.

Thomasdezeeuw commented on Oct 15 •

I would like to propose a different API based on a trait rather than a new type. It allows &mut [u8] to be used in the same way it is now, but allows easier use of Vec<u8> and any custom buffer types.

The trait is following.

/// Trait to make easier to work with slices of (uninitialised) bytes.
///
/// This is implemented for common types such as `&mut[u8]` and `Vec<u8>`.
pub trait Bytes {
    /// Returns itself as a slice of bytes that may or may not be initialised.
    /// 
    /// # Unsafety
    /// 
    /// Caller must ensure to only write valid bytes to the returned slice.
    unsafe fn as_bytes(&mut self) -> &mut [MaybeUninit<u8>];

    /// Update the length of the byte slice.
    ///
    /// # Safety
    ///
    /// The caller must ensure that at least `n` of the bytes returned by
    /// [`Bytes::as_bytes`] are initialised.
    unsafe fn update_length(&mut self, n: usize);
}

It should be used in the following way:

1. The read implementation calls Byte::as_bytes and writes bytes into the returned slice.
2. The read implementation calls Bytes::update_length updating the buffers length.
3. The caller now has a correctly sized buffer.

This my implementation for TcpStream::try_recv for example:

impl TcpStream {
    pub fn try_recv<B>(&mut self, mut buf: B) -> io::Result<usize>
    where
        B: Bytes,
    {
        let dst = buf.as_bytes();
        debug_assert!(
            !dst.is_empty(),
            "called `TcpStream::try_recv with an empty buffer"
        );
        syscall!(recv(
            self.socket.as_raw_fd(),
            dst.as_mut_ptr().cast(),
            dst.len(),
            0, // Flags.
        ))
        .map(|read| {
            let read = read as usize;
            // Safety: just read the bytes.
            unsafe { buf.update_length(read) }
            read
        })
    }
}

Some implementations of Bytes:

For slices, both using [u8] and [MaybeUninit<u8>].

impl Bytes for [MaybeUninit<u8>] {
    fn as_bytes(&mut self) -> &mut [MaybeUninit<u8>] {
        self
    }

    unsafe fn update_length(&mut self, _: usize) {
        // Can't update the length of a slice.
    }
}

impl Bytes for [u8] {
    fn as_bytes(&mut self) -> &mut [MaybeUninit<u8>] {
        // Safety: `MaybeUninit<u8>` is guaranteed to have the same layout as
        // `u8` so it same to cast the pointer.
        unsafe { &mut *(self as *mut [u8] as *mut [MaybeUninit<u8>]) }
    }

    unsafe fn update_length(&mut self, _: usize) {
        // Can't update the length of a slice.
    }
}

This results is basically the same usage as Read::read currently because the user still needs to slice the buffer properly themselves, but does allow the use of MaybeUninit<u8>. However the implement for Vec<u8> is much nicer (see usage below).

impl Bytes for Vec<u8> {
    fn as_bytes(&mut self) -> &mut [MaybeUninit<u8>] {
        // Safety: `Vec` ensures the pointer is correct for us. The pointer is
        // at least valid for start + `Vec::capacity` bytes, a range we stay
        // within.
        unsafe {
            let len = self.len();
            let data_ptr = self.as_mut_ptr().add(len).cast();
            // NOTE: `Vec` ensure capacity >= len, so this is always >= 0.
            let capacity_left = self.capacity() - len;
            slice::from_raw_parts_mut(data_ptr, capacity_left)
        }
    }

    unsafe fn update_length(&mut self, n: usize) {
        // Safety: caller must ensure that `n` is valid.
        self.set_len(self.len() + n);
    }
}

This allow the following to just work as expected.

let mut stream = TcpStream::connect(address)?;
let mut buf = Vec::with_capacity(4 * 1024); // 4 KB.
let n = stream.recv(&mut buf)?;
// `buf` now contains `n` bytes with it length set to `buf.len() + n`.
// Tthe bytes are added to the `Vec`, not overwritten.

WaffleLapkin commented on Oct 15 •

@Thomasdezeeuw please note that your impl Bytes for [u8] is unsound. While converting Box<[T]> -> Box<[MaybeUninit<T>]> or &[T] -> &[MaybeUninit<T>] is ok, converting &mut [T] to &mut [MaybeUninit<T>] is UB as you may write uninit values to T.

let mut buf = [0u8];
let maybe = unsafe { &mut *((&mut buf) as *mut [u8] as *mut [MaybeUninit<u8>]) };
// After this line `buf` will contain uninitialized value, which is UB
maybe[0] = MaybeUninit::uninit();

See also: rust-lang/rust#66699

But note, @Thomasdezeeuw, that your suggestion can be fixed the same way an actual crate with a similar API to your suggestion did: inventing (and using) an &out [u8] reference abstraction: https://docs.rs/uninit/0.4.0/uninit/out_ref/struct.Out.html

It's basically &mut [u8], but for the ability to write garbage to the pointee, which is what makes &mut [u8] -> &mut [MU<u8>] unsound.

@WaffleLapkin, @danielhenrymantilla good points. I didn't think about that case. I could be somewhat easily fixed by making Bytes::as_bytes unsafe and requiring the caller to only write valid bytes (I've updated my comment #2930 (comment)). Or as @danielhenrymantilla suggested some kind of wrapper type.

maybe Read::read should have a default implementation and also have an annotation making rustc require at least one of Read::read, Read::read_buf, or the vectored read functions must be implemented.

nikomatsakis commented on Oct 28 •

Huzzah! The @rust-lang/libs team decided in September to accept this RFC. I have merged it into the repository. The tracking issue is rust-lang/rust#78485, if you would like to follow along from there.