Re: [PATCH 4/4] archive-zip: move git_deflate() with Z_FINISH into the loop
- From
René Scharfe <l.s.r@web.de>
- Date
- Aug 7, 2025, 19:51 UTC
- Message-ID
- <2285e892-5177-42ef-b80d-552e4e39b80d@web.de>
- In-Reply-To
- <20250804-toon-archive-zip-fix-v1-4-ca89858e5eaa@iotcl.com>
On 8/4/25 6:56 PM, Toon Claes wrote:
Show 8 quoted lines
> 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.
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.
Show 39 quoted lines
>
> 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(-)
>
> diff --git a/archive-zip.c b/archive-zip.c
> index 25a0224130..559ed267be 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -451,7 +451,7 @@ static int write_zip_entry(struct archiver_args *args,
> unsigned char buf[STREAM_BUFFER_SIZE];
> ssize_t readlen;
> git_zstream zstream;
> - int result;
> + int result, flush;
> size_t out_len;
> unsigned char compressed[STREAM_BUFFER_SIZE * 2];
>
> @@ -459,44 +459,37 @@ static int write_zip_entry(struct archiver_args *args,
>
> compressed_size = 0;
>
> - for (;;) {
> + do {
> readlen = read_istream(stream, buf, sizeof(buf));
> - if (readlen <= 0)
> + if (readlen < 0)
> break;
> crc = crc32(crc, buf, readlen);
> - if (is_binary == -1)
> + if ((is_binary == -1) && readlen)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.
Show 5 quoted lines
> is_binary = entry_is_binary(args->repo->index, > path_without_prefix, > buf, readlen); > > + flush = readlen ? Z_NO_FLUSH : Z_FINISH;
... 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.
Show 15 quoted lines
> zstream.next_in = buf;
> zstream.avail_in = readlen;
> do {
> zstream.next_out = compressed;
> zstream.avail_out = sizeof(compressed);
> - result = git_deflate(&zstream, 0);
> - if (result != Z_OK)
> + result = git_deflate(&zstream, flush);
> + if ((result != Z_OK) && (result != Z_STREAM_END))
> die(_("deflate error (%d)"), result);
> out_len = zstream.next_out - compressed;
>
> write_or_die(1, compressed, out_len);
> compressed_size += out_len;
> } while (zstream.avail_out == 0);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é
Show 21 quoted lines
> - }
> + } while (flush != Z_FINISH);
> +
> close_istream(stream);
> if (readlen)
> return readlen;
>
> - zstream.next_in = buf;
> - zstream.avail_in = 0;
> - result = git_deflate(&zstream, Z_FINISH);
> - if (result != Z_STREAM_END)
> - die("deflate error (%d)", result);
> -
> git_deflate_end(&zstream);
> - out_len = zstream.next_out - compressed;
> - write_or_die(1, compressed, out_len);
> - compressed_size += out_len;
> zip_offset += compressed_size;
>
> write_zip_data_desc(size, compressed_size, crc);
>