threads / bug / 35177

Bug: git-push crash due to double-close of file descriptor

Subject: Bug: git-push crash due to double-close of file descriptor

## tl;dr

3 messages between Oct 22, 2013 and Oct 22, 2013.

replies: 2people: 2as markdown or json

Jens Lindström· Oct 22, 2013, 10:25 UTC · lore

In a repository, I have a repeatable crash when pushing a ref to a remote. The cause seems very simple, and it's more unclear to me why this doesn't happen more often.

The cause, as I understand it:

git_transport_push() calls send_pack() which calls pack_objects() which calls start_command(), which closes the output file descriptor (known in start_command() as "cmd->out" and in git_transport_push() as "data->fd[1]".)

git_transport_push(), immediately following the call to send_pack(), also closes the file descriptor. Adding error handling to this close() call shows that it fails with EBADF for a normal push (one that doesn't crash.)

In my crashing scenario, the file descriptor has been reused between the first close() and the second incorrect close(), for opening a pack file. So the second close() closes the pack file, leaving a 'struct packed_git' object with a "dangling" pack_fd member. Later on, mmap() is called to map the contents of the pack file, but at this point the file descriptor has been reused yet again for a zero-length lock file, and so mmap() succeeds but returns no accessible memory, and we crash when accessing it.

It would be trivial to fix this by simply removing the close() call in git_transport_push(), but I imagine this might cause a file descriptor leak in other cases instead.

Thoughts on this?

/ Jens

Duy Nguyen· Oct 22, 2013, 10:49 UTC · re: Jens Lindström · lore

Re: Bug: git-push crash due to double-close of file descriptor

On Tue, Oct 22, 2013 at 5:25 PM, Jens Lindström <jl@opera.com> wrote:

Show 30 quoted lines

> In a repository, I have a repeatable crash when pushing a ref to a
> remote. The cause seems very simple, and it's more unclear to me why
> this doesn't happen more often.
>
> The cause, as I understand it:
>
> git_transport_push() calls send_pack() which calls pack_objects()
> which calls start_command(), which closes the output file descriptor
> (known in start_command() as "cmd->out" and in git_transport_push() as
> "data->fd[1]".)
>
> git_transport_push(), immediately following the call to send_pack(),
> also closes the file descriptor. Adding error handling to this close()
> call shows that it fails with EBADF for a normal push (one that
> doesn't crash.)
>
> In my crashing scenario, the file descriptor has been reused between
> the first close() and the second incorrect close(), for opening a pack
> file. So the second close() closes the pack file, leaving a 'struct
> packed_git' object with a "dangling" pack_fd member. Later on, mmap()
> is called to map the contents of the pack file, but at this point the
> file descriptor has been reused yet again for a zero-length lock file,
> and so mmap() succeeds but returns no accessible memory, and we crash
> when accessing it.
>
> It would be trivial to fix this by simply removing the close() call in
> git_transport_push(), but I imagine this might cause a file descriptor
> leak in other cases instead.
>
> Thoughts on this?

set fd[1] = 0 after calling pack_objects() in send_pack() so that the close() in git_transport_push() becomes no-op (with EBADF)? Another option is dup() it first before passing to send_pack(), but I'm not sure if it has any bad effects on start_command() and friends, or Windows.

-- 
Duy

Duy Nguyen· Oct 22, 2013, 11:08 UTC · re: Duy Nguyen · lore

Re: Bug: git-push crash due to double-close of file descriptor

On Tue, Oct 22, 2013 at 5:49 PM, Duy Nguyen <pclouds@gmail.com> wrote:

> set fd[1] = 0

I thought fd[1] = -1, I wrote fd[1] = 0 :(

-- 
Duy