Fix race and deadlock when sending pack.
The best way to reproduce the problem is to locally clone your repository. When you perform a push, git-send-pack will directly set up pipes connected to stdin and stdout of git-receive-pack. You should then set up hook/post-update or hook/update to try to write lots of text to stdout. (You want to use the local protocol because ssh is robust enough to mask the worst behavior.)
The first problem is that git-send-pack closes git-receive-pack's stdout (which is inherited by the hooks) immediately after sending the pack. This almost always causes the hooks to receive SIGPIPE when they try to write to stdout.
After fixing the SIGPIPE problem, you then run into a deadlock because git-send-pack is blocked trying to reap git-receive-pack and git-receive-pack (or one of its hooks) is blocked waiting for git-send-pack to read its output.
I've also added an example a one-liner to both hooks demonstrating how to redirect all subsequent output to stderr. Because git-receive-pack's stderr is not redirected, it has always been safe to write to stderr. Thus, all current status related output appears on stderr. This can lead to confusing ordering of messages if only the hooks are using stdout. The patch has the one-liner commented out, but perhaps it should be enabled by default.
In addition, this commit reverts the work-around provided by 128aed684d0b3099092b7597c8644599b45b7503 which redirected both stdout and stderr for the hooks to /dev/null.
Signed-off-by: Paul Serice <paul@serice.net>
---
receive-pack.c | 2 + run-command.c | 27 +++++++++--------- run-command.h | 3 -- send-pack.c | 64 +++++++++++++++++++++++++++++++++++++++++- templates/hooks--post-update | 4 +++ templates/hooks--update | 4 +++ 6 files changed, 85 insertions(+), 19 deletions(-)
36800ae8c6aa1427608a2d131b24986edba91bc9
Show changes to 6 files +85 −19
receive-pack.c, run-command.c, run-command.h, send-pack.c, templates/hooks--post-update, templates/hooks--update
diff --git a/receive-pack.c b/receive-pack.c index cbe37e7..1873506 100644 --- a/receive-pack.c +++ b/receive-pack.c @@ -173,7 +173,7 @@ static void run_update_post_hook(struct argc++; } argv[argc] = NULL; - run_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO); + run_command_v(argc, argv); } /* diff --git a/run-command.c b/run-command.c index 8bf5922..38cd6cb 100644 --- a/run-command.c +++ b/run-command.c @@ -2,19 +2,23 @@ #include "run-command.h" #include <sys/wait.h> -int run_command_v_opt(int argc, char **argv, int flags) +int run_command_v(int argc, char **argv) { - pid_t pid = fork(); + + pid_t pid = (pid_t)-1; + + /* Because each process has independent buffering, if you + * don't flush before the fork, it can seem like the new + * output for the child occurs before the old output of the + * parent which can be confusing at times. */ + fflush(stdout); + fflush(stderr); + + pid = fork(); if (pid < 0) return -ERR_RUN_COMMAND_FORK; if (!pid) { - if (flags & RUN_COMMAND_NO_STDIO) { - int fd = open("/dev/null", O_RDWR); - dup2(fd, 0); - dup2(fd, 1); - close(fd); - } execvp(argv[0], (char *const*) argv); die("exec %s failed.", argv[0]); } @@ -42,11 +46,6 @@ int run_command_v_opt(int argc, char **a } } -int run_command_v(int argc, char **argv) -{ - return run_command_v_opt(argc, argv, 0); -} - int run_command(const char *cmd, ...) { int argc; @@ -65,5 +64,5 @@ int run_command(const char *cmd, ...) va_end(param); if (MAX_RUN_COMMAND_ARGS <= argc) return error("too many args to run %s", cmd); - return run_command_v_opt(argc, argv, 0); + return run_command_v(argc, argv); } diff --git a/run-command.h b/run-command.h index 2469eea..5ee0972 100644 --- a/run-command.h +++ b/run-command.h @@ -11,9 +11,6 @@ enum { ERR_RUN_COMMAND_WAITPID_NOEXIT, }; -#define RUN_COMMAND_NO_STDIO 1 - -int run_command_v_opt(int argc, char **argv, int opt); int run_command_v(int argc, char **argv); int run_command(const char *cmd, ...); diff --git a/send-pack.c b/send-pack.c index 6ce0d9f..efc66ca 100644 --- a/send-pack.c +++ b/send-pack.c @@ -274,6 +274,55 @@ static int send_pack(int in, int out, in return ret; } +/* This function copies the data from in_fd to out_fd. It returns 1 if + * all data was successfully copied; otherwise, it returns 0, and + * errno will be set depending on how read() or write() failed. */ +static int cpfd(int in_fd, int out_fd) +{ + int rv = 0; + ssize_t rcount = -1; + ssize_t wcount = 0; + ssize_t wtmp = -1; + static char cpfd_buf[4096]; + + for (;;) { + + /* Read buffer. */ + rcount = read(in_fd, cpfd_buf, sizeof(cpfd_buf)); + + /* Done. */ + if (rcount == 0) { + rv = 1; + goto out; + } + + /* Error. */ + if (rcount < 0) { + if (errno == EINTR) { + continue; + } + goto out; + } + + /* Write buffer. */ + wcount = 0; + while (wcount < rcount) { + wtmp = write(out_fd, + cpfd_buf + wcount, + rcount - wcount); + if (wtmp < 0) { + if (errno == EINTR) { + continue; + } + goto out; + } + wcount += wtmp; + } + } + + out: + return rv; +} int main(int argc, char **argv) { @@ -319,8 +368,21 @@ int main(int argc, char **argv) if (pid < 0) return 1; ret = send_pack(fd[0], fd[1], nr_heads, heads); - close(fd[0]); + + /* git_connect() sets up a pipe between this program and + * "exec" (typically git-receive-pack). The git-receive-pack + * program in turn executes the "update" and "post-update" + * hooks which might write to this program's fd[0]. To avoid + * deadlock, this program must consume all of the data on + * fd[0]. (The hooks are called after the pack has been + * transfered so it should it should be safe to allow them to + * write to stdout because it should not interfere with the + * transfer protocol which also occurs on stdout). */ + cpfd(fd[0], fileno(stdout)); + close(fd[1]); finish_connect(pid); + close(fd[0]); + return ret; } diff --git a/templates/hooks--post-update b/templates/hooks--post-update index bcba893..d470dcc 100644 --- a/templates/hooks--post-update +++ b/templates/hooks--post-update @@ -5,4 +5,8 @@ # # To enable this hook, make this file executable by "chmod +x post-update". +# If your stdout and stderr messages are interleaved, uncomment the +# following line. +#exec 1>&2 + exec git-update-server-info diff --git a/templates/hooks--update b/templates/hooks--update index 6db555f..6199deb 100644 --- a/templates/hooks--update +++ b/templates/hooks--update @@ -8,6 +8,10 @@ # (2) make this file executable by "chmod +x update". # +# If your stdout and stderr messages are interleaved, uncomment the +# following line. +#exec 1>&2 + recipient="commit-list@example.com" if expr "$2" : '0*$' >/dev/null
-- 0.99.9.GIT
