# Bug: git-push crash due to double-close of file descriptor 3 messages from 2013-10-22 to 2013-10-22. Participants: Jens Lindström, Duy Nguyen. Thread: https://gitlist.dev/t/35177 ## Jens Lindström, 2013-10-22 10:25 Subject: Bug: git-push crash due to double-close of file descriptor Message-ID: URL: https://gitlist.dev/e/CAEef6Wx59OBQPG38Ww59vse7n1Xb%3DtpJANcM8iT66zpL9-dS1w%40mail.gmail.com ``` 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, 2013-10-22 10:49 Subject: Re: Bug: git-push crash due to double-close of file descriptor Message-ID: URL: https://gitlist.dev/e/CACsJy8AxfFzL%2B2LGmqqyAm9dt7BZDpOYyrj4rgkXxgtozF7qiw%40mail.gmail.com In-Reply-To: ``` On Tue, Oct 22, 2013 at 5:25 PM, Jens Lindström wrote: > 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, 2013-10-22 11:08 Subject: Re: Bug: git-push crash due to double-close of file descriptor Message-ID: URL: https://gitlist.dev/e/CACsJy8CTo8WJTewToBM_y65fg5bOPS-Mm1nunQEU5Mw_mNZU2w%40mail.gmail.com In-Reply-To: ``` On Tue, Oct 22, 2013 at 5:49 PM, Duy Nguyen wrote: > set fd[1] = 0 I thought fd[1] = -1, I wrote fd[1] = 0 :( -- Duy ```