Fix race and deadlock when sending pack

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.

---

 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

index cbe37e7..1873506 100644
@@ -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);
 }
 
 /*

index 8bf5922..38cd6cb 100644
@@ -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);
 }

index 2469eea..5ee0972 100644
@@ -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, ...);
 

index bcba893..d470dcc 100644
@@ -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

index 6db555f..6199deb 100644
@@ -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="[email protected]"
 
 if expr "$2" : '0*$' >/dev/null

0.99.9.GIT

My immediate reaction was "do not do it then", but you are right. Hooks are run after all the protocol exchanges are done, so they should be free to throw any garbage at the other end.

It appears cpfd() seems to mostly duplicate what is in copy.c; is there any particular reason?

No, I just wasn't aware of it. I've made another patch to account for it. I wonder why copy_fd() (usually) closes ifd though. I also wonder why, if it is going to close ifd, doesn't it do so when there an error is detected after the write() call.

I would also like to mention that I'm on the fence about all of this. I think a lot of it is a matter of policy, and I'm just not sure what the policy is regarding the hooks. I wanted to submit the patch in it full form to show that it is possible to redirect stdout of the hooks to the terminal. I also feel that, even if the patch is ultimately rejected (no hard feelings :-), it would be interesting to know what's the underlying problem.

Paul Serice

========================================================================

Subject: [PATCH] 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 stdin and stdout to /dev/null.

---

 receive-pack.c               |    2 +-
 run-command.c                |   27 +++++++++++++--------------
 run-command.h                |    3 ---
 send-pack.c                  |    9 ++++++++-
 templates/hooks--post-update |    4 ++++
 templates/hooks--update      |    4 ++++
 6 files changed, 30 insertions(+), 19 deletions(-)

87b7ee91cbe1b90bbd0937a85595de3933fc9459

index cbe37e7..1873506 100644
@@ -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);
 }
 
 /*

index 8bf5922..38cd6cb 100644
@@ -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);
 }

index 2469eea..5ee0972 100644
@@ -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, ...);
 

index 6ce0d9f..5a99ba9 100644
@@ -319,8 +319,15 @@ int main(int argc, char **argv)
 	if (pid < 0)
 		return 1;
 	ret = send_pack(fd[0], fd[1], nr_heads, heads);
-	close(fd[0]);
+
+	/* Close our side of the conversation.	Wait for the child to
+	 * close its side of the conversation (copying the remainder
+	 * to our stdout).  Note that copy_fd() has the side effect of
+	 * closing fd[0]. */
 	close(fd[1]);
+	copy_fd(fd[0], fileno(stdout));
+
 	finish_connect(pid);
+
 	return ret;
 }

index bcba893..d470dcc 100644
@@ -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

index 6db555f..6199deb 100644
@@ -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="[email protected]"
 
 if expr "$2" : '0*$' >/dev/null

0.99.9.GIT

If we extend it to transfer multiple things, wouldn't we want to run hooks after each of them, rather than all at the end?

As for the policy:

We definitely want to let hooks write to stdout, because git programs that you might want to run in hooks write to stdout. I can't figure out what "cvs" does with trigger script output and "at" and "cron" email the output to the owners. I'd sort of like to avoid making people expect that there is necessarily a path for text going back to the user directly. We may, for example, want to support these hooks with pushes over HTTP(/WebDAV). I also think that messages are likely to be at least as useful to the owner of the target repository as the person pushing, which is why I'd prefer a log file. E.g., if you've got a group central repository that different people push to, it may be other group members who want to know what happened with the output from a post-update hook, not the group member who pushed.

-Daniel
*This .sig left intentionally blank*

We do transfer multiple things already, and all protocol exchange happens before everything is transferred. And hooks are run for each refs being updated, one by one. What we do not have is a reporting mechanism that says "we refused to update this ref because of the hooks/update policy return value for it". Even if we later add that reporting mechanism, as I outlined in a separate message earlier, I think it is OK to keep running the update hooks after the pack transfer part.

This part I mostly agree with. Will have to think about the details but probably I'd punt this for now and declare it post 1.0 ;-).

If we have the reporting mechanism, that will effectively be part of the protocol. It's obviously done transferring the pack at that point, but it still wants fixed-format communication, so switching over to being the stardard output of the hooks would cause problems with this.

It's probably worth making sure that all the hooks run with something sane, and punt making it configurable andnice until post-1.0. I was only really looking at post-update, so I don't know how the others run.

-Daniel
*This .sig left intentionally blank*

In order to add reporting mechanism later, I think we need to be able to identify the protocol version in a backward compatible way, something like the "server capabilities hidden behind the NUL" trick we did for fetch-pack/upload-pack protocol. Once that is in place, it does not cause harm even if the current protocol program connects hooks' stdout to send-pack, at least in theory. If we take Paul's patch now, however, it would add more work for us later when we do that protocol change, because we will need to wrap the output from the hook in the pkt-line interface in the new protocol, in order to give that back to the stdout of send-pack. Considering that, I think we may want to drop Paul's patch and declare that hooks stdout does not come back to send-pack.

Honestly speaking, I do not really care where stdout of hooks go as long as that does not cause breakage/deadlocks, and I think your earlier patch on December 7th is serving us well enough; we needed to have told users to do an "exec 1>somewhere" in their hooks before that fix, which was not nice at all (and we even forgot to tell them that). If people want to send the output to a log file, they can do so; if they want e-mails, they can do so; if they want to show the output to the pusher, they can do 1>&2; all inside their hooks. I do "echo nitfol | at now" and love the way that I do not have to worry about how "at" command gives me back execution report via e-mail at all ;-).

I think we agree that /dev/null is one of the sane choices as you did in your earlier fix. Duping stderr would have been another sane choice, but I honestly do not think we care much either way.

Actually, that was stdin and stdout. If, for some reason, a hook looked at stdin, it could get surprising results. I don't think that it's actually a good idea to have output to stdout from hooks go to git-send-pack's stdout, since we may want to have git-send-pack report some sort of information of its own to stdout, which would then get confused with output from hooks. I think /dev/null, a log file, and stderr are the reasonable choices for what happens to output (and input pretty much has to be /dev/null).

-Daniel
*This .sig left intentionally blank*

In the first patch, the hook would hang waiting for input. That was a bug. In the second patch, the hook's standard input has been closed by git-send-pack (which is the standard thing for the parent to do in a pipeline), thus if a hook looks at stdin it will get EOF (the same as if connected to /dev/null).

Paul

I admit that I haven't thought things through yet, but I do not offhand think of an argument against Paul's patch (a scenario that may be broken by the patch, that is), so I am inclined to take it, perhaps after hearing about the cpfd() thing I mentioned in the previous response to Paul.

It is conceivable that we may want to later extend the protocol so that the receiver can tell the sender the result of what happened to each of the ref-update request. Right now, the sender refuses to listen to what receiver says after it learns the current object names, but after pack transfer finishes and receiver decides what to do with each ref update request, we might want to add status, like this:

# Tell the pusher what commits we have and what their names are
R: SHA1 name
R: ...
R: SHA1 name
R: # flush -- it's your turn
# Tell the puller what the pusher wants to happen
S: old-SHA1 new-SHA1 name
S: old-SHA1 new-SHA1 name
S: ...
S: # flush -- done with the list
S: XXXXXXX --- packfile contents.
# current protocol exchange ends here, but we could add...

# ... what happened to each ref-update request.
R: name OK
R: name FAIL
R: ...

If we do something like this, we might want to say why things failed on "FAIL" line, and the output from hooks/update that prevented the ref-update would probably belong there.