Re: git error in tag ...: unterminated header

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

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> ...
>> If the buffer does *not* contain an empty line, the fsck code runs the
>> danger of looking beyond the allocated memory because it uses
>> functions that assume NUL-terminated strings, while the buffer passed
>> to the fsck code is a counted string.
>> that already, but not all of them.
> ...
> I do not think you necessarily need a NUL.  As you said, your input
> is a counted string, so you know the length of the buffer.  And you
> are verifying line by line.  As long as you make sure the buffer
> ends with "\n" (instead of saying "it has "\n\n" somewhere),
> updating the existing code that does ...
> to do this instead: ...
> shouldn't be rocket science, no?

Here is to illustrate what I meant by the above.

I did only the "tag" side (this is on 'maint'), because I did not
want to conflict with a larger change to fsck you have on 'pu'.

I made it use an updated version of require_end_of_header(), which I
named prescan_headers(), as the new sanity check does not require
\n\n as the only end of header, and also the sanity check (both old
and new) also checks for NUL.

I didn't assess how much more involved to make fsck-commit-buffer to
use prescan-headers (and retire require-end-of-header), though.

 fsck.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/fsck.c b/fsck.c
index 10bcb65..7d2d07d 100644
--- a/fsck.c
+++ b/fsck.c
@@ -261,6 +261,29 @@ static int require_end_of_header(const void *data, unsigned long size,
 	return error_func(obj, FSCK_ERROR, "unterminated header");
 }
 
+static int prescan_headers(const void *data, unsigned long size,
+			   struct object *obj, fsck_error error_func)
+{
+	const char *buffer = (const char *)data;
+	unsigned long i;
+
+	for (i = 0; i < size; i++) {
+		switch (buffer[i]) {
+		case '\0':
+			return error_func(obj, FSCK_ERROR,
+					  "unterminated header: NUL at offset %d", i);
+		case '\n':
+			if (i + 1 < size && buffer[i + 1] == '\n')
+				return 0; /* end of header */
+		}
+	}
+
+	/* no blank (i.e. headers and nothing else */
+	if (size && buffer[size - 1] == '\n')
+		return 0;
+	return error_func(obj, FSCK_ERROR, "unterminated header");
+}
+
 static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)
 {
 	char *end;
@@ -365,6 +388,7 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 	unsigned char sha1[20];
 	int ret = 0;
 	const char *buffer;
+	const char *end_buffer;
 	char *to_free = NULL, *eol;
 	struct strbuf sb = STRBUF_INIT;
 
@@ -387,10 +411,12 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 		}
 	}
 
-	if (require_end_of_header(buffer, size, &tag->object, error_func))
+	end_buffer = buffer + size;
+	if (prescan_headers(buffer, size, &tag->object, error_func))
 		goto done;
 
-	if (!skip_prefix(buffer, "object ", &buffer)) {
+	if (end_buffer <= buffer ||
+	    !skip_prefix(buffer, "object ", &buffer)) {
 		ret = error_func(&tag->object, FSCK_ERROR, "invalid format - expected 'object' line");
 		goto done;
 	}
@@ -400,7 +426,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 	}
 	buffer += 41;
 
-	if (!skip_prefix(buffer, "type ", &buffer)) {
+	if (end_buffer <= buffer ||
+	    !skip_prefix(buffer, "type ", &buffer)) {
 		ret = error_func(&tag->object, FSCK_ERROR, "invalid format - expected 'type' line");
 		goto done;
 	}
@@ -415,7 +442,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 		goto done;
 	buffer = eol + 1;
 
-	if (!skip_prefix(buffer, "tag ", &buffer)) {
+	if (end_buffer <= buffer ||
+	    !skip_prefix(buffer, "tag ", &buffer)) {
 		ret = error_func(&tag->object, FSCK_ERROR, "invalid format - expected 'tag' line");
 		goto done;
 	}
@@ -430,7 +458,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 			   (int)(eol - buffer), buffer);
 	buffer = eol + 1;
 
-	if (!skip_prefix(buffer, "tagger ", &buffer))
+	if (end_buffer <= buffer ||
+	    !skip_prefix(buffer, "tagger ", &buffer))
 		/* early tags do not contain 'tagger' lines; warn only */
 		error_func(&tag->object, FSCK_WARN, "invalid format - expected 'tagger' line");
 	else