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

From: Jeff King <peff@peff.net>
Date: Mar 22, 2015, 08:07 UTC
Message-ID: <20150322080708.GA3456@peff.net>
In-Reply-To: <20150322074455.GA1303@peff.net>

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

Show 6 quoted lines

> 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