Re: git hang with corrupted .pack

Junio C Hamano [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

quoted

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?

quoted

quoted

 		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?

We deliberately give one byte more than what we expect to see and error
out when we do get that extra byte filled.

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

On Tue, 20 Oct 2009, Junio C Hamano wrote:

I didn't spend the time needed to think about this issue and your 
proposed fix yet.  However I think that using sizeof(delta_head)-1 
makes the code a bit confusing.  At this point i'd use:

	int size = sizeof(delta_head) - 1;

and use that variable instead just like it is done in 
unpack_compressed_entry() to have the same code pattern.


Nicolas