Unexpected change of behavior when reading partial data

[anforowicz]

A Skia test decodes the first 0x8D bytes of a PNG image and expects the first 10 rows of the images to successfully decode before getting UnexpectedEof error. This roughly translates to the following ad-hoc test in the png crate:

#[test]
fn test_partial_decode() {
    // The first 0x8D bytes from the following test image from Skia:
    // resources/images/apng-test-suite--dispose-ops--none-basic.png
    let partial_png: &[u8] = &[
        0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48,
        0x44, 0x52, 0x00, 0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x40, 0x08, 0x06, 0x00, 0x00,
        0x00, 0xd2, 0xd6, 0x7f, 0x7f, 0x00, 0x00, 0x00, 0x08, 0x61, 0x63, 0x54, 0x4c, 0x00,
        0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x01, 0xb9, 0xea, 0x8a, 0x56, 0x00, 0x00, 0x00,
        0x1a, 0x66, 0x63, 0x54, 0x4c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
        0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a, 0x00,
        0x64, 0x00, 0x01, 0x26, 0x12, 0x2f, 0xe0, 0x00, 0x00, 0x00, 0x93, 0x49, 0x44, 0x41,
        0x54, 0x78, 0x9c, 0xed, 0xd2, 0xa1, 0x01, 0x00, 0x30, 0x10, 0x84, 0xb0, 0xdb, 0x7f,
        0xe9, 0xef, 0x18, 0x15, 0x44, 0xc4, 0x23, 0xd8, 0x6d, 0x47, 0xd7, 0x7e, 0x07, 0x60,
        0x00, 0x0c, 0x80, 0x01, 0x30, 0x00, 0x06, 0xc0, 0x00, 0x18, 0x00, 0x03, 0x60, 0x00,
        0x0c,
    ];

    let mut reader = Decoder::new(std::io::Cursor::new(partial_png)).read_info().unwrap();
    let mut row = vec![0; reader.output_buffer_size()];
    for i in 0..10 {
        dbg!(i);
        let result = reader.read_row(&mut row);
        assert!(result.is_ok());
    }
    let result = reader.read_row(&mut row);
    let DecodingError::IoError(err) = result.unwrap_err()
        else { panic!("Unexpected error variant"); };
    assert_eq!(err.kind(), std::io::ErrorKind::UnexpectedEof);
}

The tests above (the Skia test and the ad-hoc unit test) have been passing in 0.18-rc, but are failing in 0.18. The failure is that the very first call to read_row returns UnexpectedEof. FWIW git bisect blames the change of behavior on 303b3e4. I'll try to dig in more later today or tomorrow.

[anforowicz]

I see that the call to read_row calls into next_raw_interlaced_row(rowlen=513) which:

• Sees in the loop condition that we don't yet have a full row of data
• Calls decode_image_data which sets available = 0, filled=5544 in UnfilteringBuffer
• Somewhat unexpectedly decides that we still don't yet have a full row of data. This is because the 5544 bytes are in the 32kB of Zlib look-back window.

The commit description of 303b3e4 says that it "avoids intermediate buffering". The avoided buffering was/is necessary for working with row data (i.e. mutating the row data via "unfiltering") as soon as the row data is decompressed (i.e. without waiting until the full image data is decompressed, or until 32kB data past the row is decompressed).

[anforowicz]

@197g - I think we should consider restoring the intermediate buffer to avoid regressing the ability to decode partially received images. I think this primarily means restoring a separate out_buffer: Vec<u8>, out_pos: usize, and read_pos: usize fields of ZlibStream, but 303b3e4 is pretty big and there are probably other changes that this implies. And we probably should keep the image_data: Option<...> change in the parameter type inside fn decode_image_data - AFAICT this has no impact on partial decodes and seems desirable on its own.

