Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Lokathor opened this issue Nov 17, 2021 · 5 comments

Comments

@Lokathor
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@oyvindln
Copy link
Collaborator

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
Copy link
Collaborator

Does 5869904 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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants