CRC issue with deflate compression and a read loop

[rddesmond]

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

[nmoinvaz]

Does changing mz_strm_zlib.c at line 208 from:

while (zlib->zstream.avail_out > 0);

To:

while ((zlib->zstream.avail_out > 0)  || (flush == Z_FINISH && err == Z_OK));

Solve the issue?

[rddesmond]

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 inflate if the parent stream doesn't have any more data.

--- minizip-2.8.9/mz_strm_zlib.c
+++ new/minizip-2.8.9/mz_strm_zlib.c	2019-09-05 23:11:18.851591105 -0400
@@ -168,8 +168,8 @@
 
             if (read < 0)
                 return read;
-            if (read == 0)
-                break;
+//            if (read == 0)
+//                break;
 
             zlib->zstream.next_in = zlib->buffer;
             zlib->zstream.avail_in = read;

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]

minizip probably uses inflate a bit differently than other libraries with the flush parameter Z_SYNC_FLUSH. Other libraries might use Z_NO_FLUSH followed by Z_FINISH. In the Z_FINISH state it would finish writing all remaining bytes.

[nmoinvaz]

I have made a similar change to the bzip and lzma streams in case the problem is also there. Thanks reporting and fixing.

[rddesmond]

Thank you!

[nmoinvaz]

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 the main.cpp in the test_assert. It wouldn't return the right error number because err would be evaluated twice in a #define.

[rddesmond]

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 like Z_STREAM_END covers 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.