Re: [PATCH] status: read submodule process output before calling wait()

[Jeff King <peff@peff.net>]

On Sun, Mar 22, 2015 at 03:44:55AM -0400, Jeff King wrote:

> The status code tries to read the output of "git submodule
> summary" over a pipe by waiting for the program to finish
> and then reading its output, like this:
> 
>   run_command(&sm_summary);
>   len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);

By the way, I spotted this code as bogus immediately upon seeing it
(though certainly it helped to know there was a deadlock in the area,
which had me thinking about such things). So I wondered if it could have
been easy to catch in review, but its introduction was a little bit
subtle.

The original run_command invocation came in ac8d5af (builtin-status:
submodule summary support, 2008-04-12), which just let the sub-process
dump its stdout to the same descriptor that the rest of the status
output was going to. So the use of run_command there was fine. It was
later, in 3ba7407 (submodule summary: ignore --for-status option,
2013-09-06), that we started post-processing the output and it became
buggy. But that's harder to see in review.

> Besides being a violation of the run-command API (which
> makes no promises about the state of the struct after
> run_command returns),

This may be overly harsh of me. Certainly we make no guarantees (and
things like the dynamic "args" and "env_array" are cleaned up
automatically after finish_command returns), but I would not be
surprised if there are other spots that treat "struct child_process" as
transparent rather than as a black box.

It's really the run_command + pipe construct that is really the danger
here. I wonder if we should do something like this:

diff --git a/run-command.c b/run-command.c
index 3afb124..78807de 100644
--- a/run-command.c
+++ b/run-command.c
@@ -557,7 +557,12 @@ int finish_command(struct child_process *cmd)
 
 int run_command(struct child_process *cmd)
 {
-	int code = start_command(cmd);
+	int code;
+
+	if (cmd->out < 0)
+		die("BUG: run_command with a pipe can cause deadlock");
+
+	code = start_command(cmd);
 	if (code)
 		return code;
 	return finish_command(cmd);

It seems to catch at least one other dubious construct.

-Peff