{"thread":{"id":"63895","subject":"[PATCH] archive: flush deflate stream until Z_STREAM_END","startedAt":"2025-08-02T22:14:08Z","lastAt":"2025-08-04T17:01:05Z","messageCount":4,"participants":["Justin Tobler","René Scharfe","Toon Claes"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"523297","messageId":"20250802220803.95137-1-jltobler@gmail.com","threadId":"63895","inReplyTo":null,"subject":"[PATCH] archive: flush deflate stream until Z_STREAM_END","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-02T22:08:03Z","receivedAt":"2025-08-02T22:14:08Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"In `archive-zip.c:write_zip_entry()` when using a stream as input for\ndeflating a file, the call to `git_deflate()` with Z_FINISH always\nexpects Z_STREAM_END to be returned. Per zlib documentation[1]:\n\n        If the parameter flush is set to Z_FINISH, pending input is\n        processed, pending output is flushed and deflate returns with\n        Z_STREAM_END if there was enough output space. If deflate\n        returns with Z_OK or Z_BUF_ERROR, this function must be called\n        again with Z_FINISH and more output space (updated avail_out)\n        but no more input data, until it returns with Z_STREAM_END or an\n        error. After deflate has returned Z_STREAM_END, the only\n        possible operations on the stream are deflateReset or\n        deflateEnd.\n\nIn scenarios where the output buffer is not large enough to write all\nthe compressed data, it is perfectly valid for the underlying\n`deflate()` to return Z_OK. Thus, expecting a single pass of `deflate()`\nhere to always return Z_STREAM_END is a bug. Update the code to flush\nthe deflate stream until Z_STREAM_END is returned.\n\n[1]: https://zlib.net/manual.html\n\nHelped-by: Toon Claes <toon@iotcl.com>\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n\nGreetings,\n\nAt GitLab, we received a report of a user getting the following error\nwhen generating a zip archive of their repository via git-archive(1):\n\n        fatal: deflate error (0)\n\nI've so far only been able to reproduce this issue in the chromium.git\nrepository with a specific file:\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                ui/events/ozone/evdev/touch_filter/palm_model/onedevice_train_palm_detection_filter_inference.cc\n\nIn the above example, `core.bigFileThreshold` is set to a low value to\ncause more files to use a stream as input while being deflated. This is\nthe codepath that produces the specific error.\n\nI've tested the patch against this specific file, and it fixes the\nissue, but I'm uncertain how to reproduce and test this issue more\ngenerically. I'm open to suggestions if anyone has some ideas :)\n\nThanks,\n-Justin\n\n---\n archive-zip.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex df8866d5bae..29e7c9f5e3f 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -492,14 +492,22 @@ static int write_zip_entry(struct archiver_args *args,\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\tdo {\n+\t\t\tresult = git_deflate(&zstream, Z_FINISH);\n+\t\t\tif (result != Z_OK && result != Z_STREAM_END)\n+\t\t\t\tdie(\"deflate error (%d)\", result);\n+\n+\t\t\tout_len = zstream.next_out - compressed;\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+\t\t} while (result != Z_STREAM_END);\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-- \n2.50.1.214.ga30f80fde9\n\n"},{"id":"523330","messageId":"5817e9ed-5b5c-4539-85ea-07c601c0ac7e@web.de","threadId":"63895","inReplyTo":"20250802220803.95137-1-jltobler@gmail.com","subject":"Re: [PATCH] archive: flush deflate stream until Z_STREAM_END","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-08-03T09:52:19Z","receivedAt":"2025-08-03T09:52:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 8/3/25 12:08 AM, Justin Tobler wrote:\n> In `archive-zip.c:write_zip_entry()` when using a stream as input for\n> deflating a file, the call to `git_deflate()` with Z_FINISH always\n> expects Z_STREAM_END to be returned. Per zlib documentation[1]:\n> \n>         If the parameter flush is set to Z_FINISH, pending input is\n>         processed, pending output is flushed and deflate returns with\n>         Z_STREAM_END if there was enough output space. If deflate\n>         returns with Z_OK or Z_BUF_ERROR, this function must be called\n>         again with Z_FINISH and more output space (updated avail_out)\n>         but no more input data, until it returns with Z_STREAM_END or an\n>         error. After deflate has returned Z_STREAM_END, the only\n>         possible operations on the stream are deflateReset or\n>         deflateEnd.\n> \n> In scenarios where the output buffer is not large enough to write all\n> the compressed data, it is perfectly valid for the underlying\n> `deflate()` to return Z_OK. Thus, expecting a single pass of `deflate()`\n> here to always return Z_STREAM_END is a bug. Update the code to flush\n> the deflate stream until Z_STREAM_END is returned.\n> \n> [1]: https://zlib.net/manual.html\n\nGood find.  I guess back then I thought making the output buffer twice\nas big as the input buffer was sufficient, as deflateBound() guarantees\ncompression is possible with a much lower ratio.  But this doesn't take\nthe internal state of a stream into account.  Oof!\n\n> Helped-by: Toon Claes <toon@iotcl.com>\n> Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> ---\n> \n> Greetings,\n> \n> At GitLab, we received a report of a user getting the following error\n> when generating a zip archive of their repository via git-archive(1):\n> \n>         fatal: deflate error (0)\n> \n> I've so far only been able to reproduce this issue in the chromium.git\n> repository with a specific file:\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>                 ui/events/ozone/evdev/touch_filter/palm_model/onedevice_train_palm_detection_filter_inference.cc\n> \n> In the above example, `core.bigFileThreshold` is set to a low value to\n> cause more files to use a stream as input while being deflated. This is\n> the codepath that produces the specific error.\n> \n> I've tested the patch against this specific file, and it fixes the\n> issue, but I'm uncertain how to reproduce and test this issue more\n> generically. I'm open to suggestions if anyone has some ideas :)\n\nNot sure how to fill up zlib's pending buffer most efficiently.\nReducing the size of the output buffer would make the bug easier to\ntrigger, though.\n\n> Thanks,\n> -Justin\n> \n> ---\n>  archive-zip.c | 20 ++++++++++++++------\n>  1 file changed, 14 insertions(+), 6 deletions(-)\n> \n> diff --git a/archive-zip.c b/archive-zip.c\n> index df8866d5bae..29e7c9f5e3f 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -492,14 +492,22 @@ static int write_zip_entry(struct archiver_args *args,\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\tdo {\n> +\t\t\tresult = git_deflate(&zstream, Z_FINISH);\n> +\t\t\tif (result != Z_OK && result != Z_STREAM_END)\n> +\t\t\t\tdie(\"deflate error (%d)\", result);\n> +\n> +\t\t\tout_len = zstream.next_out - compressed;\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> +\t\t} while (result != Z_STREAM_END);\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\nLooks good.  Could probably rolled into the first loop, but that just\nwould make this fix more complicated.\n\n>  \t\tzip_offset += compressed_size;\n>  \n>  \t\twrite_zip_data_desc(size, compressed_size, crc);\n\n"},{"id":"523343","messageId":"t4rkigpst2h42r7iouwli7lj74lydz6nmehojzajt6impibpui@kvgu7pfkuf6k","threadId":"63895","inReplyTo":"5817e9ed-5b5c-4539-85ea-07c601c0ac7e@web.de","subject":"Re: [PATCH] archive: flush deflate stream until Z_STREAM_END","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-08-03T15:34:13Z","receivedAt":"2025-08-03T15:34:15Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/08/03 11:52AM, René Scharfe wrote:\n> On 8/3/25 12:08 AM, Justin Tobler wrote:\n> > I've tested the patch against this specific file, and it fixes the\n> > issue, but I'm uncertain how to reproduce and test this issue more\n> > generically. I'm open to suggestions if anyone has some ideas :)\n> \n> Not sure how to fill up zlib's pending buffer most efficiently.\n> Reducing the size of the output buffer would make the bug easier to\n> trigger, though.\n\nYa, I was able to trigger this issue more frequently by making the\noutput buffer smaller than the input buffer. I was really hoping though\nto find a way to reproduce this without code changes so we could add a\ntest. Not sure if that is really feasible though in this case.\n\n> > diff --git a/archive-zip.c b/archive-zip.c\n> > index df8866d5bae..29e7c9f5e3f 100644\n> > --- a/archive-zip.c\n> > +++ b/archive-zip.c\n> > @@ -492,14 +492,22 @@ static int write_zip_entry(struct archiver_args *args,\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\tdo {\n> > +\t\t\tresult = git_deflate(&zstream, Z_FINISH);\n> > +\t\t\tif (result != Z_OK && result != Z_STREAM_END)\n> > +\t\t\t\tdie(\"deflate error (%d)\", result);\n> > +\n> > +\t\t\tout_len = zstream.next_out - compressed;\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> > +\t\t} while (result != Z_STREAM_END);\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> \n> Looks good.  Could probably rolled into the first loop, but that just\n> would make this fix more complicated.\n\nI was also considering rolling this into the first loop, but ultimately\nwent with the minimal patch to fix the issue. I don't mind rerolling if\nwe prefer it the other way though. :)\n\nThanks for the review!\n\n-Justin\n"},{"id":"523467","messageId":"87cy9buim2.fsf@iotcl.com","threadId":"63895","inReplyTo":"t4rkigpst2h42r7iouwli7lj74lydz6nmehojzajt6impibpui@kvgu7pfkuf6k","subject":"Re: [PATCH] archive: flush deflate stream until Z_STREAM_END","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-08-04T17:00:53Z","receivedAt":"2025-08-04T17:01:05Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> On 25/08/03 11:52AM, René Scharfe wrote:\n>> On 8/3/25 12:08 AM, Justin Tobler wrote:\n>> > I've tested the patch against this specific file, and it fixes the\n>> > issue, but I'm uncertain how to reproduce and test this issue more\n>> > generically. I'm open to suggestions if anyone has some ideas :)\n>> \n>> Not sure how to fill up zlib's pending buffer most efficiently.\n>> Reducing the size of the output buffer would make the bug easier to\n>> trigger, though.\n\nYes, in my local testing I also have been reducing the size of the\noutput buffer.\n\n> Ya, I was able to trigger this issue more frequently by making the\n> output buffer smaller than the input buffer. I was really hoping though\n> to find a way to reproduce this without code changes so we could add a\n> test. Not sure if that is really feasible though in this case.\n\nI think it's really nice you were able find a repository that reproduces\nthis issue. Having an existing example in the \"wild\" is better proof to\nverify this fix work.\n\n>> > I've so far only been able to reproduce this issue in the chromium.git\n>> > repository with a specific file:\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>> >                 ui/events/ozone/evdev/touch_filter/palm_model/onedevice_train_palm_detection_filter_inference.cc\n\nWeird, for me this file didn't trigger the error, but the following file\ndid:\n\n    chrome/test/data/third_party/kraken/tests/kraken-1.1/imaging-darkroom-data.js\n\nBut also with this file, I can confirm this fix works.\n\n>> > diff --git a/archive-zip.c b/archive-zip.c\n>> > index df8866d5bae..29e7c9f5e3f 100644\n>> > --- a/archive-zip.c\n>> > +++ b/archive-zip.c\n>> > @@ -492,14 +492,22 @@ static int write_zip_entry(struct archiver_args *args,\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\tdo {\n>> > +\t\t\tresult = git_deflate(&zstream, Z_FINISH);\n>> > +\t\t\tif (result != Z_OK && result != Z_STREAM_END)\n>> > +\t\t\t\tdie(\"deflate error (%d)\", result);\n>> > +\n>> > +\t\t\tout_len = zstream.next_out - compressed;\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>> > +\t\t} while (result != Z_STREAM_END);\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>> \n>> Looks good.  Could probably rolled into the first loop, but that just\n>> would make this fix more complicated.\n>\n> I was also considering rolling this into the first loop, but ultimately\n> went with the minimal patch to fix the issue. I don't mind rerolling if\n> we prefer it the other way though. :)\n\nWhen I was researching, I have been working on a version that rolls the\nchanges into the first loop, but that diff is a lot more substantial.\nThe thing is, you cannot seem to avoid introducing another (nested)\nloop.\n\nMy refactoring is based on the zlib example page[1] page and example\nsource code[2], and also there two nested loops are used. Ideally, we'd\nmodify our code so it follows the example code more closely, but I'm\nworried this might introduce new breakage, so it's probably not worth\nit.\n\nOn the other hand, I was able to trigger failure when I made the output\nbuffer equally large as the input buffer. In this case git-archive\ncompletes successfully, but when inflated, the content is mangled.\nIt seems my version is protected against that.\n\nI've submitted my version too[3]. I'm on the fence which solution is\nbetter, so I'd love to get input from the both of you. So I'm happy to\nhear any thoughts.\n\n[1]: https://zlib.net/zlib_how.html\n[2]: https://zlib.net/zpipe.c\n[3]: https://lore.kernel.org/git/20250804-toon-archive-zip-fix-v1-0-ca89858e5eaa@iotcl.com/\n\n-- \nCheers,\nToon\n"}]}