[PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Ping Yin]

When 'FILE *fp' is assigned to child_process.out and then start_command or
run_command is run, the standard output of the child process is expected to
be outputed to fp. However, sometimes fp is not expected to be closed since
further IO may be still performmed on fp.
---
 run-command.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 476d00c..4e5f58d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -115,13 +115,9 @@ int start_command(struct child_process *cmd)
 
 	if (need_in)
 		close(fdin[0]);
-	else if (cmd->in)
-		close(cmd->in);
 
 	if (need_out)
 		close(fdout[1]);
-	else if (cmd->out > 1)
-		close(cmd->out);
 
 	if (need_err)
 		close(fderr[1]);
-- 
1.5.3.5.1876.g7ba19-dirty

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> When 'FILE *fp' is assigned to child_process.out and then start_command or
> run_command is run, the standard output of the child process is expected to
> be outputed to fp. However, sometimes fp is not expected to be closed since
> further IO may be still performmed on fp.

Could you clarify "However, sometimes" with a concrete codepath
that has this problem in the proposed commit log message, and
Sign-off your patch please?

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Johannes Sixt]

Ping Yin schrieb:
> When 'FILE *fp' is assigned to child_process.out and then start_command or
> run_command is run, the standard output of the child process is expected to
> be outputed to fp. However, sometimes fp is not expected to be closed since
> further IO may be still performmed on fp.
> ---
>  run-command.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/run-command.c b/run-command.c
> index 476d00c..4e5f58d 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -115,13 +115,9 @@ int start_command(struct child_process *cmd)
>  
>  	if (need_in)
>  		close(fdin[0]);
> -	else if (cmd->in)
> -		close(cmd->in);
>  
>  	if (need_out)
>  		close(fdout[1]);
> -	else if (cmd->out > 1)
> -		close(cmd->out);
>  
>  	if (need_err)
>  		close(fderr[1]);

This is dangerous! You have to audit all current callers whether they close 
cmd->in or cmd->out (if they don't need the fd anymore). Otherwise you risk 
to keep a writable pipe end open and then the reader hangs, waiting for 
input that will never arrive.

-- Hannes

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Ping Yin schrieb:
>> When 'FILE *fp' is assigned to child_process.out and then start_command or
>> run_command is run, the standard output of the child process is expected to
>> be outputed to fp. However, sometimes fp is not expected to be closed since
>> further IO may be still performmed on fp.
>> ---
>>  run-command.c |    4 ----
>>  1 files changed, 0 insertions(+), 4 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 476d00c..4e5f58d 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -115,13 +115,9 @@ int start_command(struct child_process *cmd)
>>   	if (need_in)
>>  		close(fdin[0]);
>> -	else if (cmd->in)
>> -		close(cmd->in);
>>   	if (need_out)
>>  		close(fdout[1]);
>> -	else if (cmd->out > 1)
>> -		close(cmd->out);
>>   	if (need_err)
>>  		close(fderr[1]);
>
> This is dangerous! You have to audit all current callers whether they
> close cmd->in or cmd->out (if they don't need the fd
> anymore).

I am reasonably sure that they are already relying on these auto
closing of the file descriptors.

Funny that somebody falls into the trap the day after we
discussed it on another thread.

[PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Ping Yin]

When the file descriptor of 'FILE *fp' is assigned to child_process.out
and then start_command or run_command is run, the standard output of the
child process is expected to be outputed to fp.

start_command will always close fp in this case. However, sometimes fp is
not expected to be closed since further IO may be still performmed on fp.

This patch disables the auto closing behavious of start_command
and corrects all codes which depend on this kind of behaviour.

Following is a case that the auto closing behaviour is not expected.

When adding submodule summary feature to builtin-commit, in wt_status_print,
I want to output the submodule summary between changed and untracked files
as the following patch shows
        wt_status_print_changed(s);
+       wt_status_print_submodule_summary(s);
        wt_status_print_untracked(s);

All the three calls will output to s->fp (which points to a file instead of
standard output when doing committing). So I don't want s->fp to be closed after
wt_status_print_submodule_summary(s) which calls run_command.

+static void wt_status_print_submodule_summary(struct wt_status *s)
+{
+       struct child_process sm_summary;
+       memset(&sm_summary, 0, sizeof(sm_summary));
+       ...
+       sm_summary.out = fileno(s->fp);
+       ...
+       run_command(&sm_summary);
+}

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 bundle.c      |    1 +
 convert.c     |    1 +
 run-command.c |    4 ----
 upload-pack.c |    3 ++-
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index e4d60cd..fc253fb 100644
--- a/bundle.c
+++ b/bundle.c
@@ -336,6 +336,7 @@ int unbundle(struct bundle_header *header, int bundle_fd)
 	memset(&ip, 0, sizeof(ip));
 	ip.argv = argv_index_pack;
 	ip.in = bundle_fd;
+	ip.close_in = 1;
 	ip.no_stdout = 1;
 	ip.git_cmd = 1;
 	if (run_command(&ip))
diff --git a/convert.c b/convert.c
index 4df7559..ce7bed0 100644
--- a/convert.c
+++ b/convert.c
@@ -212,6 +212,7 @@ static int filter_buffer(int fd, void *data)
 	child_process.argv = argv;
 	child_process.in = -1;
 	child_process.out = fd;
+	child_process.close_out = 1;
 
 	if (start_command(&child_process))
 		return error("cannot fork to run external filter %s", params->cmd);
diff --git a/run-command.c b/run-command.c
index 476d00c..4e5f58d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -115,13 +115,9 @@ int start_command(struct child_process *cmd)
 
 	if (need_in)
 		close(fdin[0]);
-	else if (cmd->in)
-		close(cmd->in);
 
 	if (need_out)
 		close(fdout[1]);
-	else if (cmd->out > 1)
-		close(cmd->out);
 
 	if (need_err)
 		close(fderr[1]);
diff --git a/upload-pack.c b/upload-pack.c
index 7e04311..7aeda80 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -163,7 +163,8 @@ static void create_pack_file(void)
 	argv[arg++] = NULL;
 
 	memset(&pack_objects, 0, sizeof(pack_objects));
-	pack_objects.in = rev_list.out;	/* start_command closes it */
+	pack_objects.in = rev_list.out;	
+	pack_objects.close_in = 1; /* finish_command closes rev_list.out */
 	pack_objects.out = -1;
 	pack_objects.err = -1;
 	pack_objects.git_cmd = 1;
-- 
1.5.3.5.1878.gb1da0-dirty

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Johannes Sixt]

