{"thread":{"id":"35177","subject":"Bug: git-push crash due to double-close of file descriptor","startedAt":"2013-10-22T10:25:03Z","lastAt":"2013-10-22T11:08:10Z","messageCount":3,"participants":["Jens Lindström","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"229277","messageId":"CAEef6Wx59OBQPG38Ww59vse7n1Xb=tpJANcM8iT66zpL9-dS1w@mail.gmail.com","threadId":"35177","inReplyTo":null,"subject":"Bug: git-push crash due to double-close of file descriptor","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-10-22T10:25:03Z","receivedAt":"2013-10-22T10:25:03Z","isPatch":false,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"In a repository, I have a repeatable crash when pushing a ref to a\nremote. The cause seems very simple, and it's more unclear to me why\nthis doesn't happen more often.\n\nThe cause, as I understand it:\n\ngit_transport_push() calls send_pack() which calls pack_objects()\nwhich calls start_command(), which closes the output file descriptor\n(known in start_command() as \"cmd->out\" and in git_transport_push() as\n\"data->fd[1]\".)\n\ngit_transport_push(), immediately following the call to send_pack(),\nalso closes the file descriptor. Adding error handling to this close()\ncall shows that it fails with EBADF for a normal push (one that\ndoesn't crash.)\n\nIn my crashing scenario, the file descriptor has been reused between\nthe first close() and the second incorrect close(), for opening a pack\nfile. So the second close() closes the pack file, leaving a 'struct\npacked_git' object with a \"dangling\" pack_fd member. Later on, mmap()\nis called to map the contents of the pack file, but at this point the\nfile descriptor has been reused yet again for a zero-length lock file,\nand so mmap() succeeds but returns no accessible memory, and we crash\nwhen accessing it.\n\nIt would be trivial to fix this by simply removing the close() call in\ngit_transport_push(), but I imagine this might cause a file descriptor\nleak in other cases instead.\n\nThoughts on this?\n\n/ Jens\n"},{"id":"229278","messageId":"CACsJy8AxfFzL+2LGmqqyAm9dt7BZDpOYyrj4rgkXxgtozF7qiw@mail.gmail.com","threadId":"35177","inReplyTo":"CAEef6Wx59OBQPG38Ww59vse7n1Xb=tpJANcM8iT66zpL9-dS1w@mail.gmail.com","subject":"Re: Bug: git-push crash due to double-close of file descriptor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-22T10:49:05Z","receivedAt":"2013-10-22T10:49:05Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 22, 2013 at 5:25 PM, Jens Lindström <jl@opera.com> wrote:\n> In a repository, I have a repeatable crash when pushing a ref to a\n> remote. The cause seems very simple, and it's more unclear to me why\n> this doesn't happen more often.\n>\n> The cause, as I understand it:\n>\n> git_transport_push() calls send_pack() which calls pack_objects()\n> which calls start_command(), which closes the output file descriptor\n> (known in start_command() as \"cmd->out\" and in git_transport_push() as\n> \"data->fd[1]\".)\n>\n> git_transport_push(), immediately following the call to send_pack(),\n> also closes the file descriptor. Adding error handling to this close()\n> call shows that it fails with EBADF for a normal push (one that\n> doesn't crash.)\n>\n> In my crashing scenario, the file descriptor has been reused between\n> the first close() and the second incorrect close(), for opening a pack\n> file. So the second close() closes the pack file, leaving a 'struct\n> packed_git' object with a \"dangling\" pack_fd member. Later on, mmap()\n> is called to map the contents of the pack file, but at this point the\n> file descriptor has been reused yet again for a zero-length lock file,\n> and so mmap() succeeds but returns no accessible memory, and we crash\n> when accessing it.\n>\n> It would be trivial to fix this by simply removing the close() call in\n> git_transport_push(), but I imagine this might cause a file descriptor\n> leak in other cases instead.\n>\n> Thoughts on this?\n\nset fd[1] = 0 after calling pack_objects() in send_pack() so that the\nclose() in git_transport_push() becomes no-op (with EBADF)? Another\noption is dup() it first before passing to send_pack(), but I'm not\nsure if it has any bad effects on start_command() and friends, or\nWindows.\n-- \nDuy\n"},{"id":"229279","messageId":"CACsJy8CTo8WJTewToBM_y65fg5bOPS-Mm1nunQEU5Mw_mNZU2w@mail.gmail.com","threadId":"35177","inReplyTo":"CACsJy8AxfFzL+2LGmqqyAm9dt7BZDpOYyrj4rgkXxgtozF7qiw@mail.gmail.com","subject":"Re: Bug: git-push crash due to double-close of file descriptor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-22T11:08:10Z","receivedAt":"2013-10-22T11:08:10Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 22, 2013 at 5:49 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> set fd[1] = 0\n\nI thought fd[1] = -1, I wrote fd[1] = 0 :(\n-- \nDuy\n"}]}