{"thread":{"id":"23601","subject":"[PATCH] start_command: close cmd->err descriptor when fork/spawn fails","startedAt":"2010-04-26T01:15:40Z","lastAt":"2010-04-26T23:58:24Z","messageCount":6,"participants":["bert Dvornik","Johannes Sixt","Johannes Schindelin","Albert Dvornik","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"140375","messageId":"1272244540-5960-1-git-send-email-dvornik+git@gmail.com","threadId":"23601","inReplyTo":null,"subject":"[PATCH] start_command: close cmd->err descriptor when fork/spawn fails","fromName":"bert Dvornik","fromEmail":"dvornik+git@gmail.com","sentAt":"2010-04-26T01:15:40Z","receivedAt":"2010-04-26T01:15:40Z","isPatch":true,"sender":{"key":"dvornik+git@gmail.com","avatar":null},"body":"Fix the problem where the cmd->err passed into start_command wasn't\nbeing properly closed when certain types of errors occurr.  (Compare\nthe affected code with the clean shutdown code later in the function.)\n\nOn Windows, this problem would be triggered if mingw_spawnvpe()\nfailed, which would happen if the command to be executed was malformed\n(e.g. a text file that didn't start with a #! line).  If cmd->err was\na pipe, the failure to close it could result in a hang while the other\nside was waiting (forever) for either input or pipe close, e.g. while\ntrying to shove the output into the side band.  On msysGit, this\nproblem was causing a hang in t5516-fetch-push.\n\nI'm not sure why (or if) this problem hasn't cropped up under Linux.\nThe non-Windows code *does* try to check for execve() failures in the\nchild, in addition to the fork() failures.\n\nSigned-off-by: bert Dvornik <dvornik+git@gmail.com>\n---\n run-command.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d1a8ceb..41666ac 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -383,6 +383,8 @@ fail_pipe:\n \t\t\tclose(cmd->out);\n \t\tif (need_err)\n \t\t\tclose_pair(fderr);\n+\t\telse if (cmd->err)\n+\t\t\tclose(cmd->err);\n \t\terrno = failed_errno;\n \t\treturn -1;\n \t}\n-- \n1.7.1.rc1.1794.g4bea1\n"},{"id":"140388","messageId":"4BD52FCB.6020902@viscovery.net","threadId":"23601","inReplyTo":"1272244540-5960-1-git-send-email-dvornik+git@gmail.com","subject":"Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-04-26T06:16:43Z","receivedAt":"2010-04-26T06:16:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/26/2010 3:15, schrieb bert Dvornik:\n> Fix the problem where the cmd->err passed into start_command wasn't\n> being properly closed when certain types of errors occurr.  (Compare\n> the affected code with the clean shutdown code later in the function.)\n\n> diff --git a/run-command.c b/run-command.c\n> index d1a8ceb..41666ac 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -383,6 +383,8 @@ fail_pipe:\n>  \t\t\tclose(cmd->out);\n>  \t\tif (need_err)\n>  \t\t\tclose_pair(fderr);\n> +\t\telse if (cmd->err)\n> +\t\t\tclose(cmd->err);\n>  \t\terrno = failed_errno;\n>  \t\treturn -1;\n>  \t}\n\nGood catch! This should go on maint (1.7.0).\n\nAcked-by: Johannes Sixt <j6t@kdbg.org>\n\n-- Hannes\n"},{"id":"140389","messageId":"alpine.DEB.1.00.1004260828470.7651@pacific.mpi-cbg.de","threadId":"23601","inReplyTo":"1272244540-5960-1-git-send-email-dvornik+git@gmail.com","subject":"Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-04-26T06:29:32Z","receivedAt":"2010-04-26T06:29:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 25 Apr 2010, bert Dvornik wrote:\n\n> Fix the problem where the cmd->err passed into start_command wasn't \n> being properly closed when certain types of errors occurr.  (Compare the \n> affected code with the clean shutdown code later in the function.)\n> \n> On Windows, this problem would be triggered if mingw_spawnvpe() failed, \n> which would happen if the command to be executed was malformed (e.g. a \n> text file that didn't start with a #! line).  If cmd->err was a pipe, \n> the failure to close it could result in a hang while the other side was \n> waiting (forever) for either input or pipe close, e.g. while trying to \n> shove the output into the side band.  On msysGit, this problem was \n> causing a hang in t5516-fetch-push.\n> \n> I'm not sure why (or if) this problem hasn't cropped up under Linux. The \n> non-Windows code *does* try to check for execve() failures in the child, \n> in addition to the fork() failures.\n\nThanks. Both patches applied and pushed to 4msysgit.git.\n\nCiao,\nDscho\n"},{"id":"140391","messageId":"4BD53749.6040708@viscovery.net","threadId":"23601","inReplyTo":"1272244540-5960-1-git-send-email-dvornik+git@gmail.com","subject":"Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-04-26T06:48:41Z","receivedAt":"2010-04-26T06:48:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/26/2010 3:15, schrieb bert Dvornik:\n> Fix the problem where the cmd->err passed into start_command wasn't\n> being properly closed when certain types of errors occurr.  (Compare\n> the affected code with the clean shutdown code later in the function.)\n> \n> On Windows, this problem would be triggered if mingw_spawnvpe()\n> failed, which would happen if the command to be executed was malformed\n> (e.g. a text file that didn't start with a #! line).  If cmd->err was\n> a pipe, the failure to close it could result in a hang while the other\n> side was waiting (forever) for either input or pipe close, e.g. while\n> trying to shove the output into the side band.  On msysGit, this\n> problem was causing a hang in t5516-fetch-push.\n> \n> I'm not sure why (or if) this problem hasn't cropped up under Linux.\n> The non-Windows code *does* try to check for execve() failures in the\n> child, in addition to the fork() failures.\n\nThe problem does show up on Linux if you trigger the right error, such as\nwith this patch:\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2de98e6..2515a96 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -528,7 +528,8 @@ test_expect_success 'push does not update local refs\n \tmk_test heads/master &&\n \tmk_child child &&\n \tmkdir testrepo/.git/hooks &&\n-\techo exit 1 >testrepo/.git/hooks/pre-receive &&\n+\techo \"#!/bin/frobnicuty\" >testrepo/.git/hooks/pre-receive &&\n+\techo exit 1 >>testrepo/.git/hooks/pre-receive &&\n \tchmod +x testrepo/.git/hooks/pre-receive &&\n \t(cd child &&\n \t\tgit pull .. master\n"},{"id":"140411","messageId":"z2n9f74dc2c1004260447y12b522d8if37cad56548738f1@mail.gmail.com","threadId":"23601","inReplyTo":"4BD53749.6040708@viscovery.net","subject":"Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails","fromName":"Albert Dvornik","fromEmail":"dvornik+git@gmail.com","sentAt":"2010-04-26T11:47:10Z","receivedAt":"2010-04-26T11:47:10Z","isPatch":true,"sender":{"key":"dvornik+git@gmail.com","avatar":null},"body":"On Mon, Apr 26, 2010 at 2:48 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 4/26/2010 3:15, schrieb bert Dvornik:\n[...]\n>> I'm not sure why (or if) this problem hasn't cropped up under Linux.\n>> The non-Windows code *does* try to check for execve() failures in the\n>> child, in addition to the fork() failures.\n>\n> The problem does show up on Linux if you trigger the right error, such as\n> with this patch:\n[...]\n> +       echo \"#!/bin/frobnicuty\" >testrepo/.git/hooks/pre-receive &&\n> +       echo exit 1 >>testrepo/.git/hooks/pre-receive &&\n\nAh, that makes sense!  Thank you for clearing up the mystery.\n\nJunio: would you like me to resend with an updated commit message, and\nmaybe the addition of Hannes's test as well?\n\n--bert\n"},{"id":"140462","messageId":"7vtyqxix5r.fsf@alter.siamese.dyndns.org","threadId":"23601","inReplyTo":"z2n9f74dc2c1004260447y12b522d8if37cad56548738f1@mail.gmail.com","subject":"Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-26T23:58:24Z","receivedAt":"2010-04-26T23:58:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Albert Dvornik <dvornik+git@gmail.com> writes:\n\n> Junio: would you like me to resend with an updated commit message, and\n> maybe the addition of Hannes's test as well?\n\nI am out of town right now and suspect my mail reception for coming few\ndays would be leaky --- as Dscho seems on top of this series, I suspect\nthat the patch flow would be more reliable if I get your change via a pull\nrequest from him when I return.\n\nThanks.\n"}]}