Fix block_open handling in deflate_quick() #880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The attached test fails with "inflate() failed", because the deflate stream that it produces ends up being corrupted. Bisect points to the commit e7bb6db ("Replace hash_bits, hash_size and hash_mask with defines."), but it's most likely a coincidence. In any case, the reason is that if we happen to simultaneously exhaust all the buffers (in, out and bi), we return finish_started without writing the end of block symbol, which will never happen afterwards. Fix by adding another check to the tricky condition: if we are in the middle of a block, return need_more instead of finish_started.
Codecov Report
@@ Coverage Diff @@
## develop #880 +/- ##
===========================================
- Coverage 75.56% 75.43% -0.13%
===========================================
Files 72 73 +1
Lines 8172 8220 +48
Branches 1349 1359 +10
===========================================
+ Hits 6175 6201 +26
- Misses 1480 1494 +14
- Partials 517 525 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@iii-i Great work on finding this, I'll release a hotfix for this. Do you have a scripted fuzzing setup that finds these? If so, is it something you can/want to share? |
|
Yeah, that's essentially 08274c7 + some new modifications. I should probably open source the full repo where I develop this, and then we can think whether it would make sense to integrate this into zlib-ng. |
|
@iii-i That looks nice indeed, but as I said I have no experience with fuzzing, so I would not know how to build or run that thing 😄 Getting it into zlib-ng/zlib-ng or perhaps better, a separate zlib-ng/fuzzing (or similar) repo would be great, and would make it easier for everyone to contribute both code and testing. |
|
I am getting these compiler warnings now on |
|
Weird that clang and/or gcc didn't catch it. There indeed are a few casts missing in this test. I'll send a PR tomorrow. |
iii-i
added a commit
to iii-i/zlib-ng
that referenced
this pull request
Mar 19, 2021
Add casts in order to fix the following warnings [1]: C:\Users\Nathan\Source\zlib-ng\test\deflate_quick_block_open.c(62,69): warning C4244: '=': conversion from '__int64' to 'uint32_t', possible loss of data [C:\Users\Nathan\Source\zlib-ng\deflate_quick_block_open.vcxproj] C:\Users\Nathan\Source\zlib-ng\test\deflate_quick_block_open.c(73,1): warning C4244: 'initializing': conversion from '_ _int64' to 'uint32_t', possible loss of data [C:\Users\Nathan\Source\zlib-ng\deflate_quick_block_open.vcxproj] [1] zlib-ng#880 (comment)
Dead2
pushed a commit
that referenced
this pull request
Mar 22, 2021
Add casts in order to fix the following warnings [1]: C:\Users\Nathan\Source\zlib-ng\test\deflate_quick_block_open.c(62,69): warning C4244: '=': conversion from '__int64' to 'uint32_t', possible loss of data [C:\Users\Nathan\Source\zlib-ng\deflate_quick_block_open.vcxproj] C:\Users\Nathan\Source\zlib-ng\test\deflate_quick_block_open.c(73,1): warning C4244: 'initializing': conversion from '_ _int64' to 'uint32_t', possible loss of data [C:\Users\Nathan\Source\zlib-ng\deflate_quick_block_open.vcxproj] [1] #880 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While tracking down #869, I stumbled upon another deflate_quick issue :-(
The attached test fails with "inflate() failed", because the deflate
stream that it produces ends up being corrupted. Bisect points to the
commit e7bb6db ("Replace hash_bits, hash_size and hash_mask with
defines."), but it's most likely a coincidence.
In any case, the reason is that if we happen to simultaneously exhaust
all the buffers (in, out and bi), we return finish_started without
writing the end of block symbol, which will never happen afterwards.
Fix by adding another check to the tricky condition: if we are in the
middle of a block, return need_more instead of finish_started.