Re: [BUG] serious inflate inconsistency on master

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> The problem seems to be in index-pack.c:unpack_data, which does this:
>
>>	git_inflate_init(&stream);
>>	stream.next_out = data;
>>	stream.avail_out = consume ? 64*1024 : obj->size;
>>
>>	do {
>>		unsigned char *last_out = stream.next_out;
>>		ssize_t n = (len < 64*1024) ? len : 64*1024;
>>		n = pread(pack_fd, inbuf, n, from);
>>		if (n < 0)
>>			die_errno(_("cannot pread pack file"));
>>		if (!n)
>>			die(Q_("premature end of pack file, %lu byte missing",
>>			       "premature end of pack file, %lu bytes missing",
>>			       len),
>>			    len);
>>		from += n;
>>		len -= n;
>>		stream.next_in = inbuf;
>>		stream.avail_in = n;
>>		status = git_inflate(&stream, 0);
>>		if (consume) {
>>			if (consume(last_out, stream.next_out - last_out, cb_data)) {
>>				free(inbuf);
>>				free(data);
>>				return NULL;
>>			}
>>			stream.next_out = data;
>>			stream.avail_out = 64*1024;
>>		}
>>	} while (len && status == Z_OK && !stream.avail_in);
>>
>>	/* This has been inflated OK when first encountered, so... */
>>	if (status != Z_STREAM_END || stream.total_out != obj->size)
>>		die(_("serious inflate inconsistency"));

Yeah, that "if (consume)" part is clearly bogus.  It should feed
what it read in "inbuf" fully to git_inflate() in a (missing) inner
loop and keep feeding consume() with the inflated data, but instead
happily goes on to read more from the packfile once the initial part
of the inflated data is fed to consume, ignoring the remainder.

> So I don't really understand what this !stream.avail_in is doing there
> in the do-while loop.  Don't we instead need to have an inner loop that
> keeps feeding the result of pread into git_inflate until we don't have
> any available data left?

Exactly.  I do not think the avain_in check should be done at the
end of the outer loop at all.  When we are buffering the entire
inflated data in core without using consume, we allocate enough
memory to "data" to hold the whole thing, so in that case, it may be
OK to expect that git_inflate() would never return without consuming
the input bytes (i.e. stream.avail_in would always be zero at the
site of the check), but with consume(), we give small piece of
memory as a temporary output area and call git_inflate(), and it is
entirely possible and normal for it to run out of output before
inflating all the input bytes.