Re: Closing fds twice when using remote helpers

From: Johannes Sixt <j6t@kdbg.org>
Date: May 15, 2019, 17:59 UTC
Message-ID: <b6ff2486-a1c2-4e89-4338-9e4e56d528bc@kdbg.org>
In-Reply-To: <87bm04vt81.fsf@evledraar.gmail.com>

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