{"thread":{"id":"38881","subject":"status hangs trying to get submodule summary","startedAt":"2015-03-22T05:56:54Z","lastAt":"2015-03-23T04:40:45Z","messageCount":26,"participants":["Wincent Colaiuta","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"258274","messageId":"CAPx-p4sKD0Nut3E1jnWfPPx4=--ZOxBgiVdt3RRb5tktw31qDg@mail.gmail.com","threadId":"38881","inReplyTo":null,"subject":"status hangs trying to get submodule summary","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2015-03-22T05:56:54Z","receivedAt":"2015-03-22T05:56:54Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"Hi,\n\nI just ran into some odd behavior trying to update a submodule in one\nof my projects where it would hang indefinitely trying to run either\n`git status` or `git commit`.\n\nHere's the minimal repro recipe:\n\nmkdir demo\ncd demo\ngit init\ngit submodule add git://github.com/ansible/ansible.git ansible\n(cd ansible && git checkout v1.6.10)\ngit add ansible\ngit commit -m \"initial commit\"\n(cd ansible && git checkout v1.8.4)\ngit config status.submodulesummary true\ngit status # hangs...\ngit commit # hangs...\n\nAt the time `git status` is hanging, these are the Git processes\nrunning on the system:\n\n12431 git status\n12462 git submodule summary --files --for-status --summary-limit -1\n12463 /bin/sh /usr/libexec/git-core/git-submodule summary --files\n--for-status --summary-limit -1\n12507 /bin/sh /usr/libexec/git-core/git-submodule summary --files\n--for-status --summary-limit -1\n12522 git log --pretty=format: %m %s --first-parent\n8959338284f6c6e44890b8911434285848f34859...ebc8d48d34296fe010096f044e2b7591df37a622\n\nAnd `strace` shows the processes `wait4`-ing, except for the `git log`\none which is doing a `write` but apparently blocked:\n\n# strace -p 12431\nProcess 12431 attached\nwait4(12462, ^CProcess 12431 detached\n \n# strace -p 12462\nProcess 12462 attached\nwait4(12463, ^CProcess 12462 detached\n \n# strace -p 12463\nProcess 12463 attached\nwait4(-1, ^CProcess 12463 detached\n \n# strace -p 12507\nProcess 12507 attached\nwait4(-1, ^CProcess 12507 detached\n \n# strace -p 12522\nProcess 12522 attached\nwrite(1, \"\\n > Merge pull request #7814 fr\"..., 108^CProcess 12522 detached\n \n\nThis repros for me on Mac OS X 10.10.2 with Git 1.9.5 and Git 2.3.3,\nand on the Amazon LInux (a RHEL-like OS) with Git 2.1.0. Both of these\nwith an empty .gitconfig (other than user.email, user.name and the\nstatus.submodulesummary value already mentioned above).\n\nI've never seen this hang before despite frequent use of submodules.\nOddly, I was able to work around the hang by moving the submodule in\ntwo hops (one from Ansible v1.6.10 to v1.7.0, then from v1.7.0 to\nv1.8.4). I am not sure if this is specific to the Ansible repo, or\nwhether the length of the summary is crossing some threshold that\ntriggers the bug to manifest. If I run the forked commands manually\nfrom an interactive shell, they complete just fine.\n\n-Greg\n"},{"id":"258276","messageId":"20150322074455.GA1303@peff.net","threadId":"38881","inReplyTo":"CAPx-p4sKD0Nut3E1jnWfPPx4=--ZOxBgiVdt3RRb5tktw31qDg@mail.gmail.com","subject":"[PATCH] status: read submodule process output before calling wait()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T07:44:56Z","receivedAt":"2015-03-22T07:44:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2015 at 10:56:54PM -0700, Wincent Colaiuta wrote:\n\n> I've never seen this hang before despite frequent use of submodules.\n> Oddly, I was able to work around the hang by moving the submodule in\n> two hops (one from Ansible v1.6.10 to v1.7.0, then from v1.7.0 to\n> v1.8.4). I am not sure if this is specific to the Ansible repo, or\n> whether the length of the summary is crossing some threshold that\n> triggers the bug to manifest. If I run the forked commands manually\n> from an interactive shell, they complete just fine.\n\nIt's the length of the summary. The fix is below.\n\n-- >8 --\nThe status code tries to read the output of \"git submodule\nsummary\" over a pipe by waiting for the program to finish\nand then reading its output, like this:\n\n run_command(&sm_summary);\n len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n\nBesides being a violation of the run-command API (which\nmakes no promises about the state of the struct after\nrun_command returns), this can easily lead to deadlock. The\n\"submodule status\" process may fill up the pipe buffer and\nblock on write(). Meanwhile, the reading side in the parent\nprocess is blocked in wait(), waiting for the child to\nfinish.\n\nInstead, we should start the process, read everything it\nproduces, and only then call wait() to finish it off.\n\nSigned-off-by: Jeff King \n---\nI notice that we also don't detect when the sub-command fails. I don't\nknow what we would do in that case (abort the status? print a message?)\nand it's orthogonal to this issue, so I left it for somebody more\nclueful in the area to think about.\n\n wt-status.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 7036fa2..96f0033 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -748,9 +748,9 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tfflush(s->fp);\n \tsm_summary.out = -1;\n \n-\trun_command(&sm_summary);\n-\n+\tstart_command(&sm_summary);\n \tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n+\tfinish_command(&sm_summary);\n \n \t/* prepend header, only if there's an actual output */\n \tif (len) {\n-- \n2.3.3.618.ga041503\n"},{"id":"258277","messageId":"20150322080708.GA3456@peff.net","threadId":"38881","inReplyTo":"20150322074455.GA1303@peff.net","subject":"Re: [PATCH] status: read submodule process output before calling wait()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T08:07:08Z","receivedAt":"2015-03-22T08:07:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2015 at 03:44:55AM -0400, Jeff King wrote:\n\n> The status code tries to read the output of \"git submodule\n> summary\" over a pipe by waiting for the program to finish\n> and then reading its output, like this:\n> \n> run_command(&sm_summary);\n> len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n\nBy the way, I spotted this code as bogus immediately upon seeing it\n(though certainly it helped to know there was a deadlock in the area,\nwhich had me thinking about such things). So I wondered if it could have\nbeen easy to catch in review, but its introduction was a little bit\nsubtle.\n\nThe original run_command invocation came in ac8d5af (builtin-status:\nsubmodule summary support, 2008-04-12), which just let the sub-process\ndump its stdout to the same descriptor that the rest of the status\noutput was going to. So the use of run_command there was fine. It was\nlater, in 3ba7407 (submodule summary: ignore --for-status option,\n2013-09-06), that we started post-processing the output and it became\nbuggy. But that's harder to see in review.\n\n> Besides being a violation of the run-command API (which\n> makes no promises about the state of the struct after\n> run_command returns),\n\nThis may be overly harsh of me. Certainly we make no guarantees (and\nthings like the dynamic \"args\" and \"env_array\" are cleaned up\nautomatically after finish_command returns), but I would not be\nsurprised if there are other spots that treat \"struct child_process\" as\ntransparent rather than as a black box.\n\nIt's really the run_command + pipe construct that is really the danger\nhere. I wonder if we should do something like this:\n\ndiff --git a/run-command.c b/run-command.c\nindex 3afb124..78807de 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -557,7 +557,12 @@ int finish_command(struct child_process *cmd)\n \n int run_command(struct child_process *cmd)\n {\n-\tint code = start_command(cmd);\n+\tint code;\n+\n+\tif (cmd->out < 0)\n+\t\tdie(\"BUG: run_command with a pipe can cause deadlock\");\n+\n+\tcode = start_command(cmd);\n \tif (code)\n \t\treturn code;\n \treturn finish_command(cmd);\n\nIt seems to catch at least one other dubious construct.\n\n-Peff\n"},{"id":"258280","messageId":"20150322095924.GA24651@peff.net","threadId":"38881","inReplyTo":"20150322074455.GA1303@peff.net","subject":"[PATCH 0/7] introduce strbuf_read_cmd to avoid deadlocks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T09:59:25Z","receivedAt":"2015-03-22T09:59:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2015 at 03:44:55AM -0400, Jeff King wrote:\n\n> diff --git a/wt-status.c b/wt-status.c\n> index 7036fa2..96f0033 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -748,9 +748,9 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n> \tfflush(s->fp);\n> \tsm_summary.out = -1;\n> \n> -\trun_command(&sm_summary);\n> -\n> +\tstart_command(&sm_summary);\n> \tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n> +\tfinish_command(&sm_summary);\n\nThis isn't quite right. In both the original and the new one, if\nstart_command fails, we may call strbuf_read on whatever it happens to\nhave left in sm_summary.out. Worse, in the new one, we would call\nfinish_command on something that was never started. And in both old and\nnew, we leak the sm_summary.out descriptor.\n\nFurthermore, I found two other potential deadlocks (and one more\ndescriptor leak). I tried at first to fix them all up like I did with\nthe above patch, but I found that I was introducing similar problems in\neach case. So instead, I pulled this pattern out into its own strbuf\nhelper.\n\nSo here's a replacement series.\n\n [1/7]: wt-status: don't flush before running \"submodule status\"\n [2/7]: wt_status: fix signedness mismatch in strbuf_read call\n [3/7]: strbuf: introduce strbuf_read_cmd helper\n [4/7]: wt-status: use strbuf_read_cmd\n [5/7]: submodule: use strbuf_read_cmd\n [6/7]: trailer: use strbuf_read_cmd\n [7/7]: run-command: forbid using run_command with piped output\n\n-Peff\n"},{"id":"258281","messageId":"20150322100001.GA11615@peff.net","threadId":"38881","inReplyTo":"20150322095924.GA24651@peff.net","subject":"[PATCH 1/7] wt-status: don't flush before running \"submodule status\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T10:00:01Z","receivedAt":"2015-03-22T10:00:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a holdover from the original implementation in\nac8d5af (builtin-status: submodule summary support,\n2008-04-12), which just had the sub-process output to our\ndescriptor; we had to make sure we had flushed any data that\nwe produced before it started writing.\n\nSince 3ba7407 (submodule summary: ignore --for-status\noption, 2013-09-06), however, we pipe the sub-process output\nback to ourselves. So there's no longer any need to flush\n(it does not hurt, but it may leave readers wondering why we\ndo it).\n\nSigned-off-by: Jeff King \n---\n wt-status.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 7036fa2..1712762 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -745,7 +745,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n-\tfflush(s->fp);\n \tsm_summary.out = -1;\n \n \trun_command(&sm_summary);\n-- \n2.3.3.618.ga041503\n"},{"id":"258282","messageId":"20150322100032.GB11615@peff.net","threadId":"38881","inReplyTo":"20150322095924.GA24651@peff.net","subject":"[PATCH 2/7] wt_status: fix signedness mismatch in strbuf_read call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T10:00:32Z","receivedAt":"2015-03-22T10:00:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We call strbuf_read(), and want to know whether we got any\noutput. To do so, we assign the result to a size_t, and\ncheck whether it is non-zero.\n\nBut strbuf_read returns a signed ssize_t. If it encounters\nan error, it will return -1, and we'll end up treating this\nthe same as if we had gotten output. Instead, we can just\ncheck whether our buffer has anything in it (which is what\nwe care about anyway, and is the same thing since we know\nthe buffer was empty to begin with).\n\nNote that the \"len\" variable actually has two roles in this\nfunction. Now that we've eliminated the first, we can push the\ndeclaration closer to the point of use for the second one.\n\nSigned-off-by: Jeff King \n---\n wt-status.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 1712762..b47f6d9 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -729,7 +729,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tstruct strbuf cmd_stdout = STRBUF_INIT;\n \tstruct strbuf summary = STRBUF_INIT;\n \tchar *summary_content;\n-\tsize_t len;\n \n \targv_array_pushf(&sm_summary.env_array, \"GIT_INDEX_FILE=%s\",\n \t\t\t s->index_file);\n@@ -749,10 +748,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \n \trun_command(&sm_summary);\n \n-\tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n+\tstrbuf_read(&cmd_stdout, sm_summary.out, 1024);\n \n \t/* prepend header, only if there's an actual output */\n-\tif (len) {\n+\tif (cmd_stdout.len) {\n \t\tif (uncommitted)\n \t\t\tstrbuf_addstr(&summary, _(\"Submodules changed but not updated:\"));\n \t\telse\n@@ -763,6 +762,7 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tstrbuf_release(&cmd_stdout);\n \n \tif (s->display_comment_prefix) {\n+\t\tsize_t len;\n \t\tsummary_content = strbuf_detach(&summary, &len);\n \t\tstrbuf_add_commented_lines(&summary, summary_content, len);\n \t\tfree(summary_content);\n-- \n2.3.3.618.ga041503\n"},{"id":"258283","messageId":"20150322100724.GC11615@peff.net","threadId":"38881","inReplyTo":"20150322095924.GA24651@peff.net","subject":"[PATCH 3/7] strbuf: introduce strbuf_read_cmd helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T10:07:25Z","receivedAt":"2015-03-22T10:07:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Something as simple as reading the stdout from a command\nturns out to be rather hard to do right. Doing:\n\n if (!run_command(&cmd))\n strbuf_read(&buf, cmd.out, 0);\n\ncan result in deadlock if the child process produces a large\namount of output. What happens is:\n\n 1. The parent spawns the child with its stdout connected\n to a pipe, of which the parent is the sole reader.\n\n 2. The parent calls wait(), blocking until the child exits.\n\n 3. The child writes to stdout. If it writes more data than\n the OS pipe buffer can hold, the write() call will\n block.\n\nThis is a deadlock; the parent is waiting for the child to\nexit, and the child is waiting for the parent to call\nread().\n\nSo we should do instead:\n\n if (!start_command(&cmd)) {\n strbuf_read(&buf, cmd.out, 0);\n finish_command(&cmd);\n }\n\nBut note that this leaks cmd.out (which must be closed). And\nthere's no error handling for strbuf_read. We probably want\nto know whether the operation succeeded, but we must make\nsure to always run finish_command even if the read failed\n(or else we leave a zombie child process).\n\nLet's introduce a strbuf helper that can make this a bit\nsimpler for callers to do right.\n\nSigned-off-by: Jeff King \n---\nThis is really at the intersection of the strbuf and\nrun-command APIs, so you could argue for it being part of\neither It is logically quite like the strbuf_read_file()\nfunction, so I put it there.\n\n strbuf.c | 17 +++++++++++++++++\n strbuf.h | 10 ++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 88cafd4..9d1d48f 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"refs.h\"\n #include \"utf8.h\"\n+#include \"run-command.h\"\n \n int starts_with(const char *str, const char *prefix)\n {\n@@ -414,6 +415,22 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n \treturn -1;\n }\n \n+int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint)\n+{\n+\tcmd->out = -1;\n+\tif (start_command(cmd) < 0)\n+\t\treturn -1;\n+\n+\tif (strbuf_read(sb, cmd->out, hint) < 0) {\n+\t\tclose(cmd->out);\n+\t\tfinish_command(cmd); /* throw away exit code */\n+\t\treturn -1;\n+\t}\n+\n+\tclose(cmd->out);\n+\treturn finish_command(cmd);\n+}\n+\n int strbuf_getcwd(struct strbuf *sb)\n {\n \tsize_t oldalloc = sb->alloc;\ndiff --git a/strbuf.h b/strbuf.h\nindex 1883494..93a50da 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -1,6 +1,8 @@\n #ifndef STRBUF_H\n #define STRBUF_H\n \n+struct child_process;\n+\n /**\n * strbuf's are meant to be used with all the usual C string and memory\n * APIs. Given that the length of the buffer is known, it's often better to\n@@ -373,6 +375,14 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n \n /**\n+ * Execute the given command, capturing its stdout in the given strbuf.\n+ * Returns -1 if starting the command fails or reading fails, and otherwise\n+ * returns the exit code of the command. The output collected in the\n+ * buffer is kept even if the command returns a non-zero exit.\n+ */\n+int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint);\n+\n+/**\n * Read a line from a FILE *, overwriting the existing contents\n * of the strbuf. The second argument specifies the line\n * terminator character, typically `'\\n'`.\n-- \n2.3.3.618.ga041503\n"},{"id":"258284","messageId":"20150322100829.GD11615@peff.net","threadId":"38881","inReplyTo":"20150322095924.GA24651@peff.net","subject":"[PATCH 4/7] wt-status: use strbuf_read_cmd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T10:08:29Z","receivedAt":"2015-03-22T10:08:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we spawn \"git submodule status\" to read its output, we\nuse run_command() followed by a strbuf_read() from a pipe.\nThis can deadlock if the subprocess output is larger than\nthe system pipe buffer.\n\nFurthermore, if start_command() fails, we'll try to read\nfrom a bogus descriptor (probably \"-1\" or a descriptor we\njust closed, but it is a bad idea for us to make assumptions\nabout how start_command implements its error handling). And\nif start_command succeeds, we leak the file descriptor for\nthe pipe to the child.\n\nAll of these can be solved by using the strbuf_read_cmd\nhelper.\n\nSigned-off-by: Jeff King \n---\n wt-status.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex b47f6d9..fd85d61 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -744,11 +744,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n-\tsm_summary.out = -1;\n \n-\trun_command(&sm_summary);\n-\n-\tstrbuf_read(&cmd_stdout, sm_summary.out, 1024);\n+\tstrbuf_read_cmd(&cmd_stdout, &sm_summary, 1024);\n \n \t/* prepend header, only if there's an actual output */\n \tif (cmd_stdout.len) {\n-- \n2.3.3.618.ga041503\n"},{"id":"258287","messageId":"20150322100858.GE11615@peff.net","threadId":"38881","inReplyTo":"20150322095924.GA24651@peff.net","subject":"[PATCH 5/7] submodule: use strbuf_read_cmd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T10:08:59Z","receivedAt":"2015-03-22T10:08:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In is_submodule_commit_present, we call run_command followed\nby a pipe read, which is prone to deadlock. It is unlikely\nto happen in this case, as rev-list should never produce\nmore than a single line of output, but it does not hurt to\navoid an anti-pattern (and using the helper simplifies the\nsetup and cleanup).\n\nSigned-off-by: Jeff King \n---\n submodule.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d37d400..d85f1bb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -576,12 +576,10 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n \t\tcp.env = local_repo_env;\n \t\tcp.git_cmd = 1;\n \t\tcp.no_stdin = 1;\n-\t\tcp.out = -1;\n \t\tcp.dir = path;\n-\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))\n+\t\tif (!strbuf_read_cmd(&buf, &cp, 1024) && !buf.len)\n \t\t\tis_present = 1;\n \n-\t\tclose(cp.out);\n \t\tstrbuf_release(&buf);\n \t}\n \treturn is_present;\n-- \n2.3.3.618.ga041503\n"},{"id":"258285","messageId":"20150322100925.GF11615@peff.net","threadId":"38881","inReplyTo":"20150322095924.GA24651@peff.net","subject":"[PATCH 6/7] trailer: use strbuf_read_cmd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T10:09:25Z","receivedAt":"2015-03-22T10:09:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read from a trailer.*.command sub-program, the\ncurrent code uses run_command followed by a pipe read, which\ncan result in deadlock (though in practice you would have to\nhave a large trailer for this to be a problem). The current\ncode also leaks the file descriptor for the pipe to the\nsub-command.\n\nInstead, let's use strbuf_read_cmd, which makes this simpler\n(and we can get rid of our custom helper).\n\nSigned-off-by: Jeff King \n---\n trailer.c | 18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 05b3859..b47bc0e 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -214,16 +214,6 @@ static struct trailer_item *remove_first(struct trailer_item **first)\n \treturn item;\n }\n \n-static int read_from_command(struct child_process *cp, struct strbuf *buf)\n-{\n-\tif (run_command(cp))\n-\t\treturn error(\"running trailer command '%s' failed\", cp->argv[0]);\n-\tif (strbuf_read(buf, cp->out, 1024) < 1)\n-\t\treturn error(\"reading from trailer command '%s' failed\", cp->argv[0]);\n-\tstrbuf_trim(buf);\n-\treturn 0;\n-}\n-\n static const char *apply_command(const char *command, const char *arg)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -240,14 +230,16 @@ static const char *apply_command(const char *command, const char *arg)\n \tcp.argv = argv;\n \tcp.env = local_repo_env;\n \tcp.no_stdin = 1;\n-\tcp.out = -1;\n \tcp.use_shell = 1;\n \n-\tif (read_from_command(&cp, &buf)) {\n+\tif (strbuf_read_cmd(&buf, &cp, 1024)) {\n+\t\terror(\"running trailer command '%s' failed\", cmd.buf);\n \t\tstrbuf_release(&buf);\n \t\tresult = xstrdup(\"\");\n-\t} else\n+\t} else {\n+\t\tstrbuf_trim(&buf);\n \t\tresult = strbuf_detach(&buf, NULL);\n+\t}\n \n \tstrbuf_release(&cmd);\n \treturn result;\n-- \n2.3.3.618.ga041503\n"},{"id":"258286","messageId":"20150322101043.GG11615@peff.net","threadId":"38881","inReplyTo":"20150322095924.GA24651@peff.net","subject":"[PATCH 7/7] run-command: forbid using run_command with piped output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T10:10:43Z","receivedAt":"2015-03-22T10:10:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because run_command both spawns and wait()s for the command\nbefore returning control to the caller, any reads from the\npipes we open must necessarily happen after wait() returns.\nThis can lead to deadlock, as the child process may block on\nwriting to us while we are blocked waiting for it to exit.\n\nWorse, it only happens when the child fills the pipe buffer,\nwhich means that the problem may come and go depending on\nthe platform and the size of the output produced by the\nchild.\n\nLet's detect and flag this dangerous construct so that we\ncan catch potential bugs early in the test suite rather than\nhaving them happen in the field.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3afb124..23636e0 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -557,7 +557,12 @@ int finish_command(struct child_process *cmd)\n \n int run_command(struct child_process *cmd)\n {\n-\tint code = start_command(cmd);\n+\tint code;\n+\n+\tif (cmd->out < 0 || cmd->err < 0)\n+\t\tdie(\"BUG: run_command with a pipe can cause deadlock\");\n+\n+\tcode = start_command(cmd);\n \tif (code)\n \t\treturn code;\n \treturn finish_command(cmd);\n-- \n2.3.3.618.ga041503\n"},{"id":"258308","messageId":"CAPig+cR5Ur4xOKZ6K=bOwOVM8bHHjJJXHxzCbvYBhqOTtD6dXg@mail.gmail.com","threadId":"38881","inReplyTo":"20150322100724.GC11615@peff.net","subject":"Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-22T19:36:01Z","receivedAt":"2015-03-22T19:36:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 22, 2015 at 6:07 AM, Jeff King wrote:\n> Something as simple as reading the stdout from a command\n> turns out to be rather hard to do right. Doing:\n>\n> if (!run_command(&cmd))\n> strbuf_read(&buf, cmd.out, 0);\n>\n> can result in deadlock if the child process produces a large\n> amount of output. [...]\n>\n> Let's introduce a strbuf helper that can make this a bit\n> simpler for callers to do right.\n>\n> Signed-off-by: Jeff King \n> ---\n> This is really at the intersection of the strbuf and\n> run-command APIs, so you could argue for it being part of\n> either It is logically quite like the strbuf_read_file()\n> function, so I put it there.\n\nIt does feel like a layering violation. If moved to the run-command\nAPI, it could given one of the following names or something better:\n\n run_command_capture()\n capture_command()\n command_capture()\n run_command_with_output()\n capture_output()\n\n> diff --git a/strbuf.h b/strbuf.h\n> index 1883494..93a50da 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -1,6 +1,8 @@\n> #ifndef STRBUF_H\n> #define STRBUF_H\n>\n> +struct child_process;\n> +\n> /**\n> * strbuf's are meant to be used with all the usual C string and memory\n> * APIs. Given that the length of the buffer is known, it's often better to\n> @@ -373,6 +375,14 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n> extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n>\n> /**\n> + * Execute the given command, capturing its stdout in the given strbuf.\n> + * Returns -1 if starting the command fails or reading fails, and otherwise\n> + * returns the exit code of the command. The output collected in the\n> + * buffer is kept even if the command returns a non-zero exit.\n> + */\n> +int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint);\n> +\n> +/**\n> * Read a line from a FILE *, overwriting the existing contents\n> * of the strbuf. The second argument specifies the line\n> * terminator character, typically `'\\n'`.\n> --\n> 2.3.3.618.ga041503\n"},{"id":"258324","messageId":"xmqqwq28bq3e.fsf@gitster.dls.corp.google.com","threadId":"38881","inReplyTo":"CAPig+cR5Ur4xOKZ6K=bOwOVM8bHHjJJXHxzCbvYBhqOTtD6dXg@mail.gmail.com","subject":"Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-22T22:54:29Z","receivedAt":"2015-03-22T22:54:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sun, Mar 22, 2015 at 6:07 AM, Jeff King wrote:\n>> Something as simple as reading the stdout from a command\n>> turns out to be rather hard to do right. Doing:\n>>\n>> if (!run_command(&cmd))\n>> strbuf_read(&buf, cmd.out, 0);\n>>\n>> can result in deadlock if the child process produces a large\n>> amount of output. [...]\n>>\n>> Let's introduce a strbuf helper that can make this a bit\n>> simpler for callers to do right.\n>>\n>> Signed-off-by: Jeff King \n>> ---\n>> This is really at the intersection of the strbuf and\n>> run-command APIs, so you could argue for it being part of\n>> either It is logically quite like the strbuf_read_file()\n>> function, so I put it there.\n>\n> It does feel like a layering violation. If moved to the run-command\n> API, it could given one of the following names or something better:\n>\n> run_command_capture()\n> capture_command()\n> command_capture()\n> run_command_with_output()\n> capture_output()\n\nSound like a good suggestion (but I haven't read the users of the\nproposed function, after doing which I might change my mind---I'll\nsee).\n\nThanks.\n"},{"id":"258325","messageId":"xmqqsicwbos5.fsf@gitster.dls.corp.google.com","threadId":"38881","inReplyTo":"20150322100724.GC11615@peff.net","subject":"Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-22T23:22:50Z","receivedAt":"2015-03-22T23:22:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/strbuf.h b/strbuf.h\n> index 1883494..93a50da 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -1,6 +1,8 @@\n> #ifndef STRBUF_H\n> #define STRBUF_H\n> \n> +struct child_process;\n> +\n> /**\n> * strbuf's are meant to be used with all the usual C string and memory\n> * APIs. Given that the length of the buffer is known, it's often better to\n> @@ -373,6 +375,14 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n> extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n> \n> /**\n> + * Execute the given command, capturing its stdout in the given strbuf.\n> + * Returns -1 if starting the command fails or reading fails, and otherwise\n> + * returns the exit code of the command. The output collected in the\n> + * buffer is kept even if the command returns a non-zero exit.\n> + */\n> +int strbuf_read_cmd(struct strbuf *sb, struct child_process *cmd, size_t hint);\n> +\n> +/**\n> * Read a line from a FILE *, overwriting the existing contents\n> * of the strbuf. The second argument specifies the line\n> * terminator character, typically `'\\n'`.\n\nIt is an unfortunate tangent that this is a bugfix that may want to\ngo to 'maint' and older, but our earlier jk/strbuf-doc-to-header\ntopic introduces an unnecessary merge conflicts.\n\nI've wiggled this part and moved the doc elsewhere, only to remove\nthat in the merge, which may not be optimal from the point of view\nof what I have to do when merging this topic down from pu to next\nto master to maint, but I do not see a good way around it.\n\nThanks. The whole series looks very sensible.\n"},{"id":"258329","messageId":"20150322233448.GA21518@peff.net","threadId":"38881","inReplyTo":"CAPig+cR5Ur4xOKZ6K=bOwOVM8bHHjJJXHxzCbvYBhqOTtD6dXg@mail.gmail.com","subject":"Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T23:34:49Z","receivedAt":"2015-03-22T23:34:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2015 at 03:36:01PM -0400, Eric Sunshine wrote:\n\n> > This is really at the intersection of the strbuf and\n> > run-command APIs, so you could argue for it being part of\n> > either It is logically quite like the strbuf_read_file()\n> > function, so I put it there.\n> \n> It does feel like a layering violation. If moved to the run-command\n> API, it could given one of the following names or something better:\n\nA layering violation implies there is an ordering to the APIs. Certainly\nwe call APIs from other APIs all the time. I guess you could argue that\nthese are the \"same\" layer, and should be next to each, and not building\non each other (i.e., that strbuf has dependencies only on system APIs\nlike stdio.h, and run-command only on system APIs like unistd.h, etc).\n\nBut then reversing the order of the dependency does not really solve\nthat. You would have to introduce a new higher-level API that combines\nthem. But that seems silly for a single function (and I do not foresee\nany other similar functions).\n\nThat being said, I'm not opposed to one of the reverse names if people\nfeel strongly (I also considered making it an option flag to\nrun_command_v_opt, but it ended up tangling things quite a bit more).\n\n-Peff\n"},{"id":"258330","messageId":"20150322233653.GB21518@peff.net","threadId":"38881","inReplyTo":"xmqqsicwbos5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-22T23:36:53Z","receivedAt":"2015-03-22T23:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2015 at 04:22:50PM -0700, Junio C Hamano wrote:\n\n> > +/**\n> > * Read a line from a FILE *, overwriting the existing contents\n> > * of the strbuf. The second argument specifies the line\n> > * terminator character, typically `'\\n'`.\n> \n> It is an unfortunate tangent that this is a bugfix that may want to\n> go to 'maint' and older, but our earlier jk/strbuf-doc-to-header\n> topic introduces an unnecessary merge conflicts.\n\nYeah, that is the worst part of refactoring and cleanup. Even when you\nmake sure you are not hurting any topics in flight, you cannot know when\na new topic will take off in your general area.\n\n> I've wiggled this part and moved the doc elsewhere, only to remove\n> that in the merge, which may not be optimal from the point of view\n> of what I have to do when merging this topic down from pu to next\n> to master to maint, but I do not see a good way around it.\n\nI'd suggest just dropping the documentation in the \"maint\" version\n(i.e., make it a moral cherry-pick of the function declaration only).\n\n-Peff\n"},{"id":"258331","messageId":"xmqqiodsbnyi.fsf@gitster.dls.corp.google.com","threadId":"38881","inReplyTo":"xmqqwq28bq3e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] strbuf: introduce strbuf_read_cmd helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-22T23:40:37Z","receivedAt":"2015-03-22T23:40:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> It does feel like a layering violation. If moved to the run-command\n>> API, it could given one of the following names or something better:\n>>\n>> run_command_capture()\n>> capture_command()\n>> command_capture()\n>> run_command_with_output()\n>> capture_output()\n>\n> Sound like a good suggestion (but I haven't read the users of the\n> proposed function, after doing which I might change my mind---I'll\n> see).\n\nNow I read the callers, it does look like this new function better\nfits in the run-command suite, essentially allowing us to do what we\nwould do with $(cmd) or `cmd` in shell and Perl scripts, even though\nI do not particularly agree with the phrase \"layering violation\" to\ncall its current placement.\n\nThanks.\n"},{"id":"258337","messageId":"20150323035302.GA30279@peff.net","threadId":"38881","inReplyTo":"xmqqiodsbnyi.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2 0/7] introduce capture_command to avoid deadlocks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:53:02Z","receivedAt":"2015-03-23T03:53:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2015 at 04:40:37PM -0700, Junio C Hamano wrote:\n\n> Now I read the callers, it does look like this new function better\n> fits in the run-command suite, essentially allowing us to do what we\n> would do with $(cmd) or `cmd` in shell and Perl scripts, even though\n> I do not particularly agree with the phrase \"layering violation\" to\n> call its current placement.\n\nI was on the fence, and you both seem to prefer it in run-command, so\nhere is a re-roll in that direction (no other changes).\n\n [1/7]: wt-status: don't flush before running \"submodule status\"\n [2/7]: wt_status: fix signedness mismatch in strbuf_read call\n [3/7]: run-command: introduce capture_command helper\n [4/7]: wt-status: use capture_command\n [5/7]: submodule: use capture_command\n [6/7]: trailer: use capture_command\n [7/7]: run-command: forbid using run_command with piped output\n\n-Peff\n"},{"id":"258338","messageId":"20150323035324.GA30337@peff.net","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"[PATCH v2 1/7] wt-status: don't flush before running \"submodule status\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:53:24Z","receivedAt":"2015-03-23T03:53:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a holdover from the original implementation in\nac8d5af (builtin-status: submodule summary support,\n2008-04-12), which just had the sub-process output to our\ndescriptor; we had to make sure we had flushed any data that\nwe produced before it started writing.\n\nSince 3ba7407 (submodule summary: ignore --for-status\noption, 2013-09-06), however, we pipe the sub-process output\nback to ourselves. So there's no longer any need to flush\n(it does not hurt, but it may leave readers wondering why we\ndo it).\n\nSigned-off-by: Jeff King \n---\n wt-status.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 7036fa2..1712762 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -745,7 +745,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n-\tfflush(s->fp);\n \tsm_summary.out = -1;\n \n \trun_command(&sm_summary);\n-- \n2.3.3.618.ga041503\n"},{"id":"258339","messageId":"20150323035329.GB30337@peff.net","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"[PATCH v2 2/7] wt_status: fix signedness mismatch in strbuf_read call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:53:29Z","receivedAt":"2015-03-23T03:53:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We call strbuf_read(), and want to know whether we got any\noutput. To do so, we assign the result to a size_t, and\ncheck whether it is non-zero.\n\nBut strbuf_read returns a signed ssize_t. If it encounters\nan error, it will return -1, and we'll end up treating this\nthe same as if we had gotten output. Instead, we can just\ncheck whether our buffer has anything in it (which is what\nwe care about anyway, and is the same thing since we know\nthe buffer was empty to begin with).\n\nNote that the \"len\" variable actually has two roles in this\nfunction. Now that we've eliminated the first, we can push the\ndeclaration closer to the point of use for the second one.\n\nSigned-off-by: Jeff King \n---\n wt-status.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 1712762..b47f6d9 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -729,7 +729,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tstruct strbuf cmd_stdout = STRBUF_INIT;\n \tstruct strbuf summary = STRBUF_INIT;\n \tchar *summary_content;\n-\tsize_t len;\n \n \targv_array_pushf(&sm_summary.env_array, \"GIT_INDEX_FILE=%s\",\n \t\t\t s->index_file);\n@@ -749,10 +748,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \n \trun_command(&sm_summary);\n \n-\tlen = strbuf_read(&cmd_stdout, sm_summary.out, 1024);\n+\tstrbuf_read(&cmd_stdout, sm_summary.out, 1024);\n \n \t/* prepend header, only if there's an actual output */\n-\tif (len) {\n+\tif (cmd_stdout.len) {\n \t\tif (uncommitted)\n \t\t\tstrbuf_addstr(&summary, _(\"Submodules changed but not updated:\"));\n \t\telse\n@@ -763,6 +762,7 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \tstrbuf_release(&cmd_stdout);\n \n \tif (s->display_comment_prefix) {\n+\t\tsize_t len;\n \t\tsummary_content = strbuf_detach(&summary, &len);\n \t\tstrbuf_add_commented_lines(&summary, summary_content, len);\n \t\tfree(summary_content);\n-- \n2.3.3.618.ga041503\n"},{"id":"258340","messageId":"20150323035343.GC30337@peff.net","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"[PATCH v2 3/7] run-command: introduce capture_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:53:43Z","receivedAt":"2015-03-23T03:53:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Something as simple as reading the stdout from a command\nturns out to be rather hard to do right. Doing:\n\n cmd.out = -1;\n run_command(&cmd);\n strbuf_read(&buf, cmd.out, 0);\n\ncan result in deadlock if the child process produces a large\namount of output. What happens is:\n\n 1. The parent spawns the child with its stdout connected\n to a pipe, of which the parent is the sole reader.\n\n 2. The parent calls wait(), blocking until the child exits.\n\n 3. The child writes to stdout. If it writes more data than\n the OS pipe buffer can hold, the write() call will\n block.\n\nThis is a deadlock; the parent is waiting for the child to\nexit, and the child is waiting for the parent to call\nread().\n\nSo we might try instead:\n\n start_command(&cmd);\n strbuf_read(&buf, cmd.out, 0);\n finish_command(&cmd);\n\nBut that is not quite right either. We are examining cmd.out\nand running finish_command whether start_command succeeded\nor not, which is wrong. Moreover, these snippets do not do\nany error handling. If our read() fails, we must make sure\nto still call finish_command (to reap the child process).\nAnd both snippets failed to close the cmd.out descriptor,\nwhich they must do (provided start_command succeeded).\n\nLet's introduce a run-command helper that can make this a\nbit simpler for callers to get right.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 16 ++++++++++++++++\n run-command.h | 13 +++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3afb124..e591d2c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -829,3 +829,19 @@ int run_hook_le(const char *const *env, const char *name, ...)\n \n \treturn ret;\n }\n+\n+int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)\n+{\n+\tcmd->out = -1;\n+\tif (start_command(cmd) < 0)\n+\t\treturn -1;\n+\n+\tif (strbuf_read(buf, cmd->out, hint) < 0) {\n+\t\tclose(cmd->out);\n+\t\tfinish_command(cmd); /* throw away exit code */\n+\t\treturn -1;\n+\t}\n+\n+\tclose(cmd->out);\n+\treturn finish_command(cmd);\n+}\ndiff --git a/run-command.h b/run-command.h\nindex d6868dc..263b966 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -71,6 +71,19 @@ int run_command_v_opt(const char **argv, int opt);\n */\n int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n \n+/**\n+ * Execute the given command, capturing its stdout in the given strbuf.\n+ * Returns -1 if starting the command fails or reading fails, and otherwise\n+ * returns the exit code of the command. The output collected in the\n+ * buffer is kept even if the command returns a non-zero exit. The hint field\n+ * gives a starting size for the strbuf allocation.\n+ *\n+ * The fields of \"cmd\" should be set up as they would for a normal run_command\n+ * invocation. But note that there is no need to set cmd->out; the function\n+ * sets it up for the caller.\n+ */\n+int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint);\n+\n /*\n * The purpose of the following functions is to feed a pipe by running\n * a function asynchronously and providing output that the caller reads.\n-- \n2.3.3.618.ga041503\n"},{"id":"258341","messageId":"20150323035352.GD30337@peff.net","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"[PATCH v2 4/7] wt-status: use capture_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:53:52Z","receivedAt":"2015-03-23T03:53:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we spawn \"git submodule status\" to read its output, we\nuse run_command() followed by strbuf_read() read from the\npipe. This can deadlock if the subprocess output is larger\nthan the system pipe buffer.\n\nFurthermore, if start_command() fails, we'll try to read\nfrom a bogus descriptor (probably \"-1\" or a descriptor we\njust closed, but it is a bad idea for us to make assumptions\nabout how start_command implements its error handling). And\nif start_command succeeds, we leak the file descriptor for\nthe pipe to the child.\n\nAll of these can be solved by using the capture_command\nhelper.\n\nSigned-off-by: Jeff King \n---\n wt-status.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex b47f6d9..853419f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -744,11 +744,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt\n \n \tsm_summary.git_cmd = 1;\n \tsm_summary.no_stdin = 1;\n-\tsm_summary.out = -1;\n \n-\trun_command(&sm_summary);\n-\n-\tstrbuf_read(&cmd_stdout, sm_summary.out, 1024);\n+\tcapture_command(&sm_summary, &cmd_stdout, 1024);\n \n \t/* prepend header, only if there's an actual output */\n \tif (cmd_stdout.len) {\n-- \n2.3.3.618.ga041503\n"},{"id":"258343","messageId":"20150323035356.GE30337@peff.net","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"[PATCH v2 5/7] submodule: use capture_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:53:56Z","receivedAt":"2015-03-23T03:53:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In is_submodule_commit_present, we call run_command followed\nby a pipe read, which is prone to deadlock. It is unlikely\nto happen in this case, as rev-list should never produce\nmore than a single line of output, but it does not hurt to\navoid an anti-pattern (and using the helper simplifies the\nsetup and cleanup).\n\nSigned-off-by: Jeff King \n---\n submodule.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d37d400..c0e6c81 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -576,12 +576,10 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n \t\tcp.env = local_repo_env;\n \t\tcp.git_cmd = 1;\n \t\tcp.no_stdin = 1;\n-\t\tcp.out = -1;\n \t\tcp.dir = path;\n-\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))\n+\t\tif (!capture_command(&cp, &buf, 1024) && !buf.len)\n \t\t\tis_present = 1;\n \n-\t\tclose(cp.out);\n \t\tstrbuf_release(&buf);\n \t}\n \treturn is_present;\n-- \n2.3.3.618.ga041503\n"},{"id":"258342","messageId":"20150323035400.GF30337@peff.net","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"[PATCH v2 6/7] trailer: use capture_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:54:00Z","receivedAt":"2015-03-23T03:54:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read from a trailer.*.command sub-program, the\ncurrent code uses run_command followed by a pipe read, which\ncan result in deadlock (though in practice you would have to\nhave a large trailer for this to be a problem). The current\ncode also leaks the file descriptor for the pipe to the\nsub-command.\n\nInstead, let's use capture_command, which makes this simpler\n(and we can get rid of our custom helper).\n\nSigned-off-by: Jeff King \n---\n trailer.c | 18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 05b3859..4b14a56 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -214,16 +214,6 @@ static struct trailer_item *remove_first(struct trailer_item **first)\n \treturn item;\n }\n \n-static int read_from_command(struct child_process *cp, struct strbuf *buf)\n-{\n-\tif (run_command(cp))\n-\t\treturn error(\"running trailer command '%s' failed\", cp->argv[0]);\n-\tif (strbuf_read(buf, cp->out, 1024) < 1)\n-\t\treturn error(\"reading from trailer command '%s' failed\", cp->argv[0]);\n-\tstrbuf_trim(buf);\n-\treturn 0;\n-}\n-\n static const char *apply_command(const char *command, const char *arg)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -240,14 +230,16 @@ static const char *apply_command(const char *command, const char *arg)\n \tcp.argv = argv;\n \tcp.env = local_repo_env;\n \tcp.no_stdin = 1;\n-\tcp.out = -1;\n \tcp.use_shell = 1;\n \n-\tif (read_from_command(&cp, &buf)) {\n+\tif (capture_command(&cp, &buf, 1024)) {\n+\t\terror(\"running trailer command '%s' failed\", cmd.buf);\n \t\tstrbuf_release(&buf);\n \t\tresult = xstrdup(\"\");\n-\t} else\n+\t} else {\n+\t\tstrbuf_trim(&buf);\n \t\tresult = strbuf_detach(&buf, NULL);\n+\t}\n \n \tstrbuf_release(&cmd);\n \treturn result;\n-- \n2.3.3.618.ga041503\n"},{"id":"258344","messageId":"20150323035405.GG30337@peff.net","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"[PATCH v2 7/7] run-command: forbid using run_command with piped output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-23T03:54:05Z","receivedAt":"2015-03-23T03:54:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because run_command both spawns and wait()s for the command\nbefore returning control to the caller, any reads from the\npipes we open must necessarily happen after wait() returns.\nThis can lead to deadlock, as the child process may block\non writing to us while we are blocked waiting for it to\nexit.\n\nWorse, it only happens when the child fills the pipe\nbuffer, which means that the problem may come and go\ndepending on the platform and the size of the output\nproduced by the child.\n\nLet's detect and flag this dangerous construct so that we\ncan catch potential bugs early in the test suite rather than\nhaving them happen in the field.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex e591d2c..aad03ab 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -557,7 +557,12 @@ int finish_command(struct child_process *cmd)\n \n int run_command(struct child_process *cmd)\n {\n-\tint code = start_command(cmd);\n+\tint code;\n+\n+\tif (cmd->out < 0 || cmd->err < 0)\n+\t\tdie(\"BUG: run_command with a pipe can cause deadlock\");\n+\n+\tcode = start_command(cmd);\n \tif (code)\n \t\treturn code;\n \treturn finish_command(cmd);\n-- \n2.3.3.618.ga041503\n"},{"id":"258345","messageId":"xmqqa8z4ba2a.fsf@gitster.dls.corp.google.com","threadId":"38881","inReplyTo":"20150323035302.GA30279@peff.net","subject":"Re: [PATCH v2 0/7] introduce capture_command to avoid deadlocks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T04:40:45Z","receivedAt":"2015-03-23T04:40:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; will queue.\n"}]}