test on custom type with std::io::Read impl fails on 1.76.0 about rust HOT 8 CLOSED

@jieyouxu just tested with 1.76.0 and the regression was there so updated the comment and title

from rust.

This is failing with a failed to fill whole buffer message, which points at read_exact, not read_to_end. So it's probably not #118222

The false branch in that match is kind of sus anyway. Turning a single read call into multiple read* calls can lead to bytes from an earlier call getting dropped when a later one returns an error. But I haven't analyzed the code yet, so it might not be relevant here.

Edit: Oh I see it's a bisection result. I'll take another look.

from rust.

searched nightlies: from nightly-2023-11-11 to nightly-2024-02-01
regressed nightly: nightly-2023-11-30
searched commit range: 5facb42...b10cfcd
regressed commit: bbefc98

cargo bisect-rustc --start=1.75.0 --end=1.77.1 -- test

from rust.

I'm going to guess it's #118222 cc @the8472. Pinging because not sure if the change in observable behavior here is intended.

from rust.

I did some testing for what types of Vec initialization this code actually panics and found these results:

// 1.75.0 panic, 1.76.0 panic
let mut v = Vec::new();

// 1.75.0 panic, 1.76.0 panic
let mut v = Vec::with_capacity(7);

// 1.75.0 NO panic, 1.76.0 panic
let mut v = Vec::with_capacity(8);

Looking at these results I would guess that the old behavior was actually wrong and that it got fixed.

from rust.

@konsti219 looks like initialisation is needed now e.g like vec![0; 12] so that is a fix if using read_exact

however read_to_end is still broken

from rust.

Ok, I had a closer look at the code.

The issue is with both the read function and with the test.

A) The test assumes that read_to_end would pass in the Vec's spare capacity to the reader. This is not guaranteed. Both the current and the previous impl used a stack buffer for probing, #118222 just changed when the probing happens.

B) The read function does not handle large buffers getting passed in properly.

                        let len = buf.len();
                        let to_end = (self.first_len - self.pos) as usize;
                        match to_end >= len {
                            true => self.first.read(buf)?,
                            false => {
                                let mut first = vec![0; to_end];
                                let mut second = vec![0; len - to_end];
                                self.first.read_exact(&mut first)?;
                                sec.read_exact(&mut second)?;
                                first.append(&mut second);
                                first.as_slice().read(buf)?

With buf: &mut [u8; 32] then len == 32. In the test to_end = 8 - 0 = 8. Therefore the false case is taken.
Now second = vec![0; 32 - 8], 24 bytes.
But sec is only 4 bytes long.
Therefore read_exact fails.

So the change in the read_to_end impl merely uncovered preexisting bug in your code.

from rust.

So the change in the read_to_end impl merely uncovered preexisting bug in your code.

thanks so much @the8472 - so it was that buf is now not always guaranteed to match the vec capacity which also exposed bad parity with large buffers

changing to this now allows passing the test :)

let len = buf.len() as u64;
let to_end = self.first_len - self.pos;
match to_end >= len {
    true => self.first.read(buf)?,
    false => {
        let mut first = vec![0; to_end as usize];
        let excess = len - to_end;
        let mut second = vec![
            0;
            match excess > self.second_len {
                true => self.second_len,
                false => excess,
            } as usize
        ];
        self.first.read_exact(&mut first)?;
        sec.read_exact(&mut second)?;
        first.append(&mut second);
        first.as_slice().read(buf)?
    }
}

thank you so much - this turned out to be an improvement rather than a regression

from rust.