UB in IoReader::fill_buffer

[jethrogb]

While doing an unsafe-audit of bincode, I found this code in src/de/read.rs:

impl<R> IoReader<R>
where
    R: io::Read,
{
    fn fill_buffer(&mut self, length: usize) -> Result<()> {
        let current_length = self.temp_buffer.len();
        if length > current_length {
            self.temp_buffer.reserve_exact(length - current_length);
        }

        unsafe {
            self.temp_buffer.set_len(length);
        }

        self.reader.read_exact(&mut self.temp_buffer)?;
        Ok(())
    }
}

If read_exact errors, this code might leave temp_buffer longer than the number of initialized bytes. Also, passing uninitialized memory to read_exact is UB. Since read_exact is a safe fn, any slice that's passed in must be initialized.

From @rkruppe on Zulip:

I don't think it is [safe]. If self.reader.read_exact doesn't fill the buffer (e.g. there is an I/O error while reading, or it's implemented incorrectly, or it panics for some reason) then this code exposes uninitialized bytes to the world. And while it's still up for debate whether there are some things that can be done in careful unsafe code with uninitialized bytes, it's certain that uninitialized memory can lead to UB when used in unsuspecting safe code.

It also exposes uninitialized memory to the implementation of read_exact so if that reads from the buffer (which it "shouldn't" but safe code can do it) it'll get uninitialized bytes too, with the same consequences.

[Dr-Emann]

I think we need to handle the read_exact failure exposing uninitialized bytes. I think the best way to do so is to pass a &mut [u8] created with slice::from_raw_parts_mut with the capacity, and only update the len after read_exact succeeds

I think without rust-lang/rust#42788 (or whatever end result comes from that), we should just document that we expect Read impls to only write into passed slices: the performance hit from zeroing is (imo) too great.

[jdm]

I agree with the suggestion from the previous comment. We would accept a pull request implementing that strategy.

[atouchet]

This could probably be unpinned now as the issue is closed.

[jethrogb]

Well, as stated, the current code may still exhibit UB. I also don't see the current “requirements” (putting this in quotes as safe functions may not make such requirements) documented in a place a user would find.

[scooter-dangle]

It might be more trouble than it's worth to library consumers, but I think you could resolve the soundness issue by marking IoReader::new unsafe with the safety requirement that the user needs to verify that the particular Read impl for R obeys the advertised contract of read_exact (i.e., that it doesn't read from the buffer and always returns an Err if it fails to / can't fill the buffer).

To avoid then needing to litter unsafe calls throughout the code, a user could make a generic initializer that requires an unsafe marker trait to be implemented to signal that they've performed this verification.

[ZoeyR]

Do we have any benchmarks of what it costs to zero the buffer? Since we're reusing the buffer we won't have to zero it on every single invocation of fill_buffer, we only have to zero newly allocated bytes.

[Shnatsel]

Exposing uninitialized bytes and either leaking them to the outside world or using them in place of data is a potential security vulnerability. Please file a security advisory at https://github.com/RustSec/advisory-db so that anyone interested could get notified about this and upgrade to a version with the fix.