Closing fds twice when using remote helpers

Closing fds twice when using remote helpers

[Mike Hommey]

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

Re: Closing fds twice when using remote helpers

[Æ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
> - 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.

Re: Closing fds twice when using remote helpers

[Johannes Sixt]

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

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

Re: Closing fds twice when using remote helpers

[Mike Hommey]

On Wed, May 15, 2019 at 07:59:49PM +0200, Johannes Sixt wrote:
> 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

Re: Closing fds twice when using remote helpers

[Jeff King]

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.

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

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

-Peff

Re: Closing fds twice when using remote helpers

[Mike Hommey]

On Wed, May 15, 2019 at 07:56:09PM +0900, 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
> - start_command is called again for git-remote-hg, passing the
>   fast_import->out as cmd->in.

My mistake, it's the opposite: start_command for fast-import is called
with the out of git-remote-hg as in.

Anyways, patches incoming.

Mike

[PATCH 1/2] dup() the input fd for fast-import used for remote helpers

[Mike Hommey]

When a remote helper exposes the "import" capability, stdout of the
helper is sent to stdin of a new fast-import process. This is done by
setting the corresponding child_process's in field to the value of the
out field of the helper child_process.

The child_process API is defined to close the file descriptors it's
given when calling start_command. This means when start_command is
called for the fast-import process, its input fd (the output fd of the
helper), is closed.

But when the transport helper is later destroyed, in disconnect_helper,
its input and output are closed, which means close() is called with
an invalid fd (since it was already closed as per above). Or worse, with
a valid fd owned by something else (since fd numbers can be reused).

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 transport-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 1f52c95fd8..29787b749e 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -421,7 +421,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");
 
-- 
2.21.0.dirty

[PATCH 2/2] Use xmmap_gently instead of xmmap in use_pack

[Mike Hommey]

use_pack has its own error message on mmap error, but it can't be
reached when using xmmap, which dies with its own error.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 packfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packfile.c b/packfile.c
index 16bcb75262..6a66b605e9 100644
--- a/packfile.c
+++ b/packfile.c
@@ -630,7 +630,7 @@ unsigned char *use_pack(struct packed_git *p,
 			while (packed_git_limit < pack_mapped
 				&& unuse_one_window(p))
 				; /* nothing */
-			win->base = xmmap(NULL, win->len,
+			win->base = xmmap_gently(NULL, win->len,
 				PROT_READ, MAP_PRIVATE,
 				p->pack_fd, win->offset);
 			if (win->base == MAP_FAILED)
-- 
2.21.0.dirty

Re: Closing fds twice when using remote helpers

[Mike Hommey]

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

Re: Closing fds twice when using remote helpers

[Jeff King]

On Thu, May 16, 2019 at 09:48:02AM +0900, Mike Hommey wrote:

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

Re: [PATCH 1/2] dup() the input fd for fast-import used for remote helpers

[Jeff King]

On Thu, May 16, 2019 at 09:37:35AM +0900, Mike Hommey wrote:

> When a remote helper exposes the "import" capability, stdout of the
> helper is sent to stdin of a new fast-import process. This is done by
> setting the corresponding child_process's in field to the value of the
> out field of the helper child_process.
> 
> The child_process API is defined to close the file descriptors it's
> given when calling start_command. This means when start_command is
> called for the fast-import process, its input fd (the output fd of the
> helper), is closed.
> 
> But when the transport helper is later destroyed, in disconnect_helper,
> its input and output are closed, which means close() is called with
> an invalid fd (since it was already closed as per above). Or worse, with
> a valid fd owned by something else (since fd numbers can be reused).

I think this strategy is OK, as explained in my other email.

-Peff

Re: [PATCH 2/2] Use xmmap_gently instead of xmmap in use_pack

[Jeff King]

On Thu, May 16, 2019 at 09:37:36AM +0900, Mike Hommey wrote:

> use_pack has its own error message on mmap error, but it can't be
> reached when using xmmap, which dies with its own error.

Makes sense. Amusingly this xmmap comes from c4712e4553 (Replace mmap
with xmmap, better handling MAP_FAILED., 2006-12-24), which specifically
moved everything to xmmap for its error handling, and dropped all of the
MAP_FAILED checks. But it forgot to remove this one.

So we could also go the other way, and just remove the unreachable
check. I do think the error from use_pack() is better though, so I like
the direction you went.

-Peff

Re: Closing fds twice when using remote helpers

[Mike Hommey]

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

Re: Closing fds twice when using remote helpers

[Jeff King]

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.

-Peff

Re: Closing fds twice when using remote helpers

[Mike Hommey]

On Thu, May 16, 2019 at 05:47:52PM -0400, Jeff King wrote:
> 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