Re: Closing fds twice when using remote helpers

[Mike Hommey <mh@glandium.org>]

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