threads / patch / 23601

patchstart_command: close cmd->err descriptor when fork/spawn fails

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

## tl;dr

6 messages between Apr 26, 2010 and Apr 26, 2010. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

bert Dvornik· Apr 26, 2010, 01:15 UTC · lore

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(-)

Show changes to run-command.c +2 −0

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· Apr 26, 2010, 06:16 UTC · re: bert Dvornik · lore

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

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

Show 13 quoted lines

> 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

Johannes Schindelin· Apr 26, 2010, 06:29 UTC · re: bert Dvornik · lore

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

Hi,

On Sun, 25 Apr 2010, bert Dvornik wrote:

Show 15 quoted lines

> 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· Apr 26, 2010, 06:48 UTC · re: bert Dvornik · lore

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

Am 4/26/2010 3:15, schrieb bert Dvornik:

Show 15 quoted lines

> 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:

Show changes to t/t5516-fetch-push.sh +2 −1

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· Apr 26, 2010, 11:47 UTC · re: Johannes Sixt · lore

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

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:

[...]

Show 6 quoted lines

>> 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· Apr 26, 2010, 23:58 UTC · re: Albert Dvornik · lore

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

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.