Ping Yin schrieb:
> This patch disables the auto closing behavious of start_command
> and corrects all codes which depend on this kind of behaviour.

I've thought about this a bit more, and I think that it is better to leave 
this auto-closing behavior unchanged and change your usage of this feature, 
like so:

> +static void wt_status_print_submodule_summary(struct wt_status *s)
> +{
> +       struct child_process sm_summary;
> +       memset(&sm_summary, 0, sizeof(sm_summary));
> +       ...
> +       sm_summary.out = fileno(s->fp);

	fflush(s->fp);
	sm_summary.out = dup(fileno(s->fp));	/* run_command closes it */

> +       ...
> +       run_command(&sm_summary);
> +}

This way the change is more local without affecting well-tested other callers.

Furthermore, I don't think that it's correct to just set the .close_in or 
.close_out flags. This will close the fd only in finish_command(), which can 
be too late: Think again of a writable pipe end that remains open and keeps 
the reader waiting for input that is not going to happen.

-- Hannes

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Ping Yin]

On Nov 21, 2007 12:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Ping Yin schrieb:
> > This patch disables the auto closing behavious of start_command
> > and corrects all codes which depend on this kind of behaviour.
>
> I've thought about this a bit more, and I think that it is better to leave
> this auto-closing behavior unchanged and change your usage of this feature,
> like so:
>
> > +static void wt_status_print_submodule_summary(struct wt_status *s)
> > +{
> > +       struct child_process sm_summary;
> > +       memset(&sm_summary, 0, sizeof(sm_summary));
> > +       ...
> > +       sm_summary.out = fileno(s->fp);
>
>         fflush(s->fp);
>         sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
>
> > +       ...
> > +       run_command(&sm_summary);
> > +}
>
> This way the change is more local without affecting well-tested other callers.
>

This way works, but it is a tricky one, not a natural or graceful one.

> Furthermore, I don't think that it's correct to just set the .close_in or
> .close_out flags. This will close the fd only in finish_command(), which can
> be too late: Think again of a writable pipe end that remains open and keeps
> the reader waiting for input that is not going to happen.

This may happen. However, i have scanned all the git codes using the
auto closing behaviour and i don't discover the problem you mentioned.
So i think it deserves to correct the misbehaviour after carefully
testing. And we can make a clarification for that if necessary.

>
> -- Hannes
>
>



-- 
Ping Yin

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Junio C Hamano]

"Ping Yin" <pkufranky@gmail.com> writes:

> On Nov 21, 2007 12:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> ...
>> This way the change is more local without affecting well-tested other callers.
>
> This way works, but it is a tricky one, not a natural or graceful one.

I do not know about "natural".  That largely would depend on
where one starts thinking about the issues from.

But I think an API definition that says "These fds are closed
after the call, so if you are going to use them, you can dup()
them beforehand" is equally valid, and I suspect that forgetting
to dup() is easier to detect than forgetting to close() --- you
will notice the former mistake immediately because your read and
write say "oops, nobody on the other end" but the latter mistake
will result in a hung process.  And for that reason, I think it
can be called more "graceful".  So ...

>> Furthermore, I don't think that it's correct to just set the .close_in or
>> .close_out flags. This will close the fd only in finish_command(), which can
>> be too late: Think again of a writable pipe end that remains open and keeps
>> the reader waiting for input that is not going to happen.
>
> This may happen. However, i have scanned all the git codes using the
> auto closing behaviour and i don't discover the problem you mentioned.
> So i think it deserves to correct the misbehaviour after carefully
> testing. And we can make a clarification for that if necessary.

... I do not necessarily agree that your patch is correcting the
misbehaviour.

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

[Ping Yin]

On Nov 21, 2007 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

> But I think an API definition that says "These fds are closed
> after the call, so if you are going to use them, you can dup()
> them beforehand" is equally valid, and I suspect that forgetting
> to dup() is easier to detect than forgetting to close() --- you
> will notice the former mistake immediately because your read and
> write say "oops, nobody on the other end" but the latter mistake
> will result in a hung process.  And for that reason, I think it
> can be called more "graceful".  So ...
>
I don't konw the original API definition and havn't found any API
deinition that clarifies the fds will be closed after start_command.
However, when i see child_process.close_in/close_out, i thought
start_command will not close the fds.

I never said that start_command must not close fd. At least this
behaviour of start_command makes child_process.close_in/close_out no
sense.
>
>



-- 
Ping Yin