# [PATCH] start_command: close cmd->err descriptor when fork/spawn fails 6 messages from 2010-04-26 to 2010-04-26. Participants: bert Dvornik, Johannes Sixt, Johannes Schindelin, Albert Dvornik, Junio C Hamano. Thread: https://gitlist.dev/t/23601 ## bert Dvornik, 2010-04-26 01:15 Subject: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails Message-ID: <1272244540-5960-1-git-send-email-dvornik+git@gmail.com> URL: https://gitlist.dev/e/1272244540-5960-1-git-send-email-dvornik%2Bgit%40gmail.com ``` Fix the problem where the cmd->err passed into start_command wasn't being properly closed when certain types of errors occurr. (Compare the affected code with the clean shutdown code later in the function.) On Windows, this problem would be triggered if mingw_spawnvpe() failed, which would happen if the command to be executed was malformed (e.g. a text file that didn't start with a #! line). If cmd->err was a pipe, the failure to close it could result in a hang while the other side was waiting (forever) for either input or pipe close, e.g. while trying to shove the output into the side band. On msysGit, this problem was causing a hang in t5516-fetch-push. I'm not sure why (or if) this problem hasn't cropped up under Linux. The non-Windows code *does* try to check for execve() failures in the child, in addition to the fork() failures. Signed-off-by: bert Dvornik --- run-command.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/run-command.c b/run-command.c index d1a8ceb..41666ac 100644 --- a/run-command.c +++ b/run-command.c @@ -383,6 +383,8 @@ fail_pipe: close(cmd->out); if (need_err) close_pair(fderr); + else if (cmd->err) + close(cmd->err); errno = failed_errno; return -1; } -- 1.7.1.rc1.1794.g4bea1 ``` ## Johannes Sixt, 2010-04-26 06:16 Subject: Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails Message-ID: <4BD52FCB.6020902@viscovery.net> URL: https://gitlist.dev/e/4BD52FCB.6020902%40viscovery.net In-Reply-To: <1272244540-5960-1-git-send-email-dvornik+git@gmail.com> ``` Am 4/26/2010 3:15, schrieb bert Dvornik: > Fix the problem where the cmd->err passed into start_command wasn't > being properly closed when certain types of errors occurr. (Compare > the affected code with the clean shutdown code later in the function.) > diff --git a/run-command.c b/run-command.c > index d1a8ceb..41666ac 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -383,6 +383,8 @@ fail_pipe: > close(cmd->out); > if (need_err) > close_pair(fderr); > + else if (cmd->err) > + close(cmd->err); > errno = failed_errno; > return -1; > } Good catch! This should go on maint (1.7.0). Acked-by: Johannes Sixt -- Hannes ``` ## Johannes Schindelin, 2010-04-26 06:29 Subject: Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.1004260828470.7651%40pacific.mpi-cbg.de In-Reply-To: <1272244540-5960-1-git-send-email-dvornik+git@gmail.com> ``` Hi, On Sun, 25 Apr 2010, bert Dvornik wrote: > Fix the problem where the cmd->err passed into start_command wasn't > being properly closed when certain types of errors occurr. (Compare the > affected code with the clean shutdown code later in the function.) > > On Windows, this problem would be triggered if mingw_spawnvpe() failed, > which would happen if the command to be executed was malformed (e.g. a > text file that didn't start with a #! line). If cmd->err was a pipe, > the failure to close it could result in a hang while the other side was > waiting (forever) for either input or pipe close, e.g. while trying to > shove the output into the side band. On msysGit, this problem was > causing a hang in t5516-fetch-push. > > I'm not sure why (or if) this problem hasn't cropped up under Linux. The > non-Windows code *does* try to check for execve() failures in the child, > in addition to the fork() failures. Thanks. Both patches applied and pushed to 4msysgit.git. Ciao, Dscho ``` ## Johannes Sixt, 2010-04-26 06:48 Subject: Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails Message-ID: <4BD53749.6040708@viscovery.net> URL: https://gitlist.dev/e/4BD53749.6040708%40viscovery.net In-Reply-To: <1272244540-5960-1-git-send-email-dvornik+git@gmail.com> ``` Am 4/26/2010 3:15, schrieb bert Dvornik: > Fix the problem where the cmd->err passed into start_command wasn't > being properly closed when certain types of errors occurr. (Compare > the affected code with the clean shutdown code later in the function.) > > On Windows, this problem would be triggered if mingw_spawnvpe() > failed, which would happen if the command to be executed was malformed > (e.g. a text file that didn't start with a #! line). If cmd->err was > a pipe, the failure to close it could result in a hang while the other > side was waiting (forever) for either input or pipe close, e.g. while > trying to shove the output into the side band. On msysGit, this > problem was causing a hang in t5516-fetch-push. > > I'm not sure why (or if) this problem hasn't cropped up under Linux. > The non-Windows code *does* try to check for execve() failures in the > child, in addition to the fork() failures. The problem does show up on Linux if you trigger the right error, such as with this patch: diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 2de98e6..2515a96 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -528,7 +528,8 @@ test_expect_success 'push does not update local refs mk_test heads/master && mk_child child && mkdir testrepo/.git/hooks && - echo exit 1 >testrepo/.git/hooks/pre-receive && + echo "#!/bin/frobnicuty" >testrepo/.git/hooks/pre-receive && + echo exit 1 >>testrepo/.git/hooks/pre-receive && chmod +x testrepo/.git/hooks/pre-receive && (cd child && git pull .. master ``` ## Albert Dvornik, 2010-04-26 11:47 Subject: Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails Message-ID: URL: https://gitlist.dev/e/z2n9f74dc2c1004260447y12b522d8if37cad56548738f1%40mail.gmail.com In-Reply-To: <4BD53749.6040708@viscovery.net> ``` On Mon, Apr 26, 2010 at 2:48 AM, Johannes Sixt wrote: > Am 4/26/2010 3:15, schrieb bert Dvornik: [...] >> I'm not sure why (or if) this problem hasn't cropped up under Linux. >> The non-Windows code *does* try to check for execve() failures in the >> child, in addition to the fork() failures. > > The problem does show up on Linux if you trigger the right error, such as > with this patch: [...] > +       echo "#!/bin/frobnicuty" >testrepo/.git/hooks/pre-receive && > +       echo exit 1 >>testrepo/.git/hooks/pre-receive && Ah, that makes sense! Thank you for clearing up the mystery. Junio: would you like me to resend with an updated commit message, and maybe the addition of Hannes's test as well? --bert ``` ## Junio C Hamano, 2010-04-26 23:58 Subject: Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails Message-ID: <7vtyqxix5r.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtyqxix5r.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Albert Dvornik writes: > Junio: would you like me to resend with an updated commit message, and > maybe the addition of Hannes's test as well? I am out of town right now and suspect my mail reception for coming few days would be leaky --- as Dscho seems on top of this series, I suspect that the patch flow would be more reliable if I get your change via a pull request from him when I return. Thanks. ```