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

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Junio,

On 2015-06-26 00:29, Junio C Hamano wrote:
> 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 understood what you were saying, but it still appears too fragile to me to mix functions that assume NUL-terminated strings with an ad-hoc counted string check. Take `skip_prefix()` for example: it really assumes implicitly that the string is NUL-terminated because a first argument that is too short for the prefix will simply compare NUL against non-NUL at some stage. With your idea, we will now assume that all the usages of `skip_prefix()` in `fsck.c`, including future ones, will refrain from asking for a prefix containing `\n`. That might not hold true, and worse: it might not be detected because a couple of code paths *already* pass NUL-terminated buffers to fsck.

Now, we are actually talking about really rare objects, right? So why not be a little generous with memory in the very unlikely event that we encounter such a tag object? I am thinking somewhat along these lines:

-- snipsnap --
diff --git a/fsck.c b/fsck.c
index 2fffa43..4c7530a 100644
--- a/fsck.c
+++ b/fsck.c
@@ -238,10 +238,11 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 	return retval;
 }
 
-static int require_end_of_header(const void *data, unsigned long size,
+static int require_end_of_header(const char **data, unsigned long size,
 	struct object *obj, fsck_error error_func)
 {
-	const char *buffer = (const char *)data;
+	static char *nul_terminated;
+	const char *buffer = *(const char **)data;
 	unsigned long i;
 
 	for (i = 0; i < size; i++) {
@@ -255,7 +256,14 @@ static int require_end_of_header(const void *data, unsigned long size,
 		}
 	}
 
-	return error_func(obj, FSCK_ERROR, "unterminated header");
+	/* TODO: when fsck-opt hits `next`, test whether to error out here */
+	if (nul_terminated)
+		free(nul_terminated);
+	nul_terminated = xmalloc(size + 1);
+	memcpy(nul_terminated, *data, size);
+	nul_terminated[size] = '\0';
+	*data = nul_terminated;
+	return 0;
 }
 
 static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)
@@ -297,15 +304,16 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f
 	return 0;
 }
 
-static int fsck_commit_buffer(struct commit *commit, const char *buffer,
+static int fsck_commit_buffer(struct commit *commit, const char *orig,
 	unsigned long size, fsck_error error_func)
 {
+	const char *buffer = orig;
 	unsigned char tree_sha1[20], sha1[20];
 	struct commit_graft *graft;
 	unsigned parent_count, parent_line_count = 0;
 	int err;
 
-	if (require_end_of_header(buffer, size, &commit->object, error_func))
+	if (require_end_of_header(&buffer, size, &commit->object, error_func))
 		return -1;
 
 	if (!skip_prefix(buffer, "tree ", &buffer))
@@ -356,9 +364,10 @@ static int fsck_commit(struct commit *commit, const char *data,
 	return ret;
 }
 
-static int fsck_tag_buffer(struct tag *tag, const char *data,
+static int fsck_tag_buffer(struct tag *tag, const char *orig,
 	unsigned long size, fsck_error error_func)
 {
+	const char *data = orig;
 	unsigned char sha1[20];
 	int ret = 0;
 	const char *buffer;
@@ -384,7 +393,7 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,
 		}
 	}
 
-	if (require_end_of_header(buffer, size, &tag->object, error_func))
+	if (require_end_of_header(&buffer, size, &tag->object, error_func))
 		goto done;
 
 	if (!skip_prefix(buffer, "object ", &buffer)) {