Repository navigation
CRC issue with deflate compression and a read loop #414
Description
With a particular output buffer, a zip file with an AES encrypted (this may be red herring) and zlib compressed item fails reading with a CRC error. I noticed it with a buffer size of 8192 and a uncompressed item of size 7077903, but I saw it at some other combinations.
On investigation, it appears that zlib::inflate may read data from the input buffer and/or write it to the output buffer, but not neccessarily both. "If inflate returns Z_OK and with zero avail_out, it must be called again after making room in the output buffer because there might be more output pending." This means: zlib may consume X bytes, decompress them to an internal buffer, and then output the decompressed results over multiple calls. In the current implementation of mz_strm_zlib, the decompress loop will be escaped if there is no data to read and the minizip controlled buffer is empty, though there may be more data to output.
main.cpp.zip is a test script that shows the problem.
Tested against minizip 2.8.9 (also 2.8.1) with
- iconv 1.15
- OpenSSL 1.0.2p
- bzip2 1.0.6
- zlib 1.2.11
Activity
nmoinvaz commentedon Sep 6, 2019
Does changing mz_strm_zlib.c at line 208 from:
To:
Solve the issue?
rddesmond commentedon Sep 6, 2019
No, but the issue disappears if I remove mz_strm_zlib.c:171 and mz_strm_zlib.c:172, where it doesn't continue to check
inflateif the parent stream doesn't have any more data.I'm testing to see if that is sufficient and doesn't break anything, though so far it looks like it is working. Do you think any other decompression libraries need a similar change? It seems reasonable that because they can decompress unpredictable amounts, they will have to have some internal mechanism to store it when it doesn't fit in the output.
nmoinvaz commentedon Sep 7, 2019
minizip probably uses
inflatea bit differently than other libraries with the flush parameterZ_SYNC_FLUSH. Other libraries might useZ_NO_FLUSHfollowed byZ_FINISH. In the Z_FINISH state it would finish writing all remaining bytes.nmoinvaz commentedon Sep 7, 2019
I have made a similar change to the bzip and lzma streams in case the problem is also there. Thanks reporting and fixing.
Change lzma & bzip read loop in case output is truncated. #414 #415
rddesmond commentedon Sep 7, 2019
Thank you!
nmoinvaz commentedon Sep 16, 2019
I took another look at this. I'm not so sure
&& (in_bytes > 0 || out_bytes > 0)is needed. Also I found a bug in themain.cppin the test_assert. It wouldn't return the right error number becauseerrwould be evaluated twice in a #define.rddesmond commentedon Sep 18, 2019
Thank for the second look. The most important part of my PR was the removal of the
if (read == 0)short circuit. The addition to the while loop clause(in_bytes > 0 || out_bytes > 0)is probably not needed, but I was a little cautious to make sure we didn't get stuck in a no-work loop. I added it to give an additional "has done no work" end case to the loop, though it looks likeZ_STREAM_ENDcovers that case completely.I see now that you added the while loop clause but not the removal of the short circuit to the other method. The removal of the short circuit is the change that really fixed it. (In zlib, I didn't encounter truncated output in the other streams, but certainly could have missed it.)
Good catch on
test_assert. I got sloppy because it was just code to show the problem and not in the PR.. but if you want it in it, I'm happy to revise.