Re: [PATCH v10 0/4] cat-file: add support for "-allow-unknown-type"

From: Junio C Hamano <gitster@pobox.com>
Date: May 4, 2015, 20:57 UTC
Message-ID: <xmqqioc8rrg0.fsf@gitster.dls.corp.google.com>
In-Reply-To: <55477478.9010406@gmail.com>

karthik nayak <karthik.188@gmail.com> writes:

Show 8 quoted lines

>> @@ -1579,7 +1578,6 @@ static int unpack_sha1_heade
>>   	if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
>>   		return 0;
>>
>> -	strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);
> Ok this works, weird, I test before sending patches, anyways getting
> to the point of discussion, shouldn't we have add the buf, since we
> did inflate once, before doing inflate again?

+static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map, + unsigned long mapsize, void *buffer, + unsigned long bufsiz, struct strbuf *header) +{ + unsigned char *cp; + int status; + + status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);

Here, buffer..stream->next_out contains the inflated result

+ /* + * Check if entire header is unpacked in the first iteration. + */ + if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer)) + return 0; + + strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);

... and that is copied to header.buf ...

+ do { + status = git_inflate(stream, 0);

... and then we inflate further into where?

It inflates to stream->next_out and then advances the pointer

+		strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);

... and then the same buffer..stream->next_out (whose early part already holds the result from the initial inflation) is appended to header.

+ if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer)) + return 0; + stream->next_out = buffer; + stream->avail_out = bufsiz;

I think my squash is wrong. The initial inflate must have filled buffer[0..bufsiz] without any NULs, leaving stream->next_out at the end of the buffer, and subsequent git_inflate() it will clobber beyond the tail of the buffer.

It should have been more like this, I think.

diff --git a/sha1_file.c b/sha1_file.c
index f65bf90..37e6f2c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1568,7 +1568,6 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
 					unsigned long mapsize, void *buffer,
 					unsigned long bufsiz, struct strbuf *header)
 {
-	unsigned char *cp;
 	int status;
 
 	status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
@@ -1579,7 +1578,15 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
 	if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
 		return 0;
 
+	/*
+	 * buffer[0..bufsiz] was not large enough.  Copy the partial
+	 * result out to header, and then append the result of further
+	 * reading the stream.
+	 */
 	strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);
+	stream->next_out = buffer;
+	stream->avail_out = bufsiz;
+
 	do {
 		status = git_inflate(stream, 0);
 		strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);