[PATCH] start_command: close cmd->err descriptor when fork/spawn fails

[PATCH] start_command: close cmd->err descriptor when fork/spawn fails

[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.

Signed-off-by: bert Dvornik <dvornik+git@gmail.com>
---
 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

Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails

[Johannes Sixt]

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 <j6t@kdbg.org>

-- Hannes

Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails

[Johannes Schindelin]

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

Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails

[Johannes Sixt]

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

Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails

[Albert Dvornik]

On Mon, Apr 26, 2010 at 2:48 AM, Johannes Sixt <j.sixt@viscovery.net> 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

Re: [PATCH] start_command: close cmd->err descriptor when fork/spawn fails

[Junio C Hamano]

Albert Dvornik <dvornik+git@gmail.com> 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.