git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Mike Hommey <mh@glandium.org>
To: Jeff King <peff@peff.net>
Cc: Johannes Sixt <j6t@kdbg.org>,
	Ævar Arnfjörð Bjarmason <avarab@gmail.com>,
	git@vger.kernel.org
Subject: Re: Closing fds twice when using remote helpers
Date: Thu, 16 May 2019 09:48:02 +0900
Message-ID: <20190516004802.fmtdz52xxcplkgcf@glandium.org> (raw)
In-Reply-To: <20190515235339.GA3579@sigill.intra.peff.net>

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

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-15 10:56 Mike Hommey
2019-05-15 11:43 ` Ævar Arnfjörð Bjarmason
2019-05-15 17:59   ` Johannes Sixt
2019-05-15 22:08     ` Mike Hommey
2019-05-15 23:53       ` Jeff King
2019-05-16  0:48         ` Mike Hommey [this message]
2019-05-16  3:28           ` Jeff King
2019-05-16  8:35             ` Mike Hommey
2019-05-16 21:47               ` Jeff King
2019-05-16 22:02                 ` Mike Hommey
2019-05-16  0:31 ` Mike Hommey
2019-05-16  0:37   ` [PATCH 1/2] dup() the input fd for fast-import used for " Mike Hommey
2019-05-16  0:37     ` [PATCH 2/2] Use xmmap_gently instead of xmmap in use_pack Mike Hommey
2019-05-16  3:34       ` Jeff King
2019-05-16  3:28     ` [PATCH 1/2] dup() the input fd for fast-import used for remote helpers Jeff King

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190516004802.fmtdz52xxcplkgcf@glandium.org \
    --to=mh@glandium.org \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox