On Tue, Oct 22, 2013 at 5:25 PM, Jens Lindström <jl@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html