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
