Re: git hang with corrupted .pack

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2009, 16:52 UTC
Message-ID: <7viqeaovmp.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20091014142351.GI9261@spearce.org>

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

Show 10 quoted lines

> 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);

Show 20 quoted lines

>  		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.