From 1fd1a919ce9135a34866d5d13aed5312ea5e07f1 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Tue, 18 Apr 2017 16:17:55 -0700 Subject: t5550: use write_script to generate post-update hook The post-update hooks created in t5550-http-fetch-dumb.sh is missing the "!#/bin/sh" line which can cause issues with portability. Instead create the hook using the 'write_script' function which includes the proper "#!" line. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 87308cd..8552184 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -20,8 +20,9 @@ test_expect_success 'create http-accessible bare repository with loose objects' (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && git config core.bare true && mkdir -p hooks && - echo "exec git update-server-info" >hooks/post-update && - chmod +x hooks/post-update && + write_script "hooks/post-update" <<-\EOF && + exec git update-server-info + EOF hooks/post-update ) && git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && -- cgit v0.10.2-6-g49f6 From c2d3119d7bb2122a715d2adac1574514aeff7723 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:18 -0700 Subject: t0061: run_command executes scripts without a #! line Add a test to 't0061-run-command.sh' to ensure that run_command can continue to execute scripts which don't include a '#!' line. As shell scripts are not natively executable on Windows, we use a workaround to check "#!" when running scripts from Git. As this test requires the platform (not with Git's help) to run scripts without "#!", skipt it on Windows. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 12228b4..98c09dd 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -26,6 +26,17 @@ test_expect_success 'run_command can run a command' ' test_cmp empty err ' +test_expect_success !MINGW 'run_command can run a script without a #! line' ' + cat >hello <<-\EOF && + cat hello-script + EOF + chmod +x hello && + test-run-command run-command ./hello >actual 2>err && + + test_cmp hello-script actual && + test_cmp empty err +' + test_expect_success POSIXPERM 'run_command reports EACCES' ' cat hello-script >hello.sh && chmod -x hello.sh && -- cgit v0.10.2-6-g49f6 From 3967e25be11ab96ced71f16b9f082de270a518db Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:19 -0700 Subject: run-command: prepare command before forking According to [1] we need to only call async-signal-safe operations between fork and exec. Using malloc to build the argv array isn't async-signal-safe. In order to avoid allocation between 'fork()' and 'exec()' prepare the argv array used in the exec call prior to forking the process. [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index 574b81d..d8d1437 100644 --- a/run-command.c +++ b/run-command.c @@ -221,18 +221,6 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv) } #ifndef GIT_WINDOWS_NATIVE -static int execv_shell_cmd(const char **argv) -{ - 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 - -#ifndef GIT_WINDOWS_NATIVE static int child_notifier = -1; static void notify_parent(void) @@ -244,6 +232,21 @@ static void notify_parent(void) */ xwrite(child_notifier, "", 1); } + +static void prepare_cmd(struct argv_array *out, const struct child_process *cmd) +{ + if (!cmd->argv[0]) + die("BUG: command is empty"); + + if (cmd->git_cmd) { + argv_array_push(out, "git"); + argv_array_pushv(out, cmd->argv); + } else if (cmd->use_shell) { + prepare_shell_cmd(out, cmd->argv); + } else { + argv_array_pushv(out, cmd->argv); + } +} #endif static inline void set_cloexec(int fd) @@ -372,9 +375,13 @@ fail_pipe: #ifndef GIT_WINDOWS_NATIVE { int notify_pipe[2]; + struct argv_array argv = ARGV_ARRAY_INIT; + if (pipe(notify_pipe)) notify_pipe[0] = notify_pipe[1] = -1; + prepare_cmd(&argv, cmd); + cmd->pid = fork(); failed_errno = errno; if (!cmd->pid) { @@ -437,12 +444,9 @@ fail_pipe: unsetenv(*cmd->env); } } - if (cmd->git_cmd) - execv_git_cmd(cmd->argv); - else if (cmd->use_shell) - execv_shell_cmd(cmd->argv); - else - sane_execvp(cmd->argv[0], (char *const*) cmd->argv); + + sane_execvp(argv.argv[0], (char *const *) argv.argv); + if (errno == ENOENT) { if (!cmd->silent_exec_failure) error("cannot run %s: %s", cmd->argv[0], @@ -458,7 +462,7 @@ fail_pipe: mark_child_for_cleanup(cmd->pid, cmd); /* - * Wait for child's execvp. If the execvp succeeds (or if fork() + * Wait for child's exec. If the exec succeeds (or if fork() * failed), EOF is seen immediately by the parent. Otherwise, the * child process sends a single byte. * Note that use of this infrastructure is completely advisory, @@ -467,7 +471,7 @@ fail_pipe: close(notify_pipe[1]); if (read(notify_pipe[0], ¬ify_pipe[1], 1) == 1) { /* - * At this point we know that fork() succeeded, but execvp() + * At this point we know that fork() succeeded, but exec() * failed. Errors have been reported to our stderr. */ wait_or_whine(cmd->pid, cmd->argv[0], 0); @@ -475,6 +479,8 @@ fail_pipe: cmd->pid = -1; } close(notify_pipe[0]); + + argv_array_clear(&argv); } #else { -- cgit v0.10.2-6-g49f6 From e3a434468fecca7c14a6bef32050dfa60534fde6 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:20 -0700 Subject: run-command: use the async-signal-safe execv instead of execvp Convert the function used to exec from 'execvp()' to 'execv()' as the (p) variant of exec isn't async-signal-safe and has the potential to call malloc during the path resolution it performs. Instead we simply do the path resolution ourselves during the preparation stage prior to forking. There also don't exist any portable (p) variants which also take in an environment to use in the exec'd process. This allows easy migration to using 'execve()' in a future patch. Also, as noted in [1], in the event of an ENOEXEC the (p) variants of exec will attempt to execute the command by interpreting it with the 'sh' utility. To maintain this functionality, if 'execv()' fails with ENOEXEC, start_command will atempt to execute the command by interpreting it with 'sh'. [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/exec.html Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index d8d1437..1c7a3b6 100644 --- a/run-command.c +++ b/run-command.c @@ -238,6 +238,12 @@ static void prepare_cmd(struct argv_array *out, const struct child_process *cmd) if (!cmd->argv[0]) die("BUG: command is empty"); + /* + * Add SHELL_PATH so in the event exec fails with ENOEXEC we can + * attempt to interpret the command with 'sh'. + */ + argv_array_push(out, SHELL_PATH); + if (cmd->git_cmd) { argv_array_push(out, "git"); argv_array_pushv(out, cmd->argv); @@ -246,6 +252,20 @@ static void prepare_cmd(struct argv_array *out, const struct child_process *cmd) } else { argv_array_pushv(out, cmd->argv); } + + /* + * If there are no '/' characters in the command then perform a path + * lookup and use the resolved path as the command to exec. If there + * are no '/' characters or if the command wasn't found in the path, + * have exec attempt to invoke the command directly. + */ + if (!strchr(out->argv[1], '/')) { + char *program = locate_in_PATH(out->argv[1]); + if (program) { + free((char *)out->argv[1]); + out->argv[1] = program; + } + } } #endif @@ -445,7 +465,15 @@ fail_pipe: } } - sane_execvp(argv.argv[0], (char *const *) argv.argv); + /* + * Attempt to exec using the command and arguments starting at + * argv.argv[1]. argv.argv[0] contains SHELL_PATH which will + * be used in the event exec failed with ENOEXEC at which point + * we will try to interpret the command using 'sh'. + */ + execv(argv.argv[1], (char *const *) argv.argv + 1); + if (errno == ENOEXEC) + execv(argv.argv[0], (char *const *) argv.argv); if (errno == ENOENT) { if (!cmd->silent_exec_failure) -- cgit v0.10.2-6-g49f6 From 3a30033327337323e91dcbcb87396d3245260585 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:21 -0700 Subject: string-list: add string_list_remove function Teach string-list to be able to remove a string from a sorted 'struct string_list'. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/string-list.c b/string-list.c index 45016ad..8f7b69a 100644 --- a/string-list.c +++ b/string-list.c @@ -67,6 +67,24 @@ struct string_list_item *string_list_insert(struct string_list *list, const char return list->items + index; } +void string_list_remove(struct string_list *list, const char *string, + int free_util) +{ + int exact_match; + int i = get_entry_index(list, string, &exact_match); + + if (exact_match) { + if (list->strdup_strings) + free(list->items[i].string); + if (free_util) + free(list->items[i].util); + + list->nr--; + memmove(list->items + i, list->items + i + 1, + (list->nr - i) * sizeof(struct string_list_item)); + } +} + int string_list_has_string(const struct string_list *list, const char *string) { int exact_match; diff --git a/string-list.h b/string-list.h index d3809a1..29bfb7a 100644 --- a/string-list.h +++ b/string-list.h @@ -63,6 +63,13 @@ int string_list_find_insert_index(const struct string_list *list, const char *st struct string_list_item *string_list_insert(struct string_list *list, const char *string); /* + * Removes the given string from the sorted list. + * If the string doesn't exist, the list is not altered. + */ +extern void string_list_remove(struct string_list *list, const char *string, + int free_util); + +/* * Checks if the given string is part of a sorted list. If it is part of the list, * return the coresponding string_list_item, NULL otherwise. */ -- cgit v0.10.2-6-g49f6 From ae25394b4c21f072f176ccbaf7047abe7fa5f04d Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:22 -0700 Subject: run-command: prepare child environment before forking In order to avoid allocation between 'fork()' and 'exec()' prepare the environment to be used in the child process prior to forking. Switch to using 'execve()' so that the construct child environment can used in the exec'd process. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index 1c7a3b6..15e2e74 100644 --- a/run-command.c +++ b/run-command.c @@ -267,6 +267,55 @@ static void prepare_cmd(struct argv_array *out, const struct child_process *cmd) } } } + +static char **prep_childenv(const char *const *deltaenv) +{ + extern char **environ; + char **childenv; + struct string_list env = STRING_LIST_INIT_DUP; + struct strbuf key = STRBUF_INIT; + const char *const *p; + int i; + + /* Construct a sorted string list consisting of the current environ */ + for (p = (const char *const *) environ; p && *p; p++) { + const char *equals = strchr(*p, '='); + + if (equals) { + strbuf_reset(&key); + strbuf_add(&key, *p, equals - *p); + string_list_append(&env, key.buf)->util = (void *) *p; + } else { + string_list_append(&env, *p)->util = (void *) *p; + } + } + string_list_sort(&env); + + /* Merge in 'deltaenv' with the current environ */ + for (p = deltaenv; p && *p; p++) { + const char *equals = strchr(*p, '='); + + if (equals) { + /* ('key=value'), insert or replace entry */ + strbuf_reset(&key); + strbuf_add(&key, *p, equals - *p); + string_list_insert(&env, key.buf)->util = (void *) *p; + } else { + /* otherwise ('key') remove existing entry */ + string_list_remove(&env, *p, 0); + } + } + + /* Create an array of 'char *' to be used as the childenv */ + childenv = xmalloc((env.nr + 1) * sizeof(char *)); + for (i = 0; i < env.nr; i++) + childenv[i] = env.items[i].util; + childenv[env.nr] = NULL; + + string_list_clear(&env, 0); + strbuf_release(&key); + return childenv; +} #endif static inline void set_cloexec(int fd) @@ -395,12 +444,14 @@ fail_pipe: #ifndef GIT_WINDOWS_NATIVE { int notify_pipe[2]; + char **childenv; struct argv_array argv = ARGV_ARRAY_INIT; if (pipe(notify_pipe)) notify_pipe[0] = notify_pipe[1] = -1; prepare_cmd(&argv, cmd); + childenv = prep_childenv(cmd->env); cmd->pid = fork(); failed_errno = errno; @@ -456,14 +507,6 @@ fail_pipe: if (cmd->dir && chdir(cmd->dir)) die_errno("exec '%s': cd to '%s' failed", cmd->argv[0], cmd->dir); - if (cmd->env) { - for (; *cmd->env; cmd->env++) { - if (strchr(*cmd->env, '=')) - putenv((char *)*cmd->env); - else - unsetenv(*cmd->env); - } - } /* * Attempt to exec using the command and arguments starting at @@ -471,9 +514,11 @@ fail_pipe: * be used in the event exec failed with ENOEXEC at which point * we will try to interpret the command using 'sh'. */ - execv(argv.argv[1], (char *const *) argv.argv + 1); + execve(argv.argv[1], (char *const *) argv.argv + 1, + (char *const *) childenv); if (errno == ENOEXEC) - execv(argv.argv[0], (char *const *) argv.argv); + execve(argv.argv[0], (char *const *) argv.argv, + (char *const *) childenv); if (errno == ENOENT) { if (!cmd->silent_exec_failure) @@ -509,6 +554,7 @@ fail_pipe: close(notify_pipe[0]); argv_array_clear(&argv); + free(childenv); } #else { -- cgit v0.10.2-6-g49f6 From db015a284e74b93db9184d39eb0be749e631242d Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:23 -0700 Subject: run-command: don't die in child when duping /dev/null Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index 15e2e74..b3a35dd 100644 --- a/run-command.c +++ b/run-command.c @@ -117,18 +117,6 @@ static inline void close_pair(int fd[2]) close(fd[1]); } -#ifndef GIT_WINDOWS_NATIVE -static inline void dup_devnull(int to) -{ - int fd = open("/dev/null", O_RDWR); - if (fd < 0) - die_errno(_("open /dev/null failed")); - if (dup2(fd, to) < 0) - die_errno(_("dup2(%d,%d) failed"), fd, to); - close(fd); -} -#endif - static char *locate_in_PATH(const char *file) { const char *p = getenv("PATH"); @@ -444,12 +432,20 @@ fail_pipe: #ifndef GIT_WINDOWS_NATIVE { int notify_pipe[2]; + int null_fd = -1; char **childenv; struct argv_array argv = ARGV_ARRAY_INIT; if (pipe(notify_pipe)) notify_pipe[0] = notify_pipe[1] = -1; + if (cmd->no_stdin || cmd->no_stdout || cmd->no_stderr) { + null_fd = open("/dev/null", O_RDWR | O_CLOEXEC); + if (null_fd < 0) + die_errno(_("open /dev/null failed")); + set_cloexec(null_fd); + } + prepare_cmd(&argv, cmd); childenv = prep_childenv(cmd->env); @@ -473,7 +469,7 @@ fail_pipe: atexit(notify_parent); if (cmd->no_stdin) - dup_devnull(0); + dup2(null_fd, 0); else if (need_in) { dup2(fdin[0], 0); close_pair(fdin); @@ -483,7 +479,7 @@ fail_pipe: } if (cmd->no_stderr) - dup_devnull(2); + dup2(null_fd, 2); else if (need_err) { dup2(fderr[1], 2); close_pair(fderr); @@ -493,7 +489,7 @@ fail_pipe: } if (cmd->no_stdout) - dup_devnull(1); + dup2(null_fd, 1); else if (cmd->stdout_to_stderr) dup2(2, 1); else if (need_out) { @@ -553,6 +549,8 @@ fail_pipe: } close(notify_pipe[0]); + if (null_fd >= 0) + close(null_fd); argv_array_clear(&argv); free(childenv); } -- cgit v0.10.2-6-g49f6 From 79319b1949f0055bd42bac7fa398fca8c2f26116 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:24 -0700 Subject: run-command: eliminate calls to error handling functions in child All of our standard error handling paths have the potential to call malloc or take stdio locks; so we must avoid them inside the forked child. Instead, the child only writes an 8 byte struct atomically to the parent through the notification pipe to propagate an error. All user-visible error reporting happens from the parent; even avoiding functions like atexit(3) and exit(3). Helped-by: Eric Wong Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index b3a35dd..1f15714 100644 --- a/run-command.c +++ b/run-command.c @@ -211,14 +211,82 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv) #ifndef GIT_WINDOWS_NATIVE static int child_notifier = -1; -static void notify_parent(void) +enum child_errcode { + CHILD_ERR_CHDIR, + CHILD_ERR_ENOENT, + CHILD_ERR_SILENT, + CHILD_ERR_ERRNO +}; + +struct child_err { + enum child_errcode err; + int syserr; /* errno */ +}; + +static void child_die(enum child_errcode err) { - /* - * execvp failed. If possible, we'd like to let start_command - * know, so failures like ENOENT can be handled right away; but - * otherwise, finish_command will still report the error. - */ - xwrite(child_notifier, "", 1); + struct child_err buf; + + buf.err = err; + buf.syserr = errno; + + /* write(2) on buf smaller than PIPE_BUF (min 512) is atomic: */ + xwrite(child_notifier, &buf, sizeof(buf)); + _exit(1); +} + +/* + * parent will make it look like the child spewed a fatal error and died + * this is needed to prevent changes to t0061. + */ +static void fake_fatal(const char *err, va_list params) +{ + vreportf("fatal: ", err, params); +} + +static void child_error_fn(const char *err, va_list params) +{ + const char msg[] = "error() should not be called in child\n"; + xwrite(2, msg, sizeof(msg) - 1); +} + +static void child_warn_fn(const char *err, va_list params) +{ + const char msg[] = "warn() should not be called in child\n"; + xwrite(2, msg, sizeof(msg) - 1); +} + +static void NORETURN child_die_fn(const char *err, va_list params) +{ + const char msg[] = "die() should not be called in child\n"; + xwrite(2, msg, sizeof(msg) - 1); + _exit(2); +} + +/* this runs in the parent process */ +static void child_err_spew(struct child_process *cmd, struct child_err *cerr) +{ + static void (*old_errfn)(const char *err, va_list params); + + old_errfn = get_error_routine(); + set_error_routine(fake_fatal); + errno = cerr->syserr; + + switch (cerr->err) { + case CHILD_ERR_CHDIR: + error_errno("exec '%s': cd to '%s' failed", + cmd->argv[0], cmd->dir); + break; + case CHILD_ERR_ENOENT: + error_errno("cannot run %s", cmd->argv[0]); + break; + case CHILD_ERR_SILENT: + break; + case CHILD_ERR_ERRNO: + error_errno("cannot exec '%s'", cmd->argv[0]); + break; + } + set_error_routine(old_errfn); } static void prepare_cmd(struct argv_array *out, const struct child_process *cmd) @@ -341,13 +409,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal) code += 128; } else if (WIFEXITED(status)) { code = WEXITSTATUS(status); - /* - * Convert special exit code when execvp failed. - */ - if (code == 127) { - code = -1; - failed_errno = ENOENT; - } } else { error("waitpid is confused (%s)", argv0); } @@ -435,6 +496,7 @@ fail_pipe: int null_fd = -1; char **childenv; struct argv_array argv = ARGV_ARRAY_INIT; + struct child_err cerr; if (pipe(notify_pipe)) notify_pipe[0] = notify_pipe[1] = -1; @@ -453,20 +515,16 @@ fail_pipe: failed_errno = errno; if (!cmd->pid) { /* - * Redirect the channel to write syscall error messages to - * before redirecting the process's stderr so that all die() - * in subsequent call paths use the parent's stderr. + * Ensure the default die/error/warn routines do not get + * called, they can take stdio locks and malloc. */ - if (cmd->no_stderr || need_err) { - int child_err = dup(2); - set_cloexec(child_err); - set_error_handle(fdopen(child_err, "w")); - } + set_die_routine(child_die_fn); + set_error_routine(child_error_fn); + set_warn_routine(child_warn_fn); close(notify_pipe[0]); set_cloexec(notify_pipe[1]); child_notifier = notify_pipe[1]; - atexit(notify_parent); if (cmd->no_stdin) dup2(null_fd, 0); @@ -501,8 +559,7 @@ fail_pipe: } if (cmd->dir && chdir(cmd->dir)) - die_errno("exec '%s': cd to '%s' failed", cmd->argv[0], - cmd->dir); + child_die(CHILD_ERR_CHDIR); /* * Attempt to exec using the command and arguments starting at @@ -517,12 +574,11 @@ fail_pipe: (char *const *) childenv); if (errno == ENOENT) { - if (!cmd->silent_exec_failure) - error("cannot run %s: %s", cmd->argv[0], - strerror(ENOENT)); - exit(127); + if (cmd->silent_exec_failure) + child_die(CHILD_ERR_SILENT); + child_die(CHILD_ERR_ENOENT); } else { - die_errno("cannot exec '%s'", cmd->argv[0]); + child_die(CHILD_ERR_ERRNO); } } if (cmd->pid < 0) @@ -533,17 +589,18 @@ fail_pipe: /* * Wait for child's exec. If the exec succeeds (or if fork() * failed), EOF is seen immediately by the parent. Otherwise, the - * child process sends a single byte. + * child process sends a child_err struct. * Note that use of this infrastructure is completely advisory, * therefore, we keep error checks minimal. */ close(notify_pipe[1]); - if (read(notify_pipe[0], ¬ify_pipe[1], 1) == 1) { + if (xread(notify_pipe[0], &cerr, sizeof(cerr)) == sizeof(cerr)) { /* * At this point we know that fork() succeeded, but exec() * failed. Errors have been reported to our stderr. */ wait_or_whine(cmd->pid, cmd->argv[0], 0); + child_err_spew(cmd, &cerr); failed_errno = errno; cmd->pid = -1; } -- cgit v0.10.2-6-g49f6 From 53fa6753b30c4fb4ea768d16d41d723ea19a3b00 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:25 -0700 Subject: run-command: handle dup2 and close errors in child Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index 1f15714..615b6e9 100644 --- a/run-command.c +++ b/run-command.c @@ -213,6 +213,8 @@ static int child_notifier = -1; enum child_errcode { CHILD_ERR_CHDIR, + CHILD_ERR_DUP2, + CHILD_ERR_CLOSE, CHILD_ERR_ENOENT, CHILD_ERR_SILENT, CHILD_ERR_ERRNO @@ -235,6 +237,24 @@ static void child_die(enum child_errcode err) _exit(1); } +static void child_dup2(int fd, int to) +{ + if (dup2(fd, to) < 0) + child_die(CHILD_ERR_DUP2); +} + +static void child_close(int fd) +{ + if (close(fd)) + child_die(CHILD_ERR_CLOSE); +} + +static void child_close_pair(int fd[2]) +{ + child_close(fd[0]); + child_close(fd[1]); +} + /* * parent will make it look like the child spewed a fatal error and died * this is needed to prevent changes to t0061. @@ -277,6 +297,12 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr) error_errno("exec '%s': cd to '%s' failed", cmd->argv[0], cmd->dir); break; + case CHILD_ERR_DUP2: + error_errno("dup2() in child failed"); + break; + case CHILD_ERR_CLOSE: + error_errno("close() in child failed"); + break; case CHILD_ERR_ENOENT: error_errno("cannot run %s", cmd->argv[0]); break; @@ -527,35 +553,35 @@ fail_pipe: child_notifier = notify_pipe[1]; if (cmd->no_stdin) - dup2(null_fd, 0); + child_dup2(null_fd, 0); else if (need_in) { - dup2(fdin[0], 0); - close_pair(fdin); + child_dup2(fdin[0], 0); + child_close_pair(fdin); } else if (cmd->in) { - dup2(cmd->in, 0); - close(cmd->in); + child_dup2(cmd->in, 0); + child_close(cmd->in); } if (cmd->no_stderr) - dup2(null_fd, 2); + child_dup2(null_fd, 2); else if (need_err) { - dup2(fderr[1], 2); - close_pair(fderr); + child_dup2(fderr[1], 2); + child_close_pair(fderr); } else if (cmd->err > 1) { - dup2(cmd->err, 2); - close(cmd->err); + child_dup2(cmd->err, 2); + child_close(cmd->err); } if (cmd->no_stdout) - dup2(null_fd, 1); + child_dup2(null_fd, 1); else if (cmd->stdout_to_stderr) - dup2(2, 1); + child_dup2(2, 1); else if (need_out) { - dup2(fdout[1], 1); - close_pair(fdout); + child_dup2(fdout[1], 1); + child_close_pair(fdout); } else if (cmd->out > 1) { - dup2(cmd->out, 1); - close(cmd->out); + child_dup2(cmd->out, 1); + child_close(cmd->out); } if (cmd->dir && chdir(cmd->dir)) -- cgit v0.10.2-6-g49f6 From e503cd6ed336d70d716e194ef6c5469330bea9da Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 19 Apr 2017 16:13:26 -0700 Subject: run-command: add note about forking and threading All non-Async-Signal-Safe functions (e.g. malloc and die) were removed between 'fork' and 'exec' in start_command in order to avoid potential deadlocking when forking while multiple threads are running. This deadlocking is possible when a thread (other than the one forking) has acquired a lock and didn't get around to releasing it before the fork. This leaves the lock in a locked state in the resulting process with no hope of it ever being released. Add a note describing this potential pitfall before the call to 'fork()' so people working in this section of the code know to only use Async-Signal-Safe functions in the child process. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index 615b6e9..df1edd9 100644 --- a/run-command.c +++ b/run-command.c @@ -537,6 +537,15 @@ fail_pipe: prepare_cmd(&argv, cmd); childenv = prep_childenv(cmd->env); + /* + * NOTE: In order to prevent deadlocking when using threads special + * care should be taken with the function calls made in between the + * fork() and exec() calls. No calls should be made to functions which + * require acquiring a lock (e.g. malloc) as the lock could have been + * held by another thread at the time of forking, causing the lock to + * never be released in the child process. This means only + * Async-Signal-Safe functions are permitted in the child. + */ cmd->pid = fork(); failed_errno = errno; if (!cmd->pid) { -- cgit v0.10.2-6-g49f6 From 45afb1ca9c28855096c94926e5b16dfbcde7381f Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Wed, 19 Apr 2017 16:13:27 -0700 Subject: run-command: block signals between fork and execve Signal handlers of the parent firing in the forked child may have unintended side effects. Rather than auditing every signal handler we have and will ever have, block signals while forking and restore default signal handlers in the child before execve. Restoring default signal handlers is required because execve does not unblock signals, it only restores default signal handlers. So we must restore them with sigprocmask before execve, leaving a window when signal handlers we control can fire in the child. Continue ignoring ignored signals, but reset the rest to defaults. Similarly, disable pthread cancellation to future-proof our code in case we start using cancellation; as cancellation is implemented with signals in glibc. Signed-off-by: Eric Wong Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index df1edd9..a97d7bf 100644 --- a/run-command.c +++ b/run-command.c @@ -215,6 +215,7 @@ enum child_errcode { CHILD_ERR_CHDIR, CHILD_ERR_DUP2, CHILD_ERR_CLOSE, + CHILD_ERR_SIGPROCMASK, CHILD_ERR_ENOENT, CHILD_ERR_SILENT, CHILD_ERR_ERRNO @@ -303,6 +304,9 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr) case CHILD_ERR_CLOSE: error_errno("close() in child failed"); break; + case CHILD_ERR_SIGPROCMASK: + error_errno("sigprocmask failed restoring signals"); + break; case CHILD_ERR_ENOENT: error_errno("cannot run %s", cmd->argv[0]); break; @@ -398,7 +402,54 @@ static char **prep_childenv(const char *const *deltaenv) strbuf_release(&key); return childenv; } + +struct atfork_state { +#ifndef NO_PTHREADS + int cs; #endif + sigset_t old; +}; + +#ifndef NO_PTHREADS +static void bug_die(int err, const char *msg) +{ + if (err) { + errno = err; + die_errno("BUG: %s", msg); + } +} +#endif + +static void atfork_prepare(struct atfork_state *as) +{ + sigset_t all; + + if (sigfillset(&all)) + die_errno("sigfillset"); +#ifdef NO_PTHREADS + if (sigprocmask(SIG_SETMASK, &all, &as->old)) + die_errno("sigprocmask"); +#else + bug_die(pthread_sigmask(SIG_SETMASK, &all, &as->old), + "blocking all signals"); + bug_die(pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &as->cs), + "disabling cancellation"); +#endif +} + +static void atfork_parent(struct atfork_state *as) +{ +#ifdef NO_PTHREADS + if (sigprocmask(SIG_SETMASK, &as->old, NULL)) + die_errno("sigprocmask"); +#else + bug_die(pthread_setcancelstate(as->cs, NULL), + "re-enabling cancellation"); + bug_die(pthread_sigmask(SIG_SETMASK, &as->old, NULL), + "restoring signal mask"); +#endif +} +#endif /* GIT_WINDOWS_NATIVE */ static inline void set_cloexec(int fd) { @@ -523,6 +574,7 @@ fail_pipe: char **childenv; struct argv_array argv = ARGV_ARRAY_INIT; struct child_err cerr; + struct atfork_state as; if (pipe(notify_pipe)) notify_pipe[0] = notify_pipe[1] = -1; @@ -536,6 +588,7 @@ fail_pipe: prepare_cmd(&argv, cmd); childenv = prep_childenv(cmd->env); + atfork_prepare(&as); /* * NOTE: In order to prevent deadlocking when using threads special @@ -549,6 +602,7 @@ fail_pipe: cmd->pid = fork(); failed_errno = errno; if (!cmd->pid) { + int sig; /* * Ensure the default die/error/warn routines do not get * called, they can take stdio locks and malloc. @@ -597,6 +651,19 @@ fail_pipe: child_die(CHILD_ERR_CHDIR); /* + * restore default signal handlers here, in case + * we catch a signal right before execve below + */ + for (sig = 1; sig < NSIG; sig++) { + /* ignored signals get reset to SIG_DFL on execve */ + if (signal(sig, SIG_DFL) == SIG_IGN) + signal(sig, SIG_IGN); + } + + if (sigprocmask(SIG_SETMASK, &as.old, NULL) != 0) + child_die(CHILD_ERR_SIGPROCMASK); + + /* * Attempt to exec using the command and arguments starting at * argv.argv[1]. argv.argv[0] contains SHELL_PATH which will * be used in the event exec failed with ENOEXEC at which point @@ -616,6 +683,7 @@ fail_pipe: child_die(CHILD_ERR_ERRNO); } } + atfork_parent(&as); if (cmd->pid < 0) error_errno("cannot fork() for %s", cmd->argv[0]); else if (cmd->clean_on_exit) -- cgit v0.10.2-6-g49f6 From 38124a40e480c1717326b7bc27bcbca758de908e Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Tue, 25 Apr 2017 16:46:59 -0700 Subject: run-command: expose is_executable function Move the logic for 'is_executable()' from help.c to run_command.c and expose it so that callers from outside help.c can access the function. This is to enable run-command to be able to query if a file is executable in a future patch. Signed-off-by: Brandon Williams Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano diff --git a/help.c b/help.c index bc6cd19..0c65a2d 100644 --- a/help.c +++ b/help.c @@ -1,6 +1,7 @@ #include "cache.h" #include "builtin.h" #include "exec_cmd.h" +#include "run-command.h" #include "levenshtein.h" #include "help.h" #include "common-cmds.h" @@ -96,48 +97,6 @@ static void pretty_print_cmdnames(struct cmdnames *cmds, unsigned int colopts) string_list_clear(&list, 0); } -static int is_executable(const char *name) -{ - struct stat st; - - if (stat(name, &st) || /* stat, not lstat */ - !S_ISREG(st.st_mode)) - return 0; - -#if defined(GIT_WINDOWS_NATIVE) - /* - * On Windows there is no executable bit. The file extension - * indicates whether it can be run as an executable, and Git - * has special-handling to detect scripts and launch them - * through the indicated script interpreter. We test for the - * file extension first because virus scanners may make - * it quite expensive to open many files. - */ - if (ends_with(name, ".exe")) - return S_IXUSR; - -{ - /* - * Now that we know it does not have an executable extension, - * peek into the file instead. - */ - char buf[3] = { 0 }; - int n; - int fd = open(name, O_RDONLY); - st.st_mode &= ~S_IXUSR; - if (fd >= 0) { - n = read(fd, buf, 2); - if (n == 2) - /* look for a she-bang */ - if (!strcmp(buf, "#!")) - st.st_mode |= S_IXUSR; - close(fd); - } -} -#endif - return st.st_mode & S_IXUSR; -} - static void list_commands_in_dir(struct cmdnames *cmds, const char *path, const char *prefix) diff --git a/run-command.c b/run-command.c index a97d7bf..2ffbd7e 100644 --- a/run-command.c +++ b/run-command.c @@ -117,6 +117,48 @@ static inline void close_pair(int fd[2]) close(fd[1]); } +int is_executable(const char *name) +{ + struct stat st; + + if (stat(name, &st) || /* stat, not lstat */ + !S_ISREG(st.st_mode)) + return 0; + +#if defined(GIT_WINDOWS_NATIVE) + /* + * On Windows there is no executable bit. The file extension + * indicates whether it can be run as an executable, and Git + * has special-handling to detect scripts and launch them + * through the indicated script interpreter. We test for the + * file extension first because virus scanners may make + * it quite expensive to open many files. + */ + if (ends_with(name, ".exe")) + return S_IXUSR; + +{ + /* + * Now that we know it does not have an executable extension, + * peek into the file instead. + */ + char buf[3] = { 0 }; + int n; + int fd = open(name, O_RDONLY); + st.st_mode &= ~S_IXUSR; + if (fd >= 0) { + n = read(fd, buf, 2); + if (n == 2) + /* look for a she-bang */ + if (!strcmp(buf, "#!")) + st.st_mode |= S_IXUSR; + close(fd); + } +} +#endif + return st.st_mode & S_IXUSR; +} + static char *locate_in_PATH(const char *file) { const char *p = getenv("PATH"); diff --git a/run-command.h b/run-command.h index 4fa8f65..3932420 100644 --- a/run-command.h +++ b/run-command.h @@ -51,6 +51,7 @@ struct child_process { #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT } void child_process_init(struct child_process *); void child_process_clear(struct child_process *); +extern int is_executable(const char *name); int start_command(struct child_process *); int finish_command(struct child_process *); -- cgit v0.10.2-6-g49f6 From 940283101ce87250cf31a592730386f5061e1286 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Tue, 25 Apr 2017 16:47:00 -0700 Subject: run-command: restrict PATH search to executable files In some situations run-command will incorrectly try (and fail) to execute a directory instead of an executable file. This was observed by having a directory called "ssh" in $PATH before the real ssh and trying to use ssh protoccol, reslting in the following: $ git ls-remote ssh://url fatal: cannot exec 'ssh': Permission denied It ends up being worse and run-command will even try to execute a non-executable file if it preceeds the executable version of a file on the PATH. For example, if PATH=~/bin1:~/bin2:~/bin3 and there exists a directory 'git-hello' in 'bin1', a non-executable file 'git-hello' in bin2 and an executable file 'git-hello' (which prints "Hello World!") in bin3 the following will occur: $ git hello fatal: cannot exec 'git-hello': Permission denied This is due to only checking 'access()' when locating an executable in PATH, which doesn't distinguish between files and directories. Instead use 'is_executable()' which check that the path is to a regular, executable file. Now run-command won't try to execute the directory or non-executable file 'git-hello': $ git hello Hello World! which matches what execvp(3) would have done when asked to execute git-hello with such a $PATH. Reported-by: Brian Hatfield Signed-off-by: Brandon Williams Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index 2ffbd7e..9e36151 100644 --- a/run-command.c +++ b/run-command.c @@ -159,6 +159,23 @@ int is_executable(const char *name) return st.st_mode & S_IXUSR; } +/* + * Search $PATH for a command. This emulates the path search that + * execvp would perform, without actually executing the command so it + * can be used before fork() to prepare to run a command using + * execve() or after execvp() to diagnose why it failed. + * + * The caller should ensure that file contains no directory + * separators. + * + * Returns the path to the command, as found in $PATH or NULL if the + * command could not be found. The caller inherits ownership of the memory + * used to store the resultant path. + * + * This should not be used on Windows, where the $PATH search rules + * are more complicated (e.g., a search for "foo" should find + * "foo.exe"). + */ static char *locate_in_PATH(const char *file) { const char *p = getenv("PATH"); @@ -179,7 +196,7 @@ static char *locate_in_PATH(const char *file) } strbuf_addstr(&buf, file); - if (!access(buf.buf, F_OK)) + if (is_executable(buf.buf)) return strbuf_detach(&buf, NULL); if (!*end) diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 98c09dd..e473917 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -37,6 +37,36 @@ test_expect_success !MINGW 'run_command can run a script without a #! line' ' test_cmp empty err ' +test_expect_success 'run_command does not try to execute a directory' ' + test_when_finished "rm -rf bin1 bin2" && + mkdir -p bin1/greet bin2 && + write_script bin2/greet <<-\EOF && + cat bin2/greet + EOF + + PATH=$PWD/bin1:$PWD/bin2:$PATH \ + test-run-command run-command greet >actual 2>err && + test_cmp bin2/greet actual && + test_cmp empty err +' + +test_expect_success POSIXPERM 'run_command passes over non-executable file' ' + test_when_finished "rm -rf bin1 bin2" && + mkdir -p bin1 bin2 && + write_script bin1/greet <<-\EOF && + cat bin1/greet + EOF + chmod -x bin1/greet && + write_script bin2/greet <<-\EOF && + cat bin2/greet + EOF + + PATH=$PWD/bin1:$PWD/bin2:$PATH \ + test-run-command run-command greet >actual 2>err && + test_cmp bin2/greet actual && + test_cmp empty err +' + test_expect_success POSIXPERM 'run_command reports EACCES' ' cat hello-script >hello.sh && chmod -x hello.sh && -- cgit v0.10.2-6-g49f6 From e3f43ce765c38f4be94239d07c8c3c596780c514 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 12 May 2017 23:48:18 -0400 Subject: usage.c: drop set_error_handle() The set_error_handle() function was introduced by 3b331e926 (vreportf: report to arbitrary filehandles, 2015-08-11) so that run-command could send post-fork, pre-exec errors to the parent's original stderr. That use went away in 79319b194 (run-command: eliminate calls to error handling functions in child, 2017-04-19), which pushes all of the error reporting to the parent. This leaves no callers of set_error_handle(). As we're not likely to add any new ones, let's drop it. Signed-off-by: Jeff King Acked-by: Brandon Williams Reviewed-by: Ramsay Jones Signed-off-by: Junio C Hamano diff --git a/git-compat-util.h b/git-compat-util.h index 8a4a3f8..f1f2a2d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -445,7 +445,6 @@ extern void (*get_error_routine(void))(const char *err, va_list params); extern void set_warn_routine(void (*routine)(const char *warn, va_list params)); extern void (*get_warn_routine(void))(const char *warn, va_list params); extern void set_die_is_recursing_routine(int (*routine)(void)); -extern void set_error_handle(FILE *); extern int starts_with(const char *str, const char *prefix); diff --git a/usage.c b/usage.c index ad6d291..2623c07 100644 --- a/usage.c +++ b/usage.c @@ -6,12 +6,9 @@ #include "git-compat-util.h" #include "cache.h" -static FILE *error_handle; - void vreportf(const char *prefix, const char *err, va_list params) { char msg[4096]; - FILE *fh = error_handle ? error_handle : stderr; char *p; vsnprintf(msg, sizeof(msg), err, params); @@ -19,7 +16,7 @@ void vreportf(const char *prefix, const char *err, va_list params) if (iscntrl(*p) && *p != '\t' && *p != '\n') *p = '?'; } - fprintf(fh, "%s%s\n", prefix, msg); + fprintf(stderr, "%s%s\n", prefix, msg); } static NORETURN void usage_builtin(const char *err, va_list params) @@ -88,11 +85,6 @@ void set_die_is_recursing_routine(int (*routine)(void)) die_is_recursing = routine; } -void set_error_handle(FILE *fh) -{ - error_handle = fh; -} - void NORETURN usagef(const char *err, ...) { va_list params; -- cgit v0.10.2-6-g49f6