Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
prepare_{git,shell}_cmd: use argv_array
These functions transform an existing argv into one suitable
for exec-ing or spawning via git or a shell. We can use an
argv_array in each to avoid dealing with manual counting and
allocation.

This also makes the memory allocation more clear and fixes
some leaks. In prepare_shell_cmd, we would sometimes
allocate a new string with "$@" in it and sometimes not,
meaning the caller could not correctly free it. On the
non-Windows side, we are in a child process which will
exec() or exit() immediately, so the leak isn't a big deal.
On Windows, though, we use spawn() from the parent process,
and leak a string for each shell command we run. On top of
that, the Windows code did not free the allocated argv array
at all (but does for the prepare_git_cmd case!).

By switching both of these functions to write into an
argv_array, we can consistently free the result as
appropriate.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
peff authored and gitster committed Feb 22, 2016
1 parent 50a6c8e commit 20574f5
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 53 deletions.
28 changes: 11 additions & 17 deletions exec_cmd.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "exec_cmd.h"
#include "quote.h"
#include "argv-array.h"
#define MAX_ARGS 32

static const char *argv_exec_path;
Expand Down Expand Up @@ -107,32 +108,25 @@ void setup_path(void)
strbuf_release(&new_path);
}

const char **prepare_git_cmd(const char **argv)
const char **prepare_git_cmd(struct argv_array *out, const char **argv)
{
int argc;
const char **nargv;

for (argc = 0; argv[argc]; argc++)
; /* just counting */
nargv = xmalloc(sizeof(*nargv) * (argc + 2));

nargv[0] = "git";
for (argc = 0; argv[argc]; argc++)
nargv[argc + 1] = argv[argc];
nargv[argc + 1] = NULL;
return nargv;
argv_array_push(out, "git");
argv_array_pushv(out, argv);
return out->argv;
}

int execv_git_cmd(const char **argv) {
const char **nargv = prepare_git_cmd(argv);
trace_argv_printf(nargv, "trace: exec:");
struct argv_array nargv = ARGV_ARRAY_INIT;

prepare_git_cmd(&nargv, argv);
trace_argv_printf(nargv.argv, "trace: exec:");

/* execvp() can only ever return if it fails */
sane_execvp("git", (char **)nargv);
sane_execvp("git", (char **)nargv.argv);

trace_printf("trace: exec failed: %s\n", strerror(errno));

free(nargv);
argv_array_clear(&nargv);
return -1;
}

Expand Down
4 changes: 3 additions & 1 deletion exec_cmd.h
@@ -1,11 +1,13 @@
#ifndef GIT_EXEC_CMD_H
#define GIT_EXEC_CMD_H

struct argv_array;

extern void git_set_argv_exec_path(const char *exec_path);
extern const char *git_extract_argv0_path(const char *path);
extern const char *git_exec_path(void);
extern void setup_path(void);
extern const char **prepare_git_cmd(const char **argv);
extern const char **prepare_git_cmd(struct argv_array *out, const char **argv);
extern int execv_git_cmd(const char **argv); /* NULL terminated */
LAST_ARG_MUST_BE_NULL
extern int execl_git_cmd(const char *cmd, ...);
Expand Down
60 changes: 25 additions & 35 deletions run-command.c
Expand Up @@ -158,50 +158,41 @@ int sane_execvp(const char *file, char * const argv[])
return -1;
}

static const char **prepare_shell_cmd(const char **argv)
static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
{
int argc, nargc = 0;
const char **nargv;

for (argc = 0; argv[argc]; argc++)
; /* just counting */
/* +1 for NULL, +3 for "sh -c" plus extra $0 */
nargv = xmalloc(sizeof(*nargv) * (argc + 1 + 3));

if (argc < 1)
if (!argv[0])
die("BUG: shell command is empty");

if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
#ifndef GIT_WINDOWS_NATIVE
nargv[nargc++] = SHELL_PATH;
argv_array_push(out, SHELL_PATH);
#else
nargv[nargc++] = "sh";
argv_array_push(out, "sh");
#endif
nargv[nargc++] = "-c";

if (argc < 2)
nargv[nargc++] = argv[0];
else {
struct strbuf arg0 = STRBUF_INIT;
strbuf_addf(&arg0, "%s \"$@\"", argv[0]);
nargv[nargc++] = strbuf_detach(&arg0, NULL);
}
}
argv_array_push(out, "-c");

for (argc = 0; argv[argc]; argc++)
nargv[nargc++] = argv[argc];
nargv[nargc] = NULL;
/*
* If we have no extra arguments, we do not even need to
* bother with the "$@" magic.
*/
if (!argv[1])
argv_array_push(out, argv[0]);
else
argv_array_pushf(out, "%s \"$@\"", argv[0]);
}

return nargv;
argv_array_pushv(out, argv);
return out->argv;
}

#ifndef GIT_WINDOWS_NATIVE
static int execv_shell_cmd(const char **argv)
{
const char **nargv = prepare_shell_cmd(argv);
trace_argv_printf(nargv, "trace: exec:");
sane_execvp(nargv[0], (char **)nargv);
free(nargv);
struct argv_array nargv = ARGV_ARRAY_INIT;
prepare_shell_cmd(&nargv, argv);
trace_argv_printf(nargv.argv, "trace: exec:");
sane_execvp(nargv.argv[0], (char **)nargv.argv);
argv_array_clear(&nargv);
return -1;
}
#endif
Expand Down Expand Up @@ -455,6 +446,7 @@ int start_command(struct child_process *cmd)
{
int fhin = 0, fhout = 1, fherr = 2;
const char **sargv = cmd->argv;
struct argv_array nargv = ARGV_ARRAY_INIT;

if (cmd->no_stdin)
fhin = open("/dev/null", O_RDWR);
Expand All @@ -480,9 +472,9 @@ int start_command(struct child_process *cmd)
fhout = dup(cmd->out);

if (cmd->git_cmd)
cmd->argv = prepare_git_cmd(cmd->argv);
cmd->argv = prepare_git_cmd(&nargv, cmd->argv);
else if (cmd->use_shell)
cmd->argv = prepare_shell_cmd(cmd->argv);
cmd->argv = prepare_shell_cmd(&nargv, cmd->argv);

cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, (char**) cmd->env,
cmd->dir, fhin, fhout, fherr);
Expand All @@ -492,9 +484,7 @@ int start_command(struct child_process *cmd)
if (cmd->clean_on_exit && cmd->pid >= 0)
mark_child_for_cleanup(cmd->pid);

if (cmd->git_cmd)
free(cmd->argv);

argv_array_clear(&nargv);
cmd->argv = sargv;
if (fhin != 0)
close(fhin);
Expand Down

0 comments on commit 20574f5

Please sign in to comment.