threads / discuss / 51099

Closing fds twice when using remote helpers

Subject: Closing fds twice when using remote helpers

## tl;dr

10 messages between May 15, 2019 and May 16, 2019.

replies: 9people: 4as markdown or json

Mike Hommey· May 15, 2019, 10:56 UTC · lore
Hi,

I started getting a weird error message during some test case involving git-cinnabar, which is a remote-helper to access mercurial repositories.

The error says: fatal: mmap failed: Bad file descriptor

... which was not making much sense. Some debugging later, and it turns out this is what happens:

- start_command is called for fast-import
- start_command is called again for git-remote-hg, passing the
  fast_import->out as cmd->in.
- in start_command, we end up on the line of code that does
  close(cmd->in), so fast_import->out/cmd->in is now closed
- much later, in disconnect_helper, we call close(data->helper->out),
  where data->helper is the cmd for fast-import, and that fd was already
closed above.
- 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.
I'm not entirely sure how to address this... Any ideas?

Relatedly, use_pack calls xmmap, which does its own error handling and die()s in case of error, but then goes on to do its own check with a different error message (which, in fact, could be more useful in other cases). It seems like it should call xmmap_gently instead.

Cheers,
Mike
Ævar Arnfjörð Bjarmason· May 15, 2019, 11:43 UTC · re: Mike Hommey · lore

Re: Closing fds twice when using remote helpers

On Wed, May 15 2019, Mike Hommey wrote:
Show 31 quoted lines
> Hi,
>
> I started getting a weird error message during some test case involving
> git-cinnabar, which is a remote-helper to access mercurial
> repositories.
>
> The error says:
> fatal: mmap failed: Bad file descriptor
>
> ... which was not making much sense. Some debugging later, and it turns
> out this is what happens:
>
> - start_command is called for fast-import
> - start_command is called again for git-remote-hg, passing the
>   fast_import->out as cmd->in.
> - in start_command, we end up on the line of code that does
>   close(cmd->in), so fast_import->out/cmd->in is now closed
> - much later, in disconnect_helper, we call close(data->helper->out),
>   where data->helper is the cmd for fast-import, and that fd was already
> closed above.
> - 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.
>
> I'm not entirely sure how to address this... Any ideas?
>
> Relatedly, use_pack calls xmmap, which does its own error handling and
> die()s in case of error, but then goes on to do its own check with a
> different error message (which, in fact, could be more useful in other
> cases). It seems like it should call xmmap_gently instead.

The "obvious" hacky fix is to pass in some "I own it, don't close it" new flag in the child_process struct.

In fact we used to have such a thing in the code, see e72ae28895 ("start_command(), .in/.out/.err = -1: Callers must close the file descriptor", 2008-02-16).

So we could bring it back, but I wonder if a better long-term solution is to refactor the API to have explicit start_command() -> free_command() steps, even if the free() is something that happens implicitly unless some "gutsy" function is called.

Johannes Sixt· May 15, 2019, 17:59 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Closing fds twice when using remote helpers

Am 15.05.19 um 13:43 schrieb Ævar Arnfjörð Bjarmason:
Show 16 quoted lines
> 
> On Wed, May 15 2019, Mike Hommey wrote:
> 
>> Hi,
>>
>> I started getting a weird error message during some test case involving
>> git-cinnabar, which is a remote-helper to access mercurial
>> repositories.
>>
>> The error says:
>> fatal: mmap failed: Bad file descriptor
>>
>> ... which was not making much sense. Some debugging later, and it turns
>> out this is what happens:
>>
>> - start_command is called for fast-import
I guess, you request fast_import->out = -1.
>> - start_command is called again for git-remote-hg, passing the
>>   fast_import->out as cmd->in.
OK.
>> - in start_command, we end up on the line of code that does
>>   close(cmd->in), so fast_import->out/cmd->in is now closed
Yes. That's how the interface is specified.
>> - much later, in disconnect_helper, we call close(data->helper->out),
>>   where data->helper is the cmd for fast-import, and that fd was already
>> closed above.

That must is wrong. Passing a fd to start_command() relinquishes responsibility.

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

