git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 7/7] run-command: forbid using run_command with piped output

From
Jeff King <peff@peff.net>
Date
Mar 23, 2015, 03:54 UTC
Message-ID
<20150323035405.GG30337@peff.net>
In-Reply-To
<20150323035302.GA30279@peff.net>

Because run_command both spawns and wait()s for the command before returning control to the caller, any reads from the pipes we open must necessarily happen after wait() returns. This can lead to deadlock, as the child process may block on writing to us while we are blocked waiting for it to exit.

Worse, it only happens when the child fills the pipe buffer, which means that the problem may come and go depending on the platform and the size of the output produced by the child.

Let's detect and flag this dangerous construct so that we can catch potential bugs early in the test suite rather than having them happen in the field.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/run-command.c b/run-command.c
index e591d2c..aad03ab 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 || cmd->err < 0)
+		die("BUG: run_command with a pipe can cause deadlock");
+
+	code = start_command(cmd);
 	if (code)
 		return code;
 	return finish_command(cmd);
-- 
2.3.3.618.ga041503
Previous: Jeff KingNext: Junio C Hamano
Message 18 of 26 in “status hangs trying to get submodule summary”
  1. Wincent ColaiutaMar 22, 2015
  2. status: read submodule process output before calling wait()Jeff King, Mar 22, 2015
  3. Jeff KingMar 22, 2015
  4. 0/7 introduce strbuf_read_cmd to avoid deadlocksJeff King, Mar 22, 2015
  5. 1/7 wt-status: don't flush before running "submodule status"Jeff King, Mar 22, 2015
  6. 2/7 wt_status: fix signedness mismatch in strbuf_read callJeff King, Mar 22, 2015
  7. 3/7 strbuf: introduce strbuf_read_cmd helperJeff King, Mar 22, 2015
  8. Eric SunshineMar 22, 2015
  9. Junio C HamanoMar 22, 2015
  10. Junio C HamanoMar 22, 2015
  11. 0/7 introduce capture_command to avoid deadlocksJeff King, Mar 23, 2015
  12. 1/7 wt-status: don't flush before running "submodule status"Jeff King, Mar 23, 2015
  13. 2/7 wt_status: fix signedness mismatch in strbuf_read callJeff King, Mar 23, 2015
  14. 3/7 run-command: introduce capture_command helperJeff King, Mar 23, 2015
  15. 4/7 wt-status: use capture_commandJeff King, Mar 23, 2015
  16. 5/7 submodule: use capture_commandJeff King, Mar 23, 2015
  17. 6/7 trailer: use capture_commandJeff King, Mar 23, 2015
  18. 7/7 run-command: forbid using run_command with piped outputJeff King, Mar 23, 2015
  19. Junio C HamanoMar 23, 2015
  20. Jeff KingMar 22, 2015
  21. Junio C HamanoMar 22, 2015
  22. Jeff KingMar 22, 2015
  23. 4/7 wt-status: use strbuf_read_cmdJeff King, Mar 22, 2015
  24. 5/7 submodule: use strbuf_read_cmdJeff King, Mar 22, 2015
  25. 6/7 trailer: use strbuf_read_cmdJeff King, Mar 22, 2015
  26. 7/7 run-command: forbid using run_command with piped outputJeff King, Mar 22, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.