Re: git-push through git protocol

From: Linus Torvalds <torvalds@osdl.org>
Date: Jan 22, 2007, 00:17 UTC
Message-ID: <Pine.LNX.4.64.0701211554450.14248@woody.osdl.org>
In-Reply-To: <Pine.LNX.4.64.0701211341300.14248@woody.osdl.org>

On Sun, 21 Jan 2007, Linus Torvalds wrote:

Show 7 quoted lines

> 
>  - git-send-pack wants expects the status report, and doesn't get one. 
>    That, in turn, seems to be because it expects "out" and "in" to be 
>    different file descriptors, and with the git:// protocol they aren't 
>    (they're the same file descriptor)
> 
> This attached patch should fix the second problem. Maybe.

This actually looks like potentially a generic real problem.

It so *happens* that git_connect() is different from all the other connection types in that it returns the same socket for both input and output. All the others return separate fd's for the input and output sides, because they end up using pipes.

This is not normally a problem, since people just use "read()" and "write()" on the things. Who cares if they are the same fd or not? Nobody.

Well, almost nobody. Anybody who ends up closing the file descriptors will inevitably care. In this case it was git-send-pack (through send_pack() that happened to close the two file descriptors, but because they were actually the same one, it would end up (a) closing the same thing twice and (b) doing some operations on an already-closed fd).

I really think this should be fixed, regardless of whether we want to have git:// usable for pushing or not, because it's really conceptually a bug.

We could do it either with my stupid patch (which just makes "send_pack()" do the dup itself when they happen to be identical, and thus avoids the problem in the caller), but it might actually make sense to just make the rule be that "git_connect()" always returns separate file descriptors for the input/output cases so that nobody can ever be confused, and so that you can always close those things independently.

In fact, looking more closely, a lot of the other users of git_connect() seem to avoid it by simply leaking file descriptors (ie builtin-archive.c closes only one of the file descriptors). Others (like receive-pack.c) close both, but make sure that they always close them together, and apparently fetch-pack.c just makes sure to close them together (so the second close might cause EBADF, but in the absense of any threading or signals, you're ok).

Same goes for peek-remote: it also just closes the same fd twice, and depends on it having no bad side effects (which is true, modulo race conditions that we don't have because we're single-threaded).

But everything considered, I'd almost suggest just solving this by the trivial one-liner to make git_connect() _always_ return two separate file descriptors, so that nobody needs to do strange tests and so that we don't call "close()" on the same file descriptor twice.

So I'd suggest this simple change to "connect.c". Yeah, it adds an "unnecessary" (but cheap) system call to the git:// case, but it allows all users to stop worrying about whether the result is a single in-out file descriptor or two separate file descriptors for the in/out cases.

As shown by the send-pack example, there was actually code out there that depended on the "two separate file descriptors" case, which we get for all the normal and secure cases anyway (since they use pipe() to create the communication channels with a proxy or ssh or whatever).

So Junio, I'd suggest adding this whether you then want to add the trivial code to allow pushing over git:// too or not. One less special case to worry about.

		Linus

---

diff --git a/connect.c b/connect.c
index 66daa11..7844888 100644
--- a/connect.c
+++ b/connect.c
@@ -529,7 +529,7 @@ static void git_tcp_connect(int fd[2], char *host)
 	int sockfd = git_tcp_connect_sock(host);
 
 	fd[0] = sockfd;
-	fd[1] = sockfd;
+	fd[1] = dup(sockfd);
 }