patch, 4 partsFix archiving some corner-case files into zip

At $DAYJOB, we've add a customer report an issue where they failed to download a zip archive from a repository. The error they saw come from git-archive(1) is:

        fatal: deflate error (0)

My friendly colleague Justin Tobler was able to reproduce this issue[1]. We've diagnosed this error happens on some files that exceed core.bigFileThreshold. To reproduce the issue, you can run:

        git clone --depth=1 https://github.com/chromium/chromium.git
        cd chromium
        git -c core.bigFileThreshold=1 archive -o foo.zip --format=zip HEAD -- \
                chrome/test/data/third_party/kraken/tests/kraken-1.1/imaging-darkroom-data.js

(originally he mentioned another file, but that didn't trigger the bug for me)

And a patch to fix the issue was presented that message.

I have tested the fix, and I can confirm this fixes the issue. But I'm concerned this doesn't fix all issues.

Another way one could trigger the issue, is by initializing `unsigned char compressed` with length `STREAM_BUFFER_SIZE / 2` (so half the length of the input buffer, instead of double).

With Justin's fix, you see the error doesn't happen no more. But it seems, the resulting zip archive isn't valid. When I try to unzip it, I see:

    inflating: chrome/test/data/third_party/kraken/tests/kraken-1.1/imaging-darkroom-data.js   bad CRC 3ba68a86  (should be b09a04a2)

And when the length is set to `STREAM_BUFFER_SIZE` (so equal length to input buffer), the decompress goes well, but the data seems to be mangled.

This is because only the final call of git_deflate() is being wrapped in a loop for the current chunk of input data. We can see in various other callsites in the Git codebase, git_deflate() is usually called in a `while` loop (even when the `flush` parameter is set to `0` = Z_NO_FLUSH).

For the record, I want to give all the credit to Justin for diagnosing this bug and to determine a solution. Where he aims to provide a fix that is minimal, I wanted to present an alternative solution that implements zlib usuage according to the official usage example[2], but the changes are more substantial.

I'm on the fence which of two is the better approach. Because the ZIP format has a End Of Central Directory record (EOCD) at the end, it's far more likely *only* the final git_deflate() call suffers from unprocessed input data, so the final Justin provides probably Just Works. I'm gonna leave it up to the community to decide what is "better"?

[1]: https://lore.kernel.org/git/20250802220803.95137-1-jltobler@gmail.com/ [2]: https://zlib.net/zlib_how.html [3]: https://en.wikipedia.org/wiki/ZIP_(file_format)#End_of_central_directory_record_(EOCD)

-- Cheers, Toon

---
Toon Claes (4):
      archive-zip: deduplicate code setting output buffer in write_zip_entry()
      archive-zip: remove unneccesarry condition in write_zip_entry()
      archive-zip: in write_zip_entry() call git_deflate() in a loop
      archive-zip: move git_deflate() with Z_FINISH into the loop

 archive-zip.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)
---

---

base-commit: e813a0200a7121b97fec535f0d0b460b0a33356c change-id: 20250801-toon-archive-zip-fix-2deac42d5aa3

Thanks -- Toon

There were two callsites setting the size and address of the output buffer. Instead of setting them outside the loop and in the loop after calling git_deflate(). Set them once in the loop, right before the git_deflate() call.

Co-authored-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 archive-zip.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

On 8/4/25 6:56 PM, Toon Claes wrote:

Less lines, great! This works because for out_len == 0 these assignments are noops, and that condition so unlikely to be true that we don't need to worry about the then unnecessary writes.

René

The function write_or_die() can handle a length that's zero, thus we can remove the condition that checks the value of `out_len` that surrounds this call. The value shall never be negative as this would have caused data being omitted in the deflated output.

Co-authored-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 archive-zip.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

On 8/4/25 6:56 PM, Toon Claes wrote:

OK, less lines again, great! write_or_die() handles out_len == 0 just fine; this won't increase the number of syscalls we make.

René

The function git_deflate() might not complete to deflate all the input data in one go. While the function is already being called in a loop, every loop fresh data is read from the stream. This is not correct, because input data might get lost.

As we see in many other callsites, git_deflate() should be called in a loop on the existing input to make it process all the input data.

Add in a nested loop around git_deflate() to process the input buffer completely, before continuing the parent loop that reads from more data from the input stream.

Co-authored-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 archive-zip.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

On 8/4/25 6:56 PM, Toon Claes wrote:

Makes sense. If deflate somehow fills the output buffer (with internally pending data, I suppose -- the fresh input data alone is not enough), this clears it and lets it go another round without feeding new input.

The mistake was thinking that the existence of deflateBound(), which gives a maximum deflated size for any given input, implies a similarly tight limit on individual chunks of input, which makes no sense in hindsight.

René

Instead duplicating code to do the final deflate (with `flush` value Z_FINISH), bring this call inside the loop that's deflate parts of the input stream. This causes also this final deflate to be wrapped in a loop to ensure the whole input is taken care of.

This change makes crc32() to be called without checking if the `readlen` is greater than zero, but looking at the zlib manual[1] should be allowed.

This patch concluded some refactoring, making the code more similar to the example usage of the official zlib docs[2].

[1]: https://zlib.net/manual.html [2]: https://zlib.net/zlib_how.html

Co-authored-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 archive-zip.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

On 8/4/25 6:56 PM, Toon Claes wrote:

Reading the manual is surely allowed, that's what it was written for. Snarkiness aside, s/should/it should/

But if you're referring to the example with a zero length there:

	uLong crc = crc32(0L, Z_NULL, 0);

... then note that it's also giving a Z_NULL pointer, which crc32_z() (https://github.com/madler/zlib/blob/develop/crc32.c#L585) has special handling for; the length is ignored in this example.

It's probably fine to call crc32 with zero length, but since you have the readlen condition here anyway it would basically be free to extend it to that call as well.

... and setting Z_FINISH could be moved under there as well to avoid setting Z_NO_FLUSH over and over. I'm probably nitpicking here, though.

Hmm, the manual says "After deflate has returned Z_STREAM_END, the only possible operations on the stream are deflateReset or deflateEnd.". Here we could have deflate fill the output buffer to the brim, be done and return Z_STREAM_END and we'd go again, which would result in an error, I suppose. So the while loop above needs to end on Z_STREAM_END as well, no?

René