Re: git hang with corrupted .pack

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

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Z_BUF_ERROR is returned from inflate() if either the input buffer
> needs more input bytes, or the output buffer has run out of space.
> Previously we only considered the former case, as it meant we needed
> to move the stream's input buffer to the next window in the pack.
>
> We now abort the loop if inflate() returns Z_BUF_ERROR without
> consuming the entire input buffer it was given, or has filled
> the entire output buffer but has not yet returned Z_STREAM_END.
> Either state is a clear indicator that this loop is not working
> as expected, and should not continue.

When the inflated contents is of size 0, avail_out would be 0 and avail_in
would still have something because the input stream needs to have the end
of stream marker that is more than zero byte long.

If that is more than one-byte long, and your avail_in originally fed only
the first byte from that sequence, what happens?  Wouldn't inflate eat all
what was given (now avail_in is zero), updated its internal state but it
still hasn't produced anything (avail_out is zero)?

I am not saying the end-of-stream is more than one-byte long (I didn't
check), but we had a similar bug arising from confusing "no more output
data" and "fully consumed input stream" (e.g. 456cdf6 (Fix loose object
uncompression check., 2007-03-19).

Something like that may be what is happening to cause problem Alex is
seeing.

I think the corrupt packdata detection needs an output buffer at least
one-byte larger than what is needed to store the correct result.  Then
when we get BUF_ERROR:

 - We never look at avail to see if it is zero; !avail_out is not the same
   as "it stopped because it ran out of output space".  It might mean
   "there is nothing more to come but the input stream ended before
   signalling that fact to the inflate engine fully".

 - We do look at avail_out to find how much data we ended up getting.  If
   inflate has consumed more buffer than we planned to give it, the stream
   is corrupt (at least it is not what we expected to see);

>  		st = git_inflate(&stream, Z_FINISH);
> +		if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))
> +			break;
>  		curpos += stream.next_in - in;
>  	} while ((st == Z_OK || st == Z_BUF_ERROR) &&
>  		 stream.total_out < sizeof(delta_head));
> @@ -1594,6 +1596,8 @@ static void *unpack_compressed_entry(struct packed_git *p,
>  		in = use_pack(p, w_curs, curpos, &stream.avail_in);
>  		stream.next_in = in;
>  		st = git_inflate(&stream, Z_FINISH);
> +		if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))
> +			break;
>  		curpos += stream.next_in - in;
>  	} while (st == Z_OK || st == Z_BUF_ERROR);
>  	git_inflate_end(&stream);
> -- 
> 1.6.5.52.g0ff2e
>
> -- 
> Shawn.