Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes

On Mon, May 16, 2011 at 09:57:58PM +0200, Johannes Sixt wrote:

quoted

The code path for git://, on the other hand, always sets it
to no_fork. In the case of a direct TCP connection, this
makes sense; we have no child process. But in the case of a
proxy command (configured by core.gitproxy), we do have a
child process, but we throw away its pid, and therefore
ignore its return code.

Instead, let's keep that information in the proxy case, and
respect its return code, which can help catch some errors

This patch looks strikingly familiar. I had written an almost identical
change more than 3 years ago and forgot about it, though the
justification I noted in the commit was more to properly shutdown the
proxy process rather than to abandon it and let it be collected by
init(8). Your justification is much better.

Thanks, I had no idea your patch existed. I hate to duplicate work, but
at least it's a sanity check that it's not a totally stupid idea. ;)

quoted

 	const char *argv[4];
-	struct child_process proxy;
+	struct child_process *proxy;

[...]
At this point, proxy->argv would point to automatic storage; but we
need argv[0] in finish_command() for error reporting.

Ick. Good catch.

In my implementation, I xmalloced the pointer array and leaked it.
(And that's probably the reason that I never submitted the patch.) I
wouldn't dare to make argv just static because this limits us to have
just one open connection at a time established via
git_proxy_connect().  Dunno...

We also need to worry about the contents of each argv[] element, no? So
we should be xstrdup()ing the host and port, which point into some
string which gets passed to us. I didn't trace its provenance but I
think it is better to be defensive.

The leak is probably OK in a practical sense (you generally make no more
than one such connection per command), but it does seem ugly. I would
not be surprised if many other run-command invocations leak similarly.

The interdiff with the strdups is:

diff --git a/connect.c b/connect.c
index c678ceb..c24866e 100644
--- a/connect.c
+++ b/connect.c

@@ -398,14 +398,15 @@ static int git_use_proxy(const char *host)
 static struct child_process *git_proxy_connect(int fd[2], char *host)
 {
 	const char *port = STR(DEFAULT_GIT_PORT);
-	const char *argv[4];
+	const char **argv;
 	struct child_process *proxy;
 
 	get_host_and_port(&host, &port);
 
+	argv = xmalloc(4 * sizeof(*argv));
 	argv[0] = git_proxy_command;
-	argv[1] = host;
-	argv[2] = port;
+	argv[1] = xstrdup(host);
+	argv[2] = xstrdup(port);
 	argv[3] = NULL;
 	proxy = xcalloc(1, sizeof(*proxy));
 	proxy->argv = argv;