Re: git hang with corrupted .pack
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 20, 2009, 20:50 UTC
- Message-ID
- <7vaazln61u.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <alpine.LFD.2.00.0910201538180.21460@xanadu.home>
Nicolas Pitre <nico@fluxnic.net> writes:
Show 8 quoted lines
> 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.
Sounds good. Here is a reroll with a bit more explanation.
-- >8 -- From: Junio C Hamano <gitster@pobox.com> Date: Tue, 20 Oct 2009 12:40:02 -0700 Subject: [PATCH] Fix "corrupt input stream" check while reading from packfiles
An ealier "fix" made us break out of the loop when we get Z_BUF_ERROR back from inflate(), and either the input stream still had some data to consume, or we have already got the full output we expected.
This is the same kind of mistake as we corrected with 456cdf6 (Fix loose object uncompression check., 2007-03-19); it is valid for inflate() to produce full output before it consumes the input stream fully; e.g. immediately before reading the end of stream marker.
Instead, detect corrupt input stream by feeding the input as long as inflate() wants to without detecting a real error, and giving it an output buffer that is one byte longer than necessary. If it touches the extra byte, we know that the input stream is corrupt; otherwise inflate() will notice the broken input stream by itself.
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- sha1_file.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c index 4cc8939..f0907b8 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1344,7 +1344,8 @@ 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; + unsigned long expected_size = sizeof(delta_head) - 1; z_stream stream; int st; @@ -1357,13 +1358,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 < expected_size); git_inflate_end(&stream); - if ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head)) { + if ((st != Z_STREAM_END) && + stream.total_out != expected_size) { error("delta data unpack-initial failed"); return 0; } @@ -1589,15 +1591,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);
-- 1.6.5.1.107.gba912