Re: git hang with corrupted .pack

[Alex Riesen <raa.lkml@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On Thu, Oct 15, 2009 at 09:39, Junio C Hamano <gitster@pobox.com> wrote:
> Nicolas Pitre <nico@fluxnic.net> writes:
>
>> I confirm this test without the fix reproduces the infinite loop (and
>> does stall the test suite).
>
> Thanks, both of you.

I seem to have problems with this change (on Cygwin). Sometimes
accessing an object in a pack fails in unpack_compressed_entry.
When it happens, both avail_in and avail_out of the stream are 0,
and the reported status is Z_BUF_ERROR.
Output with the second attached patch:

error: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697),
avail_out=0 (was 1256)
error: *** unpack_compressed_entry failed
error: failed to read object 3296766eb5531ef051ae392114de5d75556f5613
at offset 2620741 from
.git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack
fatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted

I cannot reproduce the problem on a normal system (a 64bit, reasonably modern
Linux in my case). An attempt to use an upgraded zlib on this cygwin system was
not successful, there was an updated library, but a clean recompile didn't
change anything.

I worked the case around by allocating a bit more than uncompressed data need.
In case someone else also sees the problem, below is how. The size of the
overallocation is arbitrary.

diff --git a/sha1_file.c b/sha1_file.c
index 4cc8939..66c2519 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1585,11 +1585,11 @@ static void *unpack_compressed_entry(struct
packed_git *p,
 	z_stream stream;
 	unsigned char *buffer, *in;

-	buffer = xmalloc(size + 1);
-	buffer[size] = 0;
+	buffer = xmalloc(size + 8);
+	memset(buffer + size, 0, 8);
 	memset(&stream, 0, sizeof(stream));
 	stream.next_out = buffer;
-	stream.avail_out = size;
+	stream.avail_out = size + 8;

 	git_inflate_init(&stream);
 	do {
-- 
1.6.5.59.g000dd

The problematic repo is a little big to post (it's my cygwin-git repo),
I'll have to find hosting for it first.

[-- Attachment #2: 0001-Workaround-inflate-sometimes-failing-to-unpack-data.diff --]
[-- Type: application/octet-stream, Size: 1341 bytes --]

From 643fdf50e4343c3ce3a4b99183dba8d35a10c877 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 20 Oct 2009 16:42:26 +0200
Subject: [PATCH 1/2] Workaround inflate sometimes fails to unpack data

When it happens, zlib stream's avail_in and avail_out are 0, the returned
status is Z_BUF_ERROR. The values weren't 0 before calling inflate.

error: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697), avail_out=0 (was 1256)
error: *** unpack_compressed_entry failed
error: failed to read object 3296766eb5531ef051ae392114de5d75556f5613 at offset 2620741 from .git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack
fatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted
---
 sha1_file.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4cc8939..66c2519 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1585,11 +1585,11 @@ static void *unpack_compressed_entry(struct packed_git *p,
 	z_stream stream;
 	unsigned char *buffer, *in;
 
-	buffer = xmalloc(size + 1);
-	buffer[size] = 0;
+	buffer = xmalloc(size + 8);
+	memset(buffer + size, 0, 8);
 	memset(&stream, 0, sizeof(stream));
 	stream.next_out = buffer;
-	stream.avail_out = size;
+	stream.avail_out = size + 8;
 
 	git_inflate_init(&stream);
 	do {
-- 
1.6.5.59.g000dd


[-- Attachment #3: 0002-Debugging-strange-failure-in-zlibs-inflate.diff --]
[-- Type: application/octet-stream, Size: 1910 bytes --]

From fc5b47d953f171d7591349acad9a23d3ec683ce9 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 20 Oct 2009 15:53:33 +0200
Subject: [PATCH 2/2] Debugging strange failure in zlibs inflate

---
 sha1_file.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 66c2519..1bf240e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1594,10 +1594,24 @@ static void *unpack_compressed_entry(struct packed_git *p,
 	git_inflate_init(&stream);
 	do {
 		in = use_pack(p, w_curs, curpos, &stream.avail_in);
+		unsigned prev_ain = stream.avail_in;
+		unsigned prev_aout = stream.avail_out;
+		if (!stream.avail_in)
+			error("*** avail_in=0, inflate will fail!");
+		if (!stream.avail_out)
+			error("*** avail_out=0, inflate will fail!");
 		stream.next_in = in;
 		st = git_inflate(&stream, Z_FINISH);
 		if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))
+                {
+			error("*** inflate error: %p size=%lu, "
+			      "avail_in=%d (was %u), "
+			      "avail_out=%d (was %u)",
+			      buffer, size,
+			      stream.avail_in, prev_ain,
+			      stream.avail_out, prev_aout);
 			break;
+                }
 		curpos += stream.next_in - in;
 	} while (st == Z_OK || st == Z_BUF_ERROR);
 	git_inflate_end(&stream);
@@ -1654,6 +1668,8 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
 		delta_base_cached -= ent->size;
 	} else {
 		ret = xmemdupz(ent->data, ent->size);
+		if (!ret)
+			fprintf(stderr, "*** no memory\n");
 	}
 	*type = ent->type;
 	*base_size = ent->size;
@@ -1815,6 +1831,8 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
 	case OBJ_BLOB:
 	case OBJ_TAG:
 		data = unpack_compressed_entry(p, &w_curs, curpos, *sizep);
+		if (!data)
+			error("*** unpack_compressed_entry failed");
 		break;
 	default:
 		data = NULL;
-- 
1.6.5.59.g000dd