`inflate::core::decompress` panics on input

[HeroicKatora]

The function seems to get confused when the vector is partially drained between calls. This contradicts the documentation which specifies it should never panic.

Reproduction

Execute the test render_images at this exact commit of image-png: https://github.com/image-rs/image-png/tree/cb9a340e6e1b9cd4dbcc1bb69368399d2354da77

I don't have more minimized test cases at the moment, sorry. It's getting attached as soon as one is found.

Note the manual streaming inflate implementation. It transfers 'fully decoded' data to a separate buffer provided by the caller, modifying the position of the constructed Cursor. According to documentation it leaves 32KB data in the intermediate buffer. Tracing shows that the second call to decompress panics already and skipping the call to transfer_finished_data also avoids it.

https://github.com/image-rs/image-png/blob/cb9a340e6e1b9cd4dbcc1bb69368399d2354da77/src/decoder/stream.rs#L681-L807

Expected behaviour

The PNG format is specifically designed for decoding a rough view of image in a small subset at the beginning of the image called pass extraction such as adam7 interlacing. The benefit is substantially reduced if the image data can only be decompressed as a whole.

It also puts the decoding party at risk of resource exhaustion since it may be impossible to progress without violating limits on buffer allocations.

Context: we would like to switch inflate implementation in image-png to reduce the amount of unsafe code in dependencies, better stability and performance.

[oyvindln]

Hm, seems the function may be making some assumptions on the input that we didn't realize, it's not supposed to reach a panic state.

[oyvindln]

Sorry for being slow on this, I'm looking into it now.

[oyvindln]

Should be fixed in 2a5703a4de8557dcbac981a52ea7cb1e414a169b The buffer position was stored in two variables as a leftover from the porting process, and one wasn't getting updated in one of the cases, fixed it's only stored once now. New version is up with the fix. The test runs fine after the change, I'll close once you have verified.

Btw you shouldn't need a power of 2 size buffer unless you are using a wrapping one. If that for some reason doesn't work that's another issue.

The API is a bit clunky right now, I think it needs to be overhauled. The current streaming stuff was just something quickly put together to avoid the C indirection when using it from flate2, while avoiding any breaking changes. I'll open a new issue for it.

[HeroicKatora]

Yep, seems to have fixed the panic. Not 100% on as one sample in the fuzzing corpus seems to lead to an infinite loop so fuzzing coverage is fairly small in CI. Probably a mistake in the integration though.

[oyvindln]

I'll leave this open for now then, until you can find if the issue is in miniz_oxide or elsewhere. Ill do a fuzzing run as well.

[esotericnonsense]

I have encountered this issue just now via a HTTP call using the reqwest crate.

I don't have too much information but thought I'd chime in and say that using latest master fixed the problem. If I encounter it again I'll investigate further and report back.

[oyvindln]

Hopefully it's fixed then. The newest version on crates.io has the fix. Maybe I should yank the older ones.

[Shnatsel]

Latest png relies on miniz_oxide and fuzzing has discovered no further panics. This appears to be fixed, and can be closed.