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

Jeff King [off-list ref] writes:

On Fri, Jun 26, 2015 at 10:06:20AM +0200, Johannes Schindelin wrote:

quoted

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.

Yeah, I agree. It is not that you cannot make it safe, but that it is
simply a fragile maintenance burden in the future. I thought we dealt
with this already with a1e920a (index-pack: terminate object buffers
with NUL, 2014-12-08), though.

Hmph, that is an interesting point.

It would mean that the require_eoh() can be reduced a bit further.

 * It is still a good idea to make sure we do not have NUL in the
   header part,

 * It can still stop scanning when it finds a blank line (i.e. we do
   not care what is in the message part of commit and tag),

 * It does not have to insist that a commit or a tag has a blank
   line to reject a header-only object.

That would mean the name of the helper needs to change, though.

Perhaps like this?

 fsck.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fsck.c b/fsck.c
index 10bcb65..c23e84e 100644
--- a/fsck.c
+++ b/fsck.c

@@ -241,8 +241,8 @@ 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,
-	struct object *obj, fsck_error error_func)
+static int verify_headers(const void *data, unsigned long size,
+			  struct object *obj, fsck_error error_func)
 {
 	const char *buffer = (const char *)data;
 	unsigned long i;

@@ -257,8 +257,13 @@ static int require_end_of_header(const void *data, unsigned long size,
 				return 0;
 		}
 	}
-
-	return error_func(obj, FSCK_ERROR, "unterminated header");
+	/*
+	 * did not find a blank line -- is the last header line
+	 * correctly terminated with LF?
+	 */
+	if (size && buffer[size - 1] != '\n')
+		return error_func(obj, FSCK_ERROR, "unterminated header");
+	return 0;
 }
 
 static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)

Hi Junio,

On 2015-06-26 19:37, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Fri, Jun 26, 2015 at 10:06:20AM +0200, Johannes Schindelin wrote:

quoted

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.

Yeah, I agree. It is not that you cannot make it safe, but that it is
simply a fragile maintenance burden in the future. I thought we dealt
with this already with a1e920a (index-pack: terminate object buffers
with NUL, 2014-12-08), though.

Hmph, that is an interesting point.

It would mean that the require_eoh() can be reduced a bit further.

 * It is still a good idea to make sure we do not have NUL in the
   header part,

 * It can still stop scanning when it finds a blank line (i.e. we do
   not care what is in the message part of commit and tag),

 * It does not have to insist that a commit or a tag has a blank
   line to reject a header-only object.

That would mean the name of the helper needs to change, though.

You mean in addition to your changes to read new lines only when we're still inside the buffer? I cannot say that I like this fragility (and would prefer the aforementioned patch that simply allocates a NUL-terminated buffer in the rather unlikely event of tag/commit objects without an empty line), but then: you are stuck with maintaining this code, so it is your decision. ;-)

I will hopefully have time starting Tuesday this week to work on that patch, if nobody else beats me to it.

Ciao,
Dscho