Re: Infinite loop regression in git-fsck in v2.12.0

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"John Szakmeister" <john@szakmeister.net>,
	"Dennis Kaarsemaker" <dennis@kaarsemaker.net>
Subject: Re: Infinite loop regression in git-fsck in v2.12.0
Date: Tue, 30 Oct 2018 18:56:03 -0400	[thread overview]
Message-ID: <20181030225603.GA5889@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqq4ld3134f.fsf@gitster-ct.c.googlers.com>

On Wed, Oct 31, 2018 at 07:28:00AM +0900, Junio C Hamano wrote:

> > So we need to distinguish those cases. I think this is the simplest fix:
> >
> > diff --git a/sha1-file.c b/sha1-file.c
> > index dd0b6aa873..a7ff5fe25d 100644
> > --- a/sha1-file.c
> > +++ b/sha1-file.c
> > @@ -2199,6 +2199,7 @@ static int check_stream_sha1(git_zstream *stream,
> >  	 * see the comment in unpack_sha1_rest for details.
> >  	 */
> >  	while (total_read <= size &&
> > +	       stream->avail_in > 0 &&
> >  	       (status == Z_OK || status == Z_BUF_ERROR)) {
> >  		stream->next_out = buf;
> >  		stream->avail_out = sizeof(buf);
> 
> Hmph.  If the last round consumed the final input byte and needed
> output space of N bytes, but only M (< N) bytes of the output space
> was available, then it would have reduced both avail_in and
> avail_out down to zero and yielded Z_BUF_ERROR, no?  Or would zlib
> refrain from consuming that final byte (leaving avail_in to at least
> one) and give us Z_BUF_ERROR in such a case?

Hmm, yeah, good thinking. I think zlib could consume that final byte
into its internal buffer.

As part of my digging, I looked at how the loose streaming code handles
this. It checks that when we see Z_BUF_ERROR, we actually did run out of
output bytes (so if we didn't, then we know it's not the case we
expected to be looping on).

I have some patches almost ready to send; I'll use that technique.

-Peff