Re: git hang with corrupted .pack

From

Junio C Hamano <gitster@pobox.com>

Date

Oct 20, 2009, 19:33 UTC

Message-ID

<7vpr8hn9ly.fsf@alter.siamese.dyndns.org>

In-Reply-To

<7vzl7mng35.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> 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.
>
> After thinking about this a bit more, I am reasonably sure that this is
> it.  The contents does not have to be a 0-length string, but you would hit
> this if the pure-data portion of the deflated stream aligns at the end of
> your (un)pack window and it happens to require another use_pack() to move
> the window to read the end-of-stream signal.  In that situation, the
> output buffer has already been filled, but you haven't read the input
> stream fully.  Would't the new check incorrectly trigger in such a case?
>
>>>  		st = git_inflate(&stream, Z_FINISH);
>>> +		if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))
>>> +			break;
>
> We won't see this on 64-bit platforms because we use larger (un)pack
> window and the condition is much less likely to be met.

Perhaps it would be as simple as this?

 sha1_file.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4cc8939..8c9f392 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1344,7 +1344,7 @@ unsigned long get_size_from_delta(struct packed_git *p,
 			          off_t curpos)
 {
 	const unsigned char *data;
-	unsigned char delta_head[20], *in;
+	unsigned char delta_head[21], *in;
 	z_stream stream;
 	int st;
 
@@ -1357,13 +1357,14 @@ unsigned long get_size_from_delta(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;
+		if (!stream.avail_out)
+			break; /* the payload is larger than it should be! */
 		curpos += stream.next_in - in;
 	} while ((st == Z_OK || st == Z_BUF_ERROR) &&
-		 stream.total_out < sizeof(delta_head));
+		 stream.total_out < (sizeof(delta_head) - 1));
 	git_inflate_end(&stream);
-	if ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head)) {
+	if ((st != Z_STREAM_END) &&
+	    stream.total_out != sizeof(delta_head) - 1) {
 		error("delta data unpack-initial failed");
 		return 0;
 	}
@@ -1589,15 +1590,15 @@ static void *unpack_compressed_entry(struct packed_git *p,
 	buffer[size] = 0;
 	memset(&stream, 0, sizeof(stream));
 	stream.next_out = buffer;
-	stream.avail_out = size;
+	stream.avail_out = size + 1;
 
 	git_inflate_init(&stream);
 	do {
 		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;
+		if (!stream.avail_out)
+			break; /* the payload is larger than it should be! */
 		curpos += stream.next_in - in;
 	} while (st == Z_OK || st == Z_BUF_ERROR);
 	git_inflate_end(&stream);