Frommi / miniz_oxide

Rust replacement for miniz
MIT License
168 stars 48 forks source link

decompression logic error edge case prevents clean decompression into an exact sized buffer. #110

Closed Lokathor closed 2 years ago

Lokathor commented 2 years ago

Deep inside of decompress after all the state machines is the following:

    if status == TINFLStatus::NeedsMoreInput && out_buf.bytes_left() == 0 {
        status = TINFLStatus::HasMoreOutput
    }

If you're streaming in the decompression and your output buffer is full but the decompressor needs more input to finish reading the adler32 value, this will cause the decompressor to claim that it needs more output space when no actual additional output will occur. To ensure a successful decompression no matter how the stream is chunked up your output buffer will always have to be 1 more byte than the actual output size.

Simply deleting this line allows all tests to pass except for a single test which enumerates every possible error case in every possible situation and which expects the current behavior.

oyvindln commented 2 years ago

This will only be the case for zlib streams, as raw deflate streams won't have the checksum. Would have to test a bit to see whether that thing is needed or not, or it could simply check if the remaining data is the checksum and the stream itself is done.

Lokathor commented 2 years ago

As I said, all other existing tests passed when that one line was deleted.

oyvindln commented 2 years ago

Yup, just want to make sure we're not creating a rare edge case that's not covered by the tests with that change, will have a look at it this week.

oyvindln commented 2 years ago

Does 5869904c7f789580ace18f7e9084acbcd54a95be sort it for you? It will now skip the check if the decoder is in the ReadAdler32 state as if we are there we know for sure that there won't be any more output.

I'm a bit hesitant to just remove check entirely, it was added as a response to #68 as a safety measure to avoid data loss if both input is empty and output is full at the same time. It's possible it could be done in a better way though.

Lokathor commented 2 years ago

Yes, I rebased my branch and it no longer requires the +1 byte to pass tests.