[PATCH 0/7] introduce strbuf_read_cmd to avoid deadlocks

From: Jeff King <peff@peff.net>
Date: Mar 22, 2015, 09:59 UTC
Message-ID: <20150322095924.GA24651@peff.net>
In-Reply-To: <20150322074455.GA1303@peff.net>

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

Show 13 quoted lines

> diff --git a/wt-status.c b/wt-status.c
> index 7036fa2..96f0033 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -748,9 +748,9 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
>  	fflush(s->fp);
>  	sm_summary.out = -1;
>  
> -	run_command(&sm_summary);
> -
> +	start_command(&sm_summary);
>  	len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);
> +	finish_command(&sm_summary);

This isn't quite right. In both the original and the new one, if start_command fails, we may call strbuf_read on whatever it happens to have left in sm_summary.out. Worse, in the new one, we would call finish_command on something that was never started. And in both old and new, we leak the sm_summary.out descriptor.

Furthermore, I found two other potential deadlocks (and one more descriptor leak). I tried at first to fix them all up like I did with the above patch, but I found that I was introducing similar problems in each case. So instead, I pulled this pattern out into its own strbuf helper.

So here's a replacement series.

  [1/7]: wt-status: don't flush before running "submodule status"
  [2/7]: wt_status: fix signedness mismatch in strbuf_read call
  [3/7]: strbuf: introduce strbuf_read_cmd helper
  [4/7]: wt-status: use strbuf_read_cmd
  [5/7]: submodule: use strbuf_read_cmd
  [6/7]: trailer: use strbuf_read_cmd
  [7/7]: run-command: forbid using run_command with piped output

-Peff