Having the intermediate buffer will probably have an impact on runtime performance. But an ability to display partially-decoded image seems important for clients like Chrome (/cc @fmalita and @danieldilan as a heads-up that this issue may block rolling the png to 0.18 in Skia and Chrome - see the CQ failures in https://review.skia.org/1049477).

[197g]

I think the main reason might be the change to the use of decode_next. That is, the UnexpectedEof is the one returned from src/decoder/read_decoder.rs:69 in

image-png/src/decoder/read_decoder.rs

Line 69 in 0e032e2

return Err(DecodingError::IoError(ErrorKind::UnexpectedEof.into()));

The UnfilterBuf doesn't seem immediately responsible to me. But yeah, I'll investigate further. It is responsible, since zlib needs lookback we do not yet free up the first lines of data for use.

Full test:

use png::{Decoder, DecodingError};

#[test]
fn test_partial_decode() {
    // The first 0x8D bytes from the following test image from Skia:
    // resources/images/apng-test-suite--dispose-ops--none-basic.png
    let partial_png: &[u8] = &[
        0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44,
        0x52, 0x00, 0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x40, 0x08, 0x06, 0x00, 0x00, 0x00, 0xd2,
        0xd6, 0x7f, 0x7f, 0x00, 0x00, 0x00, 0x08, 0x61, 0x63, 0x54, 0x4c, 0x00, 0x00, 0x00, 0x03,
        0x00, 0x00, 0x00, 0x01, 0xb9, 0xea, 0x8a, 0x56, 0x00, 0x00, 0x00, 0x1a, 0x66, 0x63, 0x54,
        0x4c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a, 0x00, 0x64, 0x00, 0x01, 0x26, 0x12, 0x2f,
        0xe0, 0x00, 0x00, 0x00, 0x93, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0xed, 0xd2, 0xa1, 0x01,
        0x00, 0x30, 0x10, 0x84, 0xb0, 0xdb, 0x7f, 0xe9, 0xef, 0x18, 0x15, 0x44, 0xc4, 0x23, 0xd8,
        0x6d, 0x47, 0xd7, 0x7e, 0x07, 0x60, 0x00, 0x0c, 0x80, 0x01, 0x30, 0x00, 0x06, 0xc0, 0x00,
        0x18, 0x00, 0x03, 0x60, 0x00, 0x0c,
    ];

    let mut reader = Decoder::new(std::io::Cursor::new(partial_png))
        .read_info()
        .unwrap();
    let mut row = vec![0; reader.output_buffer_size().unwrap()];
    for i in 0..10 {
        let result = reader.read_row(&mut row);
        assert!(matches!(result, Ok(_)), "{result:?} at {i}");
    }

    let result = reader.read_row(&mut row);
    let DecodingError::IoError(err) = result.unwrap_err() else {
        panic!("Unexpected error variant");
    };
    assert_eq!(err.kind(), std::io::ErrorKind::UnexpectedEof);
}

To be PR'd to avoid that kind of regression in the future.

[197g]

A note on that zlib behavior: you still get partial data, but the codes in zlib/deflate may refer to spans within the 32kB of preceding data which must be unchanged for correct decoding. So while you still get some partial data from the current state, the last 32kB of data are unavailable for unfiltering. It would seem to me we could still opportunistically preserve the copy elision but add an extra code path where an UnexpectedEof / failure to fill the buffer makes that last bit of data available by unfiltering in a separate buffer. That even has constant memory overhead.

[anforowicz]

Thanks for looking into this!

It would seem to me we could still opportunistically preserve the copy elision but add an extra code path where an UnexpectedEof / failure to fill the buffer makes that last bit of data available by unfiltering in a separate buffer.

That seems like a great idea that would 1) avoid having to revert a big chunk of 303b3e4 and 2) preserve performance runtime gains (i.e. avoiding extra copies) when the full data is available.

That even has constant memory overhead.

Right - you only need to remember the mutated/unfiltered/previous row + current/to-be-unfiltered row.

---

I hope the extra bookkeeping related to the copy-if-needed-behavior won't be too bad.

FWIW I think that baseline_reader and intermittent_eofs_reader in fuzz/fuzz_targets/buf_independent.rs should provide reasonable reassurance that the code can recover from 1) partial data / need to mutate in a separate buffer to 2) got full data / can mutate the original decompression buffer.

[197g]

Re: buf_independent.rs there's a problem we noticed which makes this signal currently very fuzzy (intermittent_eofs_reader should work though). Basically, in some situations the zlib decoder will detect errors in the symbol stream at different times based on the amount of data that is available. And in particular if the symbols are not necessary for the actual pixel data then we may never poll that error when we drip-feed the zlib bytes one-by-one. So this fuzzing test currently detects some variations that are however somewhat begnign. (#605 is related). But we had to deprioritize the fuzzer warnings; we know the cause; the fix is .. fuzzy and will only fix some corrupt images that do not work reliably elsewhere either.