Re: Closing fds twice when using remote helpers

[Mike Hommey <mh@glandium.org>]

On Wed, May 15, 2019 at 07:53:40PM -0400, Jeff King wrote:
> On Thu, May 16, 2019 at 07:08:34AM +0900, Mike Hommey wrote:
> 
> > > >> - Except, well, fds being what they are, we in fact just closed a fd
> > > >>   from a packed_git->pack_fd. So, when use_pack is later called, and
> > > >>   tries to mmap data from that pack, it fails because the file
> > > >>   descriptor was closed.
> > > 
> > > Either dup() the file descriptor, or mmap() before you call the
> > > consuming start_command().
> 
> If I understand your case correctly, the mmap() is not relevant.  The
> issue is that we close a random file descriptor accidentally, and it
> just happens to be a pack descriptor later on. Right?
> 
> > You seem to imply this is my code doing something. It's not. The process
> > that is doing all the things I noted above is an unmodified `git fetch`,
> > when using a remote-helper transport. The use_pack happens after the
> > transport is disposed because that's at the end of git fetch, when it
> > updates refs.
> 
> Yes, it's a bug in git.
> 
> > Anyway, it would seem the fix is to dup(out) when passing it as in to
> > start_command?
> 
> Generally, yes. It looks like maybe this spot?
> 
> diff --git a/transport-helper.c b/transport-helper.c
> index fcd2a58d0e..45cdf891ec 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -433,7 +433,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
>  	struct helper_data *data = transport->data;
>  	int cat_blob_fd, code;
>  	child_process_init(fastimport);
> -	fastimport->in = helper->out;
> +	fastimport->in = xdup(helper->out);
>  	argv_array_push(&fastimport->args, "fast-import");
>  	argv_array_push(&fastimport->args, debug ? "--stats" : "--quiet");
>  
> 
> One thing I'd wonder, though: what is the contract between the helper
> and fast-import here? In the current code, when the helper closes its
> stdout, fast-import will see EOF. But not if we are holding on to
> another copy of the descriptor.

The helper is supposed to finish the fast-import stream with "done".
The documentation doesn't say much, but it also seems like the helper
could theoretically continue to respond to commands it's sent after
having done so, but that currently never happens AFAICT.

Mike