Show 13 quoted lines
>> I'm not entirely sure how to address this... Any ideas?
>>
>> Relatedly, use_pack calls xmmap, which does its own error handling and
>> die()s in case of error, but then goes on to do its own check with a
>> different error message (which, in fact, could be more useful in other
>> cases). It seems like it should call xmmap_gently instead.
> 
> The "obvious" hacky fix is to pass in some "I own it, don't close it"
> new flag in the child_process struct.
> 
> In fact we used to have such a thing in the code, see e72ae28895
> ("start_command(), .in/.out/.err = -1: Callers must close the file
> descriptor", 2008-02-16).

That's a different thing. -1 tells that a pipe end should be passed back to the caller. Of course, it must not be closed by start_command.

But if the caller passes their own fd *into* start_command/run_command, then the caller must not close it.

> So we could bring it back, but I wonder if a better long-term solution
> is to refactor the API to have explicit start_command() ->
> free_command() steps, even if the free() is something that happens
> implicitly unless some "gutsy" function is called.
*Shrug* I'd use C++ to make the interface a no-brainer.
-- Hannes
Mike Hommey· May 15, 2019, 22:08 UTC · re: Johannes Sixt · lore

Re: Closing fds twice when using remote helpers

On Wed, May 15, 2019 at 07:59:49PM +0200, Johannes Sixt wrote:
Show 44 quoted lines
> Am 15.05.19 um 13:43 schrieb Ævar Arnfjörð Bjarmason:
> > 
> > On Wed, May 15 2019, Mike Hommey wrote:
> > 
> >> Hi,
> >>
> >> I started getting a weird error message during some test case involving
> >> git-cinnabar, which is a remote-helper to access mercurial
> >> repositories.
> >>
> >> The error says:
> >> fatal: mmap failed: Bad file descriptor
> >>
> >> ... which was not making much sense. Some debugging later, and it turns
> >> out this is what happens:
> >>
> >> - start_command is called for fast-import
> 
> I guess, you request fast_import->out = -1.
> 
> >> - start_command is called again for git-remote-hg, passing the
> >>   fast_import->out as cmd->in.
> 
> OK.
> 
> >> - in start_command, we end up on the line of code that does
> >>   close(cmd->in), so fast_import->out/cmd->in is now closed
> 
> Yes. That's how the interface is specified.
> 
> >> - much later, in disconnect_helper, we call close(data->helper->out),
> >>   where data->helper is the cmd for fast-import, and that fd was already
> >> closed above.
> 
> That must is wrong. Passing a fd to start_command() relinquishes
> responsibility.
> 
> >> - 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().

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.

Anyway, it would seem the fix is to dup(out) when passing it as in to start_command?

Mike
Jeff King· May 15, 2019, 23:53 UTC · re: Mike Hommey · lore

Re: Closing fds twice when using remote helpers

On Thu, May 16, 2019 at 07:08:34AM +0900, Mike Hommey wrote:
Show 7 quoted lines
> > >> - 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?

Show 5 quoted lines
> 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.

In that case, the right solution is probably more like:

  fastimport->in = helper->out;
  helper->out = -1; /* hand ownership to fast-import */

-Peff
Mike Hommey· May 16, 2019, 00:48 UTC · re: Jeff King · lore

Re: Closing fds twice when using remote helpers

On Wed, May 15, 2019 at 07:53:40PM -0400, Jeff King wrote:
Show 45 quoted lines
> 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
Jeff King· May 16, 2019, 03:28 UTC · re: Mike Hommey · lore

Re: Closing fds twice when using remote helpers

On Thu, May 16, 2019 at 09:48:02AM +0900, Mike Hommey wrote:
Show 23 quoted lines
> > 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.

Hmm. We do not even pass --done to fast-import. If we are really expecting everybody to say "done", then it seems like we ought to be doing so. I think that "done" came much later than the concept of fast-import, so while most reasonable importers would send it, I suspect antique ones would not.

So I was all ready to say that we need to do it the other way (pass off ownership) in order for fast-import to exit when the helper closes the descriptor. But actually, I think I am being silly. The duplicated descriptor is the _output_ from the helper, not the _input_ to fast-import. So if we are also holding that output descriptor, fast-import will not care. It is only the helper which would then not notice fast-import dying (and continue writing to the descriptor without EPIPE, since we are still on the other end of it).

I think that's probably OK, as we would see fast-import exit and then continue ourselves. We'd probably die() immediately assuming fast-import exits with an error. But if we don't, want happens? The helper would eventually block if it fills the pipe buffer. We'd eventually end up in disconnect_helper(). I think it would work out because we close(data->helper->out) before trying to reap the child, so it would get SIGPIPE then and exit.

So I think that works. But I have to admit that handing off ownership seems simpler to reason about. :)

Totally orthogonal, but I think we might also want to introduce a helper capability so that import helpers can say "I always send 'done' to fast-import". And then we can pass "--done" to fast-import, which means it would detect a truncated stream.

-Peff
Mike Hommey· May 16, 2019, 08:35 UTC · re: Jeff King · lore

Re: Closing fds twice when using remote helpers

On Wed, May 15, 2019 at 11:28:02PM -0400, Jeff King wrote:
> Totally orthogonal, but I think we might also want to introduce a helper
> capability so that import helpers can say "I always send 'done' to
> fast-import". And then we can pass "--done" to fast-import, which means
> it would detect a truncated stream.

Doubly orthogonal, but I wouldn't mind a helper capability that allows import helpers to deal with creating git objects on their own rather than having a fast-import spawned (git-cinnabar actually doesn't use the fast-import stream it's offered to use, but can't be a fetch helper either)

Mike
Jeff King· May 16, 2019, 21:47 UTC · re: Mike Hommey · lore

Re: Closing fds twice when using remote helpers

On Thu, May 16, 2019 at 05:35:19PM +0900, Mike Hommey wrote:
Show 11 quoted lines
> On Wed, May 15, 2019 at 11:28:02PM -0400, Jeff King wrote:
> > Totally orthogonal, but I think we might also want to introduce a helper
> > capability so that import helpers can say "I always send 'done' to
> > fast-import". And then we can pass "--done" to fast-import, which means
> > it would detect a truncated stream.
> 
> Doubly orthogonal, but I wouldn't mind a helper capability that allows
> import helpers to deal with creating git objects on their own rather
> than having a fast-import spawned (git-cinnabar actually doesn't use the
> fast-import stream it's offered to use, but can't be a fetch helper
> either)

Yeah, while writing that I definitely thought "Gee, wouldn't it be easier if the importer was just responsible for running fast-import itself?".

What makes it impossible to run as a normal fetch-helper? Is it that we expect the helper to then report back the refs for us to update?

So I take it your strategy is to just run your own fast-import, and then you just pass along EOF with no input to the one that Git runs, to signal that you're done importing? So maybe the right capability is to let the helper say "by the way, I don't need you to run fast-import for me". But it's probably not that big a deal, since it's just wasting an extra process startup.

-Peff
Mike Hommey· May 16, 2019, 22:02 UTC · re: Jeff King · lore

Re: Closing fds twice when using remote helpers

On Thu, May 16, 2019 at 05:47:52PM -0400, Jeff King wrote:
Show 27 quoted lines
> On Thu, May 16, 2019 at 05:35:19PM +0900, Mike Hommey wrote:
> 
> > On Wed, May 15, 2019 at 11:28:02PM -0400, Jeff King wrote:
> > > Totally orthogonal, but I think we might also want to introduce a helper
> > > capability so that import helpers can say "I always send 'done' to
> > > fast-import". And then we can pass "--done" to fast-import, which means
> > > it would detect a truncated stream.
> > 
> > Doubly orthogonal, but I wouldn't mind a helper capability that allows
> > import helpers to deal with creating git objects on their own rather
> > than having a fast-import spawned (git-cinnabar actually doesn't use the
> > fast-import stream it's offered to use, but can't be a fetch helper
> > either)
> 
> Yeah, while writing that I definitely thought "Gee, wouldn't it be
> easier if the importer was just responsible for running fast-import
> itself?".
> 
> What makes it impossible to run as a normal fetch-helper? Is it that we
> expect the helper to then report back the refs for us to update?
> 
> So I take it your strategy is to just run your own fast-import, and then
> you just pass along EOF with no input to the one that Git runs, to
> signal that you're done importing? So maybe the right capability is to
> let the helper say "by the way, I don't need you to run fast-import for
> me". But it's probably not that big a deal, since it's just wasting an
> extra process startup.

When it received the "import" command, git-cinnabar does its stuff, creates the namespaced refs it advertized in its response to "capabilities", and writes "done". That's all. So in fact, the current setup, without --done passed to fast-import, just works.

Mike

← back to recent threads