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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html