Re: [PATCH 1/3] validate_headref:...

Re: [PATCH 1/3] validate_headref: NUL-terminate HEAD buffer

From: Junio C Hamano <hidden>
Date: 2017-09-27 07:06:29

Jeff King [off-list ref] writes:

quoted hunk

diff --git a/path.c b/path.c
index b533ec938d..3e4d7505ef 100644
--- a/path.c
+++ b/path.c

@@ -662,6 +662,10 @@ int validate_headref(const char *path)
 	len = read_in_full(fd, buffer, sizeof(buffer)-1);
 	close(fd);
 
+	if (len < 0)
+		return -1;
+	buffer[len] = '\0';
+
 	/*
 	 * Is it a symbolic ref?
 	 */

A few tangents I noticed:

 - the result of readlink should be checked with starts_with() in
   the modern codebase (#leftoverbits).

 - buffer[256] would mean that we cannot have a branch whose name is
   more than a couple of hundred bytes long; as you said, we may be
   better off using strbuf_read to read the whole thing.

Neither should be touched by this patch, of course.

Thanks.

Re: [PATCH 1/3] validate_headref: NUL-terminate HEAD buffer

From: Jeff King <hidden>
Date: 2017-09-27 07:16:32

On Wed, Sep 27, 2017 at 04:06:22PM +0900, Junio C Hamano wrote:

A few tangents I noticed:

 - the result of readlink should be checked with starts_with() in
   the modern codebase (#leftoverbits).

Yes, though it needs to NUL-terminate, too (readlink does not do so
automatically). Again, we seem to have left room for the NUL but didn't
bother to put it there.

 - buffer[256] would mean that we cannot have a branch whose name is
   more than a couple of hundred bytes long; as you said, we may be
   better off using strbuf_read to read the whole thing.

Heh, I almost talked about this in the cover letter, but didn't want to
go off on a tangent. But since you mention it...

I had the same concern, but actually truncation is not a problem here
(for a symlink or a symref). We are only seeing if the contents look
vaguely correct, so really we never parse past "refs/" in either case.

The real symref resolution happens in refs/files-backend.c these days,
and does use a strbuf.

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help