Re: [PATCH] archive: flush deflate stream until Z_STREAM_END
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Aug 3, 2025, 15:34 UTC
- Message-ID
- <t4rkigpst2h42r7iouwli7lj74lydz6nmehojzajt6impibpui@kvgu7pfkuf6k>
- In-Reply-To
- <5817e9ed-5b5c-4539-85ea-07c601c0ac7e@web.de>
On 25/08/03 11:52AM, René Scharfe wrote:
Show 8 quoted lines
> On 8/3/25 12:08 AM, Justin Tobler wrote: > > I've tested the patch against this specific file, and it fixes the > > issue, but I'm uncertain how to reproduce and test this issue more > > generically. I'm open to suggestions if anyone has some ideas :) > > Not sure how to fill up zlib's pending buffer most efficiently. > Reducing the size of the output buffer would make the bug easier to > trigger, though.
Ya, I was able to trigger this issue more frequently by making the output buffer smaller than the input buffer. I was really hoping though to find a way to reproduce this without code changes so we could add a test. Not sure if that is really feasible though in this case.
Show 33 quoted lines
> > diff --git a/archive-zip.c b/archive-zip.c
> > index df8866d5bae..29e7c9f5e3f 100644
> > --- a/archive-zip.c
> > +++ b/archive-zip.c
> > @@ -492,14 +492,22 @@ static int write_zip_entry(struct archiver_args *args,
> >
> > zstream.next_in = buf;
> > zstream.avail_in = 0;
> > - result = git_deflate(&zstream, Z_FINISH);
> > - if (result != Z_STREAM_END)
> > - die("deflate error (%d)", result);
> > +
> > + do {
> > + result = git_deflate(&zstream, Z_FINISH);
> > + if (result != Z_OK && result != Z_STREAM_END)
> > + die("deflate error (%d)", result);
> > +
> > + out_len = zstream.next_out - compressed;
> > + if (out_len > 0) {
> > + write_or_die(1, compressed, out_len);
> > + compressed_size += out_len;
> > + zstream.next_out = compressed;
> > + zstream.avail_out = sizeof(compressed);
> > + }
> > + } while (result != Z_STREAM_END);
> >
> > git_deflate_end(&zstream);
> > - out_len = zstream.next_out - compressed;
> > - write_or_die(1, compressed, out_len);
> > - compressed_size += out_len;
>
> Looks good. Could probably rolled into the first loop, but that just
> would make this fix more complicated.I was also considering rolling this into the first loop, but ultimately went with the minimal patch to fix the issue. I don't mind rerolling if we prefer it the other way though. :)
Thanks for the review!
-Justin