{"thread":{"id":"63910","subject":"[PATCH 0/4] Fix archiving some corner-case files into zip","startedAt":"2025-08-04T16:56:52Z","lastAt":"2025-08-07T19:56:39Z","messageCount":9,"participants":["Toon Claes","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"523462","messageId":"20250804-toon-archive-zip-fix-v1-0-ca89858e5eaa@iotcl.com","threadId":"63910","inReplyTo":null,"subject":"[PATCH 0/4] Fix archiving some corner-case files into zip","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-08-04T16:56:31Z","receivedAt":"2025-08-04T16:56:52Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"At $DAYJOB, we've add a customer report an issue where they failed to\ndownload a zip archive from a repository. The error they saw come from\ngit-archive(1) is:\n\n fatal: deflate error (0)\n\nMy friendly colleague Justin Tobler was able to reproduce this issue[1].\nWe've diagnosed this error happens on some files that exceed\ncore.bigFileThreshold. To reproduce the issue, you can run:\n\n git clone --depth=1 https://github.com/chromium/chromium.git\n cd chromium\n git -c core.bigFileThreshold=1 archive -o foo.zip --format=zip HEAD -- \\\n chrome/test/data/third_party/kraken/tests/kraken-1.1/imaging-darkroom-data.js\n\n(originally he mentioned another file, but that didn't trigger the bug\nfor me)\n\nAnd a patch to fix the issue was presented that message.\n\nI have tested the fix, and I can confirm this fixes the issue. But I'm\nconcerned this doesn't fix all issues.\n\nAnother way one could trigger the issue, is by initializing\n`unsigned char compressed` with length `STREAM_BUFFER_SIZE / 2` (so half\nthe length of the input buffer, instead of double).\n\nWith Justin's fix, you see the error doesn't happen no more. But it\nseems, the resulting zip archive isn't valid. When I try to unzip it, I\nsee:\n\n inflating: chrome/test/data/third_party/kraken/tests/kraken-1.1/imaging-darkroom-data.js bad CRC 3ba68a86 (should be b09a04a2)\n\nAnd when the length is set to `STREAM_BUFFER_SIZE` (so equal length to\ninput buffer), the decompress goes well, but the data seems to be\nmangled.\n\nThis is because only the final call of git_deflate() is being wrapped in\na loop for the current chunk of input data. We can see in various other\ncallsites in the Git codebase, git_deflate() is usually called in a\n`while` loop (even when the `flush` parameter is set to `0` =\nZ_NO_FLUSH).\n\nFor the record, I want to give all the credit to Justin for diagnosing\nthis bug and to determine a solution. Where he aims to provide a fix\nthat is minimal, I wanted to present an alternative solution that\nimplements zlib usuage according to the official usage example[2], but\nthe changes are more substantial.\n\nI'm on the fence which of two is the better approach. Because the ZIP\nformat has a End Of Central Directory record (EOCD) at the end, it's far\nmore likely *only* the final git_deflate() call suffers from unprocessed\ninput data, so the final Justin provides probably Just Works. I'm gonna\nleave it up to the community to decide what is \"better\"?\n\n[1]: https://lore.kernel.org/git/20250802220803.95137-1-jltobler@gmail.com/\n[2]: https://zlib.net/zlib_how.html\n[3]: https://en.wikipedia.org/wiki/ZIP_(file_format)#End_of_central_directory_record_(EOCD)\n\n--\nCheers,\nToon\n\n---\nToon Claes (4):\n archive-zip: deduplicate code setting output buffer in write_zip_entry()\n archive-zip: remove unneccesarry condition in write_zip_entry()\n archive-zip: in write_zip_entry() call git_deflate() in a loop\n archive-zip: move git_deflate() with Z_FINISH into the loop\n\n archive-zip.c | 40 +++++++++++++++-------------------------\n 1 file changed, 15 insertions(+), 25 deletions(-)\n---\n\n\n\n---\n\nbase-commit: e813a0200a7121b97fec535f0d0b460b0a33356c\nchange-id: 20250801-toon-archive-zip-fix-2deac42d5aa3\n\nThanks\n--\nToon\n\n"},{"id":"523463","messageId":"20250804-toon-archive-zip-fix-v1-1-ca89858e5eaa@iotcl.com","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-0-ca89858e5eaa@iotcl.com","subject":"[PATCH 1/4] archive-zip: deduplicate code setting output buffer in write_zip_entry()","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-08-04T16:56:32Z","receivedAt":"2025-08-04T16:56:56Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"There were two callsites setting the size and address of the output\nbuffer. Instead of setting them outside the loop and in the loop after\ncalling git_deflate(). Set them once in the loop, right before the\ngit_deflate() call.\n\nCo-authored-by: Justin Tobler \nSigned-off-by: Toon Claes \n---\n archive-zip.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex df8866d5ba..cc6d0cadd9 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -458,8 +458,6 @@ static int write_zip_entry(struct archiver_args *args,\n \t\tgit_deflate_init_raw(&zstream, args->compression_level);\n \n \t\tcompressed_size = 0;\n-\t\tzstream.next_out = compressed;\n-\t\tzstream.avail_out = sizeof(compressed);\n \n \t\tfor (;;) {\n \t\t\treadlen = read_istream(stream, buf, sizeof(buf));\n@@ -473,6 +471,8 @@ static int write_zip_entry(struct archiver_args *args,\n \n \t\t\tzstream.next_in = buf;\n \t\t\tzstream.avail_in = readlen;\n+\t\t\tzstream.next_out = compressed;\n+\t\t\tzstream.avail_out = sizeof(compressed);\n \t\t\tresult = git_deflate(&zstream, 0);\n \t\t\tif (result != Z_OK)\n \t\t\t\tdie(_(\"deflate error (%d)\"), result);\n@@ -481,8 +481,6 @@ static int write_zip_entry(struct archiver_args *args,\n \t\t\tif (out_len > 0) {\n \t\t\t\twrite_or_die(1, compressed, out_len);\n \t\t\t\tcompressed_size += out_len;\n-\t\t\t\tzstream.next_out = compressed;\n-\t\t\t\tzstream.avail_out = sizeof(compressed);\n \t\t\t}\n \n \t\t}\n\n-- \n2.50.1.327.g047016eb4a\n\n"},{"id":"523464","messageId":"20250804-toon-archive-zip-fix-v1-2-ca89858e5eaa@iotcl.com","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-0-ca89858e5eaa@iotcl.com","subject":"[PATCH 2/4] archive-zip: remove unneccesarry condition in write_zip_entry()","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-08-04T16:56:33Z","receivedAt":"2025-08-04T16:56:59Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"The function write_or_die() can handle a length that's zero, thus we can\nremove the condition that checks the value of `out_len` that surrounds\nthis call. The value shall never be negative as this would have caused\ndata being omitted in the deflated output.\n\nCo-authored-by: Justin Tobler \nSigned-off-by: Toon Claes \n---\n archive-zip.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex cc6d0cadd9..d41a12de5f 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -478,11 +478,8 @@ static int write_zip_entry(struct archiver_args *args,\n \t\t\t\tdie(_(\"deflate error (%d)\"), result);\n \t\t\tout_len = zstream.next_out - compressed;\n \n-\t\t\tif (out_len > 0) {\n-\t\t\t\twrite_or_die(1, compressed, out_len);\n-\t\t\t\tcompressed_size += out_len;\n-\t\t\t}\n-\n+\t\t\twrite_or_die(1, compressed, out_len);\n+\t\t\tcompressed_size += out_len;\n \t\t}\n \t\tclose_istream(stream);\n \t\tif (readlen)\n\n-- \n2.50.1.327.g047016eb4a\n\n"},{"id":"523465","messageId":"20250804-toon-archive-zip-fix-v1-3-ca89858e5eaa@iotcl.com","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-0-ca89858e5eaa@iotcl.com","subject":"[PATCH 3/4] archive-zip: in write_zip_entry() call git_deflate() in a loop","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-08-04T16:56:34Z","receivedAt":"2025-08-04T16:57:02Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"The function git_deflate() might not complete to deflate all the input\ndata in one go. While the function is already being called in a loop,\nevery loop fresh data is read from the stream. This is not correct,\nbecause input data might get lost.\n\nAs we see in many other callsites, git_deflate() should be called in a\nloop on the existing input to make it process all the input data.\n\nAdd in a nested loop around git_deflate() to process the input buffer\ncompletely, before continuing the parent loop that reads from more data\nfrom the input stream.\n\nCo-authored-by: Justin Tobler \nSigned-off-by: Toon Claes \n---\n archive-zip.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex d41a12de5f..25a0224130 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -471,15 +471,17 @@ static int write_zip_entry(struct archiver_args *args,\n \n \t\t\tzstream.next_in = buf;\n \t\t\tzstream.avail_in = readlen;\n-\t\t\tzstream.next_out = compressed;\n-\t\t\tzstream.avail_out = sizeof(compressed);\n-\t\t\tresult = git_deflate(&zstream, 0);\n-\t\t\tif (result != Z_OK)\n-\t\t\t\tdie(_(\"deflate error (%d)\"), result);\n-\t\t\tout_len = zstream.next_out - compressed;\n+\t\t\tdo {\n+\t\t\t\tzstream.next_out = compressed;\n+\t\t\t\tzstream.avail_out = sizeof(compressed);\n+\t\t\t\tresult = git_deflate(&zstream, 0);\n+\t\t\t\tif (result != Z_OK)\n+\t\t\t\t\tdie(_(\"deflate error (%d)\"), result);\n+\t\t\t\tout_len = zstream.next_out - compressed;\n \n-\t\t\twrite_or_die(1, compressed, out_len);\n-\t\t\tcompressed_size += out_len;\n+\t\t\t\twrite_or_die(1, compressed, out_len);\n+\t\t\t\tcompressed_size += out_len;\n+\t\t\t} while (zstream.avail_out == 0);\n \t\t}\n \t\tclose_istream(stream);\n \t\tif (readlen)\n\n-- \n2.50.1.327.g047016eb4a\n\n"},{"id":"523466","messageId":"20250804-toon-archive-zip-fix-v1-4-ca89858e5eaa@iotcl.com","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-0-ca89858e5eaa@iotcl.com","subject":"[PATCH 4/4] archive-zip: move git_deflate() with Z_FINISH into the loop","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-08-04T16:56:35Z","receivedAt":"2025-08-04T16:57:06Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Instead duplicating code to do the final deflate (with `flush` value\nZ_FINISH), bring this call inside the loop that's deflate parts of the\ninput stream. This causes also this final deflate to be wrapped in a\nloop to ensure the whole input is taken care of.\n\nThis change makes crc32() to be called without checking if the `readlen`\nis greater than zero, but looking at the zlib manual[1] should be\nallowed.\n\nThis patch concluded some refactoring, making the code more similar to\nthe example usage of the official zlib docs[2].\n\n[1]: https://zlib.net/manual.html\n[2]: https://zlib.net/zlib_how.html\n\nCo-authored-by: Justin Tobler \nSigned-off-by: Toon Claes \n---\n archive-zip.c | 25 +++++++++----------------\n 1 file changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex 25a0224130..559ed267be 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -451,7 +451,7 @@ static int write_zip_entry(struct archiver_args *args,\n \t\tunsigned char buf[STREAM_BUFFER_SIZE];\n \t\tssize_t readlen;\n \t\tgit_zstream zstream;\n-\t\tint result;\n+\t\tint result, flush;\n \t\tsize_t out_len;\n \t\tunsigned char compressed[STREAM_BUFFER_SIZE * 2];\n \n@@ -459,44 +459,37 @@ static int write_zip_entry(struct archiver_args *args,\n \n \t\tcompressed_size = 0;\n \n-\t\tfor (;;) {\n+\t\tdo {\n \t\t\treadlen = read_istream(stream, buf, sizeof(buf));\n-\t\t\tif (readlen <= 0)\n+\t\t\tif (readlen < 0)\n \t\t\t\tbreak;\n \t\t\tcrc = crc32(crc, buf, readlen);\n-\t\t\tif (is_binary == -1)\n+\t\t\tif ((is_binary == -1) && readlen)\n \t\t\t\tis_binary = entry_is_binary(args->repo->index,\n \t\t\t\t\t\t\t path_without_prefix,\n \t\t\t\t\t\t\t buf, readlen);\n \n+\t\t\tflush = readlen ? Z_NO_FLUSH : Z_FINISH;\n \t\t\tzstream.next_in = buf;\n \t\t\tzstream.avail_in = readlen;\n \t\t\tdo {\n \t\t\t\tzstream.next_out = compressed;\n \t\t\t\tzstream.avail_out = sizeof(compressed);\n-\t\t\t\tresult = git_deflate(&zstream, 0);\n-\t\t\t\tif (result != Z_OK)\n+\t\t\t\tresult = git_deflate(&zstream, flush);\n+\t\t\t\tif ((result != Z_OK) && (result != Z_STREAM_END))\n \t\t\t\t\tdie(_(\"deflate error (%d)\"), result);\n \t\t\t\tout_len = zstream.next_out - compressed;\n \n \t\t\t\twrite_or_die(1, compressed, out_len);\n \t\t\t\tcompressed_size += out_len;\n \t\t\t} while (zstream.avail_out == 0);\n-\t\t}\n+\t\t} while (flush != Z_FINISH);\n+\n \t\tclose_istream(stream);\n \t\tif (readlen)\n \t\t\treturn readlen;\n \n-\t\tzstream.next_in = buf;\n-\t\tzstream.avail_in = 0;\n-\t\tresult = git_deflate(&zstream, Z_FINISH);\n-\t\tif (result != Z_STREAM_END)\n-\t\t\tdie(\"deflate error (%d)\", result);\n-\n \t\tgit_deflate_end(&zstream);\n-\t\tout_len = zstream.next_out - compressed;\n-\t\twrite_or_die(1, compressed, out_len);\n-\t\tcompressed_size += out_len;\n \t\tzip_offset += compressed_size;\n \n \t\twrite_zip_data_desc(size, compressed_size, crc);\n\n-- \n2.50.1.327.g047016eb4a\n\n"},{"id":"523769","messageId":"7650da07-dd89-4e79-ac81-1c99f4700d9e@web.de","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-2-ca89858e5eaa@iotcl.com","subject":"Re: [PATCH 2/4] archive-zip: remove unneccesarry condition in write_zip_entry()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-07T19:51:17Z","receivedAt":"2025-08-07T19:51:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/4/25 6:56 PM, Toon Claes wrote:\n> The function write_or_die() can handle a length that's zero, thus we can\n> remove the condition that checks the value of `out_len` that surrounds\n> this call. The value shall never be negative as this would have caused\n> data being omitted in the deflated output.\n> \n> Co-authored-by: Justin Tobler \n> Signed-off-by: Toon Claes \n> ---\n> archive-zip.c | 7 ++-----\n> 1 file changed, 2 insertions(+), 5 deletions(-)\n> \n> diff --git a/archive-zip.c b/archive-zip.c\n> index cc6d0cadd9..d41a12de5f 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -478,11 +478,8 @@ static int write_zip_entry(struct archiver_args *args,\n> \t\t\t\tdie(_(\"deflate error (%d)\"), result);\n> \t\t\tout_len = zstream.next_out - compressed;\n> \n> -\t\t\tif (out_len > 0) {\n> -\t\t\t\twrite_or_die(1, compressed, out_len);\n> -\t\t\t\tcompressed_size += out_len;\n> -\t\t\t}\n> -\n> +\t\t\twrite_or_die(1, compressed, out_len);\n> +\t\t\tcompressed_size += out_len;\n> \t\t}\n> \t\tclose_istream(stream);\n> \t\tif (readlen)\n> \n\nOK, less lines again, great! write_or_die() handles out_len == 0\njust fine; this won't increase the number of syscalls we make.\n\nRené\n\n"},{"id":"523770","messageId":"2285e892-5177-42ef-b80d-552e4e39b80d@web.de","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-4-ca89858e5eaa@iotcl.com","subject":"Re: [PATCH 4/4] archive-zip: move git_deflate() with Z_FINISH into the loop","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-07T19:51:22Z","receivedAt":"2025-08-07T19:51:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/4/25 6:56 PM, Toon Claes wrote:\n> Instead duplicating code to do the final deflate (with `flush` value\n> Z_FINISH), bring this call inside the loop that's deflate parts of the\n> input stream. This causes also this final deflate to be wrapped in a\n> loop to ensure the whole input is taken care of.\n> \n> This change makes crc32() to be called without checking if the `readlen`\n> is greater than zero, but looking at the zlib manual[1] should be\n> allowed.\n\nReading the manual is surely allowed, that's what it was written for.\nSnarkiness aside, s/should/it should/\n\nBut if you're referring to the example with a zero length there:\n\n\tuLong crc = crc32(0L, Z_NULL, 0);\n\n... then note that it's also giving a Z_NULL pointer, which crc32_z()\n(https://github.com/madler/zlib/blob/develop/crc32.c#L585) has special\nhandling for; the length is ignored in this example.\n\n> \n> This patch concluded some refactoring, making the code more similar to\n> the example usage of the official zlib docs[2].\n> \n> [1]: https://zlib.net/manual.html\n> [2]: https://zlib.net/zlib_how.html\n> \n> Co-authored-by: Justin Tobler \n> Signed-off-by: Toon Claes \n> ---\n> archive-zip.c | 25 +++++++++----------------\n> 1 file changed, 9 insertions(+), 16 deletions(-)\n> \n> diff --git a/archive-zip.c b/archive-zip.c\n> index 25a0224130..559ed267be 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -451,7 +451,7 @@ static int write_zip_entry(struct archiver_args *args,\n> \t\tunsigned char buf[STREAM_BUFFER_SIZE];\n> \t\tssize_t readlen;\n> \t\tgit_zstream zstream;\n> -\t\tint result;\n> +\t\tint result, flush;\n> \t\tsize_t out_len;\n> \t\tunsigned char compressed[STREAM_BUFFER_SIZE * 2];\n> \n> @@ -459,44 +459,37 @@ static int write_zip_entry(struct archiver_args *args,\n> \n> \t\tcompressed_size = 0;\n> \n> -\t\tfor (;;) {\n> +\t\tdo {\n> \t\t\treadlen = read_istream(stream, buf, sizeof(buf));\n> -\t\t\tif (readlen <= 0)\n> +\t\t\tif (readlen < 0)\n> \t\t\t\tbreak;\n> \t\t\tcrc = crc32(crc, buf, readlen);\n> -\t\t\tif (is_binary == -1)\n> +\t\t\tif ((is_binary == -1) && readlen)\n\nIt's probably fine to call crc32 with zero length, but since you have the\nreadlen condition here anyway it would basically be free to extend it to\nthat call as well.\n\n> \t\t\t\tis_binary = entry_is_binary(args->repo->index,\n> \t\t\t\t\t\t\t path_without_prefix,\n> \t\t\t\t\t\t\t buf, readlen);\n> \n> +\t\t\tflush = readlen ? Z_NO_FLUSH : Z_FINISH;\n\n... and setting Z_FINISH could be moved under there as well to avoid\nsetting Z_NO_FLUSH over and over. I'm probably nitpicking here, though.\n\n> \t\t\tzstream.next_in = buf;\n> \t\t\tzstream.avail_in = readlen;\n> \t\t\tdo {\n> \t\t\t\tzstream.next_out = compressed;\n> \t\t\t\tzstream.avail_out = sizeof(compressed);\n> -\t\t\t\tresult = git_deflate(&zstream, 0);\n> -\t\t\t\tif (result != Z_OK)\n> +\t\t\t\tresult = git_deflate(&zstream, flush);\n> +\t\t\t\tif ((result != Z_OK) && (result != Z_STREAM_END))\n> \t\t\t\t\tdie(_(\"deflate error (%d)\"), result);\n> \t\t\t\tout_len = zstream.next_out - compressed;\n> \n> \t\t\t\twrite_or_die(1, compressed, out_len);\n> \t\t\t\tcompressed_size += out_len;\n> \t\t\t} while (zstream.avail_out == 0);\n\nHmm, the manual says \"After deflate has returned Z_STREAM_END, the only\npossible operations on the stream are deflateReset or deflateEnd.\".\nHere we could have deflate fill the output buffer to the brim, be done\nand return Z_STREAM_END and we'd go again, which would result in an\nerror, I suppose. So the while loop above needs to end on Z_STREAM_END\nas well, no?\n\nRené\n\n\n> -\t\t}\n> +\t\t} while (flush != Z_FINISH);\n> +\n> \t\tclose_istream(stream);\n> \t\tif (readlen)\n> \t\t\treturn readlen;\n> \n> -\t\tzstream.next_in = buf;\n> -\t\tzstream.avail_in = 0;\n> -\t\tresult = git_deflate(&zstream, Z_FINISH);\n> -\t\tif (result != Z_STREAM_END)\n> -\t\t\tdie(\"deflate error (%d)\", result);\n> -\n> \t\tgit_deflate_end(&zstream);\n> -\t\tout_len = zstream.next_out - compressed;\n> -\t\twrite_or_die(1, compressed, out_len);\n> -\t\tcompressed_size += out_len;\n> \t\tzip_offset += compressed_size;\n> \n> \t\twrite_zip_data_desc(size, compressed_size, crc);\n> \n\n\n"},{"id":"523771","messageId":"3858f1ef-7afc-471e-ae21-99b4587bafed@web.de","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-1-ca89858e5eaa@iotcl.com","subject":"Re: [PATCH 1/4] archive-zip: deduplicate code setting output buffer in write_zip_entry()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-07T19:51:10Z","receivedAt":"2025-08-07T19:56:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/4/25 6:56 PM, Toon Claes wrote:\n> There were two callsites setting the size and address of the output\n> buffer. Instead of setting them outside the loop and in the loop after\n> calling git_deflate(). Set them once in the loop, right before the\n> git_deflate() call.\n> \n> Co-authored-by: Justin Tobler \n> Signed-off-by: Toon Claes \n> ---\n> archive-zip.c | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/archive-zip.c b/archive-zip.c\n> index df8866d5ba..cc6d0cadd9 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -458,8 +458,6 @@ static int write_zip_entry(struct archiver_args *args,\n> \t\tgit_deflate_init_raw(&zstream, args->compression_level);\n> \n> \t\tcompressed_size = 0;\n> -\t\tzstream.next_out = compressed;\n> -\t\tzstream.avail_out = sizeof(compressed);\n> \n> \t\tfor (;;) {\n> \t\t\treadlen = read_istream(stream, buf, sizeof(buf));\n> @@ -473,6 +471,8 @@ static int write_zip_entry(struct archiver_args *args,\n> \n> \t\t\tzstream.next_in = buf;\n> \t\t\tzstream.avail_in = readlen;\n> +\t\t\tzstream.next_out = compressed;\n> +\t\t\tzstream.avail_out = sizeof(compressed);\n> \t\t\tresult = git_deflate(&zstream, 0);\n> \t\t\tif (result != Z_OK)\n> \t\t\t\tdie(_(\"deflate error (%d)\"), result);\n> @@ -481,8 +481,6 @@ static int write_zip_entry(struct archiver_args *args,\n> \t\t\tif (out_len > 0) {\n> \t\t\t\twrite_or_die(1, compressed, out_len);\n> \t\t\t\tcompressed_size += out_len;\n> -\t\t\t\tzstream.next_out = compressed;\n> -\t\t\t\tzstream.avail_out = sizeof(compressed);\n> \t\t\t}\n> \n> \t\t}\n> \n\nLess lines, great! This works because for out_len == 0 these\nassignments are noops, and that condition so unlikely to be true that\nwe don't need to worry about the then unnecessary writes.\n\nRené\n\n"},{"id":"523772","messageId":"da21879f-5a73-41e1-9fc9-1ab128d8717d@web.de","threadId":"63910","inReplyTo":"20250804-toon-archive-zip-fix-v1-3-ca89858e5eaa@iotcl.com","subject":"Re: [PATCH 3/4] archive-zip: in write_zip_entry() call git_deflate() in a loop","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-07T19:51:21Z","receivedAt":"2025-08-07T19:56:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/4/25 6:56 PM, Toon Claes wrote:\n> The function git_deflate() might not complete to deflate all the input\n> data in one go. While the function is already being called in a loop,\n> every loop fresh data is read from the stream. This is not correct,\n> because input data might get lost.\n> \n> As we see in many other callsites, git_deflate() should be called in a\n> loop on the existing input to make it process all the input data.\n> \n> Add in a nested loop around git_deflate() to process the input buffer\n> completely, before continuing the parent loop that reads from more data\n> from the input stream.\n> \n> Co-authored-by: Justin Tobler \n> Signed-off-by: Toon Claes \n> ---\n> archive-zip.c | 18 ++++++++++--------\n> 1 file changed, 10 insertions(+), 8 deletions(-)\n> \n> diff --git a/archive-zip.c b/archive-zip.c\n> index d41a12de5f..25a0224130 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -471,15 +471,17 @@ static int write_zip_entry(struct archiver_args *args,\n> \n> \t\t\tzstream.next_in = buf;\n> \t\t\tzstream.avail_in = readlen;\n> -\t\t\tzstream.next_out = compressed;\n> -\t\t\tzstream.avail_out = sizeof(compressed);\n> -\t\t\tresult = git_deflate(&zstream, 0);\n> -\t\t\tif (result != Z_OK)\n> -\t\t\t\tdie(_(\"deflate error (%d)\"), result);\n> -\t\t\tout_len = zstream.next_out - compressed;\n> +\t\t\tdo {\n> +\t\t\t\tzstream.next_out = compressed;\n> +\t\t\t\tzstream.avail_out = sizeof(compressed);\n> +\t\t\t\tresult = git_deflate(&zstream, 0);\n> +\t\t\t\tif (result != Z_OK)\n> +\t\t\t\t\tdie(_(\"deflate error (%d)\"), result);\n> +\t\t\t\tout_len = zstream.next_out - compressed;\n> \n> -\t\t\twrite_or_die(1, compressed, out_len);\n> -\t\t\tcompressed_size += out_len;\n> +\t\t\t\twrite_or_die(1, compressed, out_len);\n> +\t\t\t\tcompressed_size += out_len;\n> +\t\t\t} while (zstream.avail_out == 0);\n> \t\t}\n> \t\tclose_istream(stream);\n> \t\tif (readlen)\n> \n\nMakes sense. If deflate somehow fills the output buffer (with\ninternally pending data, I suppose -- the fresh input data alone is not\nenough), this clears it and lets it go another round without feeding new\ninput.\n\nThe mistake was thinking that the existence of deflateBound(), which\ngives a maximum deflated size for any given input, implies a similarly\ntight limit on individual chunks of input, which makes no sense in\nhindsight.\n\nRené\n\n"}]}