[PATCH 3/6] git_connect: use argv_array
- From
Jeff King <peff@peff.net>
- Date
- May 15, 2014, 08:34 UTC
- Message-ID
- <20140515083409.GC26866@sigill.intra.peff.net>
- In-Reply-To
- <20140515082943.GA26473@sigill.intra.peff.net>
This avoids magic numbers when we allocate fixed-size argv arrays, and makes it more obvious that we are not overflowing.
It is also the first step to fixing a memory leak. When git_connect returns a child_process struct, the argv array in the struct is dynamically allocated, but the individual strings are not (they are either owned elsewhere, or are freed). Later, in finish_connect, we free the array but leave the strings alone.
This works for the child_process created by git_connect, but if we use transport_take_over, we may also end up with a child_process created by transport-helper's get_helper. In that case, the strings are freshly allocated, and we would want to free them. However, we have no idea in finish_connect which type we have.
By consistently using run-command's internal argv-array, we do not have to worry about this issue at all; finish_command takes care of it for us, and we can drop our manual free entirely.
Note that this actually makes the get_helper leak slightly worse; now we are leaking both the strings and the array. But when we adjust it in a future patch, that leak will go away entirely.
Signed-off-by: Jeff King <peff@peff.net> --- connect.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/connect.c b/connect.c index a983d06..94a6650 100644 --- a/connect.c +++ b/connect.c @@ -534,22 +534,18 @@ 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; struct child_process *proxy; get_host_and_port(&host, &port); - argv = xmalloc(sizeof(*argv) * 4); - argv[0] = git_proxy_command; - argv[1] = host; - argv[2] = port; - argv[3] = NULL; proxy = xcalloc(1, sizeof(*proxy)); - proxy->argv = argv; + argv_array_push(&proxy->args, git_proxy_command); + argv_array_push(&proxy->args, host); + argv_array_push(&proxy->args, port); proxy->in = -1; proxy->out = -1; if (start_command(proxy)) - die("cannot start proxy %s", argv[0]); + die("cannot start proxy %s", git_proxy_command); fd[0] = proxy->out; /* read from proxy stdout */ fd[1] = proxy->in; /* write to proxy stdin */ return proxy; @@ -663,7 +659,6 @@ struct child_process *git_connect(int fd[2], const char *url, char *hostandport, *path; struct child_process *conn = &no_fork; enum protocol protocol; - const char **arg; struct strbuf cmd = STRBUF_INIT; /* Without this we cannot rely on waitpid() to tell @@ -707,7 +702,6 @@ struct child_process *git_connect(int fd[2], const char *url, sq_quote_buf(&cmd, path); conn->in = conn->out = -1; - conn->argv = arg = xcalloc(7, sizeof(*arg)); if (protocol == PROTO_SSH) { const char *ssh = getenv("GIT_SSH"); int putty = ssh && strcasestr(ssh, "plink"); @@ -718,22 +712,21 @@ struct child_process *git_connect(int fd[2], const char *url, if (!ssh) ssh = "ssh"; - *arg++ = ssh; + argv_array_push(&conn->args, ssh); if (putty && !strcasestr(ssh, "tortoiseplink")) - *arg++ = "-batch"; + argv_array_push(&conn->args, "-batch"); if (port) { /* P is for PuTTY, p is for OpenSSH */ - *arg++ = putty ? "-P" : "-p"; - *arg++ = port; + argv_array_push(&conn->args, putty ? "-P" : "-p"); + argv_array_push(&conn->args, port); } - *arg++ = ssh_host; + argv_array_push(&conn->args, ssh_host); } else { /* remove repo-local variables from the environment */ conn->env = local_repo_env; conn->use_shell = 1; } - *arg++ = cmd.buf; - *arg = NULL; + argv_array_push(&conn->args, cmd.buf); if (start_command(conn)) die("unable to fork"); @@ -759,7 +752,6 @@ int finish_connect(struct child_process *conn) return 0; code = finish_command(conn); - free(conn->argv); free(conn); return code; }
-- 2.0.0.rc1.436.g03cb729