From ac78663b0da0a5b0ad1b87cfe70eecebc0c8a68d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 29 Dec 2015 03:12:22 -0500 Subject: run-command: don't warn on SIGPIPE deaths When git executes a sub-command, we print a warning if the command dies due to a signal, but make an exception for "uninteresting" cases like SIGINT and SIGQUIT (since the user presumably just hit ^C). We should make a similar exception for SIGPIPE, because it's an expected and uninteresting return in most cases; it generally means the user quit the pager before git had finished generating all output. This used to be very hard to trigger in practice, because: 1. We only complain if we see a real SIGPIPE death, not the shell-induced 141 exit code. This means that anything we run via the shell does not trigger the warning, which includes most non-trivial aliases. 2. The common case for SIGPIPE is the user quitting the pager before git has finished generating all output. But if the user triggers a pager with "-p", we redirect the git wrapper's stderr to that pager, too. Since the pager is dead, it means that the message goes nowhere. 3. You can see it if you run your own pager, like "git foo | head". But that only happens if "foo" is a non-builtin (so it doesn't work with "log", for example). However, it may become more common after 86d26f2, which teaches alias to re-exec builtins rather than running them in the same process. This case doesn't trigger (1), as we don't need a shell to run a git command. It doesn't trigger (2), because the pager is not started by the original git, but by the inner re-exec of git. And it doesn't trigger (3), because builtins are treated more like non-builtins in this case. Given how flaky this message already is (e.g., you cannot even know whether you will see it, as git optimizes out some shell invocations behind the scenes based on the contents of the command!), and that it is unlikely to ever provide useful information, let's suppress it for all cases of SIGPIPE. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- run-command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index 0d01671c1f..a573ab88d0 100644 --- a/run-command.c +++ b/run-command.c @@ -236,7 +236,7 @@ static int wait_or_whine(pid_t pid, const char *argv0) error("waitpid is confused (%s)", argv0); } else if (WIFSIGNALED(status)) { code = WTERMSIG(status); - if (code != SIGINT && code != SIGQUIT) + if (code != SIGINT && code != SIGQUIT && code != SIGPIPE) error("%s died of signal %d", argv0, code); /* * This return value is chosen so that code & 0xff -- cgit v1.3-5-g9baa From 20574f551bcc5fcf0f0e20236af174754fa11363 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 22 Feb 2016 17:44:39 -0500 Subject: 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 Signed-off-by: Junio C Hamano --- exec_cmd.c | 28 +++++++++++----------------- exec_cmd.h | 4 +++- run-command.c | 60 +++++++++++++++++++++++++---------------------------------- 3 files changed, 39 insertions(+), 53 deletions(-) (limited to 'run-command.c') diff --git a/exec_cmd.c b/exec_cmd.c index e85f0fd8d8..cf442a97f8 100644 --- a/exec_cmd.c +++ b/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; @@ -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; } diff --git a/exec_cmd.h b/exec_cmd.h index 93b0c02529..1f6b43378b 100644 --- a/exec_cmd.h +++ b/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, ...); diff --git a/run-command.c b/run-command.c index 13fa452e8c..171cbaa944 100644 --- a/run-command.c +++ b/run-command.c @@ -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 @@ -455,6 +446,7 @@ fail_pipe: { 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); @@ -480,9 +472,9 @@ fail_pipe: 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); @@ -492,9 +484,7 @@ fail_pipe: 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); -- cgit v1.3-5-g9baa From 9658846ce3d379b9ff8010a2ed326fcafc10eb82 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 24 Feb 2016 02:40:16 -0500 Subject: write_or_die: handle EPIPE in async threads When write_or_die() sees EPIPE, it treats it specially by converting it into a SIGPIPE death. We obviously cannot ignore it, as the write has failed and the caller expects us to die. But likewise, we cannot just call die(), because printing any message at all would be a nuisance during normal operations. However, this is a problem if write_or_die() is called from a thread. Our raised signal ends up killing the whole process, when logically we just need to kill the thread (after all, if we are ignoring SIGPIPE, there is good reason to think that the main thread is expecting to handle it). Inside an async thread, the die() code already does the right thing, because we use our custom die_async() routine, which calls pthread_join(). So ideally we would piggy-back on that, and simply call: die_quietly_with_code(141); or similar. But refactoring the die code to do this is surprisingly non-trivial. The die_routines themselves handle both printing and the decision of the exit code. Every one of them would have to be modified to take new parameters for the code, and to tell us to be quiet. Instead, we can just teach write_or_die() to check for the async case and handle it specially. We do have to build an interface to abstract the async exit, but it's simple and self-contained. If we had many call-sites that wanted to do this die_quietly_with_code(), this approach wouldn't scale as well, but we don't. This is the only place where do this weird exit trick. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- run-command.c | 10 ++++++++++ run-command.h | 1 + write_or_die.c | 4 ++++ 3 files changed, 15 insertions(+) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index 13fa452e8c..3add1d66ac 100644 --- a/run-command.c +++ b/run-command.c @@ -633,6 +633,11 @@ int in_async(void) return !pthread_equal(main_thread, pthread_self()); } +void NORETURN async_exit(int code) +{ + pthread_exit((void *)(intptr_t)code); +} + #else static struct { @@ -678,6 +683,11 @@ int in_async(void) return process_is_async; } +void NORETURN async_exit(int code) +{ + exit(code); +} + #endif int start_async(struct async *async) diff --git a/run-command.h b/run-command.h index 12bb26c2a6..c0969c7695 100644 --- a/run-command.h +++ b/run-command.h @@ -121,5 +121,6 @@ struct async { int start_async(struct async *async); int finish_async(struct async *async); int in_async(void); +void NORETURN async_exit(int code); #endif diff --git a/write_or_die.c b/write_or_die.c index e7afe7a295..49e80aa222 100644 --- a/write_or_die.c +++ b/write_or_die.c @@ -1,8 +1,12 @@ #include "cache.h" +#include "run-command.h" static void check_pipe(int err) { if (err == EPIPE) { + if (in_async()) + async_exit(141); + signal(SIGPIPE, SIG_DFL); raise(SIGPIPE); /* Should never happen, but just in case... */ -- cgit v1.3-5-g9baa