Re: Closing fds twice when using remote helpers

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.