From: Linus Torvalds <torvalds@osdl.org>
Date: Tue, 18 Oct 2005 22:25:17 GMT
Subject: Re: [PATCH] git-daemon extra paranoia
Message-ID: <Pine.LNX.4.64.0510181517280.3369@g5.osdl.org>
In-Reply-To: <435560F7.4080006@zytor.com>



On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> This patch adds some extra paranoia to the git-daemon filename test.  In
> particular, it now rejects pathnames containing // or ending with /; it also
> adds a redundant test for pathname absoluteness (belts and suspenders.)

Hmm. The "not ending in /" is a bad test. 

Especially in light of the fact that the git-pack protocol quite by design 
tends to add a ".git" to the end as a fallback, so that a user that wants 
to specify a particular directory _without_ that fallback needs to have 
the slash at the end.

Now, git-daemon hasn't implemented that, but I think that was just a 
mistake that grew out of it not getting a lot of testing, since it wasn't 
used much. I personally use the "without the final .git" version quite 
often, because it just looks so much nicer for the user.

In fact, here's a patch that makes git-daemon allow it, and thus match the 
behaviour of the ssh transport.

The logic is simple: if the original "chdir()" fails, try another one with 
".git" appended. This is in _addition_ to doing the 'chdir(".git")' later, 
so that if you have a checked-out git repository in /home/linux-2.6.git, 
then doing a

	git pull git://host/home/linux-2.6

will on the remote end do:

	chmod("/home/linux-2.6")	// fails with ENOENT
	chmod("/home/linux-2.6.git")	// works
	chmod(".git")			// works

resulting in it ending up in /home/linux-2.6.git/.git, which is exactly 
correct, and where it wants to be.

I personally find it a nice bit of usability enhancement. You can name 
your git repositories with a ".git" suffix (which can help all kinds of 
automated tasks - like autopacking), but you don't force your users to 
care.

		Linus

---
diff --git a/daemon.c b/daemon.c
index 11fa3ed..a488512 100644
--- a/daemon.c
+++ b/daemon.c
@@ -128,8 +128,13 @@ static int upload(char *dir, int dirlen)
 	}
 
 	if (chdir(dir) < 0) {
-		logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
-		return -1;
+		int err = errno;
+		strcpy(dir + dirlen, ".git");
+		if (err != ENOENT || chdir(dir) < 0) {
+			dir[dirlen] = 0;
+			logerror("Cannot chdir('%s'): %s", dir, strerror(err));
+			return -1;
+		}
 	}
 
 	chdir(".git");
@@ -164,7 +169,12 @@ static int execute(void)
 	static char line[1000];
 	int len;
 
-	len = packet_read_line(0, line, sizeof(line));
+	/*
+	 * Make sure that we leave room for an extra ".git" at
+	 * the end of the line. Note that the packet interfaces
+	 * already guarantee that there is an ending '\0'.
+	 */
+	len = packet_read_line(0, line, sizeof(line)-4);
 
 	if (len && line[len-1] == '\n')
 		line[--len] = 0;