Tracking issue for Read::initializer

Implemented in #42002.

Open questions / blockers

The code currently creates references to uninitialized integers (grep for 42788), can we avoid that?

I'm personally no longer sold on the infrastructure that we've got here I think. I was initially hoping that we'd have some safe way of working with these APIs and such, but in the end it all turned up unsafe. In light of all that I'd personally advocate for much simpler API:

impl Read {
    #[inline]
    unsafe fn may_read_buffer(&self) -> bool { true }
}

I don't think the complexity we have today is worth the cost currently, and I'd imagine that we could start making abstractions like:

fn initialize_buffer_for(r: &Read, buf: &mut [u8]) {
    // ...
}

I wrote a tiny library for reading into uninitialized buffers: https://crates.io/crates/buffer/

Maybe its ideas could be adapted for the standard library. The following is safe code using the library:

let mut vec = Vec::with_capacity(1024);
if reader.read_buffer(&mut vec)?.len() != 0 {
    if vec[0] == 0 {
        // ...
    }
}

Adoption of something like this in the standard library would probably also help with adoption by third party crates, see e.g. tokio-rs/mio#628.

An alternative to the current API is to go along the path that TrustedLen did, which is just to have an unsafe trait.

unsafe trait ReadUninit: Read {}

With specialization landing, anyone who cares to can take advantage of ReadUinit:

impl<T: Read> ReadBuf for T {
    fn read_buf<B: BufMut>(&mut self, buf: &mut B) {
        self.read(buf.zeroed())
    }
}

impl<T: ReadUninit> ReadBuf for T {
    fn read_buf<B: BufMut>(&mut self, buf: &mut B) {
        self.read(buf.bytes())
    }
}

That's incompatible with trait objects which are used pretty heavily with Read in particular.

Specialization is incompatible with trait objects?

If I have a &mut Read, I have no way of checking if it implements ReadUninit or not.

@RalfJung

@RalfJung suggested a function that takes &mut [MaybeUninit<u8>]

@sfackler

@sfackler do you recall if we have a tracking issue for the alternative method of implementing this where we hint to the compiler that a buffer shouldn't be considered as undefined? (the strategy we discussed at the last work week)

There's a bunch of discussion in the PR adding it: #58363.

It turns out that we explicitly decided to not go in that direction in the lead up to 1.0, so I think it's worth an RFC that I haven't gotten around to writing. In particular, that approach can change behavior for user-provided Read implementations that are passed to e.g. io::copy even before it's stable.

I second the proposal for &mut [MaybeUninit<u8>] especially since the API design with MaybeUninit seems to be going in that direction anyway; there's just a few methods that I think need to be added to make this work nicely:

// std::mem
impl<T> MaybeUninit<T> {
    pub fn init_all(uninit: &mut [MaybeUninit<T>], val: T) -> &mut [T]  where T: Copy { // `T` could even be `Clone` since this method is panic-safe except for leaks
        ... // write `val` to all elements
        Self::as_init_all(uninit)
    }

    pub fn init_copy(uninit: &mut [MaybeUninit<T>], data: &[T]) -> &mut [T] where T: Copy { // `T` could also be `Clone`
        assert_eq!(uninit.len(), data.len());
        unsafe {
            Self::as_first_ptr(uninit).copy(data, uninit.len());
        }
        Self::as_init_all(uninit)
    }

    pub unsafe fn as_init_all(uninit: &mut [MaybeUninit<T>] -> &mut [T] { 
        ... // assume-initialized intrinsic
        mem::transmute(uninit)
    }
}

// std::io
pub trait Read {
    // ... existing methods

    fn read_uninit(&mut self, uninit: &mut [MaybeUninit<u8>]) -> io::Result<()> {
        self.read(MaybeUninit::init_all(uninit, 0))
    }
}

Then implementations that don't read the buffer can override read_uninit() and use MaybeUninit::as_first_ptr() to pass it to a syscall or copy to it from another buffer with init_copy().

I wonder about having a method like the following in Read.

fn read_uninit<'a>(&mut self, buf: &'a mut [MaybeUninit<u8>]) -> Result<&'a mut [u8]> {
    for byte in &mut *buf {
        unsafe {
            *byte.as_mut_ptr() = 0;
        }
    }
    let initialized = unsafe { mem::transmute::<&mut [MaybeUninit<u8>], &mut [u8]>(buf) };
    let size = self.read(initialized)?;
    Ok(&mut initialized[..size])
}

The reason for it returning Result<&'a mut [u8]> is to allow using this in safe code as well as preventing malicious implementations - if Result<usize> was returned, as it is a case for other Read methods, an implementation could just say Ok(42) and watch the world burn when trying to interpret the uninitialized bytes. It is the responsibility of an implementation to ensure the mutable slice it returns is initialized.

The choice of 0 assignment is somewhat arbitrary. It can be some other constant or frozen uninitialized memory, as long it doesn't outright cause UB it's fine.

Note that such an implementation doesn't ensure that that result.as_ptr() == MaybeUninit::first_ptr(buf). Unsafe implementations that depend on this should check for this manually. This is necessary, as implementations could return values like Box::leak(vec![1, 2, 3].into_boxed_slice()) otherwise, which isn't verified by the compiler. Optimizer should be able to remove this assertion most of the time.

The default implementation will make that pretty hard to use in practice, unless you already know that your Read implementation overrides it. You're commonly calling read with a large buffer and don't expect it to be entirely filled, so writing to the entire thing before the raw read call imposes a very large performance penalty. There's an issue somewhere on Tokio's issue tracker about a similar issue with their AsyncRead::read_buf method.

Rollup merge of rust-lang#64123 - danielhenrymantilla:add_comment_abo…

Rollup merge of rust-lang#76242 - RalfJung:read-fixme, r=Dylan-DPC

Can this be closed now that the reading into unintialized buffers RFC is merged and #78485 exists? Since the API is going to be removed anyway, it seems pointless to keep this open.

It will be closed when the API is removed.

Remove Initializer, unsafe feature, compiles

Auto merge of rust-lang#81156 - DrMeepster:read_buf, r=joshtriplett

Navigation Menu

Repository navigation

Tracking issue for `Read::initializer` #42788

Open questions / blockers

7 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Tracking issue for Read::initializer #42788

Description

Open questions / blockers

Activity

alexcrichton commented on Aug 29, 2017

tbu- commented on Dec 13, 2017

seanmonstar commented on Feb 21, 2018

sfackler commented on Feb 21, 2018

seanmonstar commented on Feb 21, 2018

sfackler commented on Feb 21, 2018

jethrogb commented on Jan 28, 2019

alexcrichton commented on Apr 10, 2019

sfackler commented on Apr 10, 2019

abonander commented on Jul 10, 2019

sugar700 commented on Jul 31, 2019

sfackler commented on Jul 31, 2019

7 remaining items

Kestrer commented on Jan 10, 2021

sfackler commented on Jan 10, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Tracking issue for `Read::initializer` #42788