On Sun, Mar 22, 2015 at 03:44:55AM -0400, Jeff King wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
