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

[Junio C Hamano <gitster@pobox.com>]

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

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