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

From: Jeff King <peff@peff.net>
Date: May 17, 2011, 05:54 UTC
Message-ID: <20110517055422.GD10048@sigill.intra.peff.net>
In-Reply-To: <4DD181C6.4020104@kdbg.org>

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

Show 15 quoted lines

> > 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. ;)

Show 6 quoted lines

> >  	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.

Show 5 quoted lines

> 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;