{"thread":{"id":"35178","subject":"[PATCH] Clear fd after closing to avoid double-close error","startedAt":"2013-10-22T12:10:23Z","lastAt":"2013-10-23T16:06:53Z","messageCount":6,"participants":["Jens Lindström","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"229280","messageId":"1382443823-31317-1-git-send-email-jl@opera.com","threadId":"35178","inReplyTo":null,"subject":"[PATCH] Clear fd after closing to avoid double-close error","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-10-22T12:10:23Z","receivedAt":"2013-10-22T12:10:23Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"From: Jens Lindstrom \n\nIn send_pack(), clear the fd passed to pack_objects() by setting\nit to -1, since pack_objects() closes the fd (via a call to\nrun_command()).\n\nNot doing so risks having git_transport_push(), caller of\nsend_pack(), closing the fd again, possibly incorrectly closing\nsome other open file.\n\nSigned-off-by: Jens Lindström \n---\n send-pack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 7d172ef..7def2af 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -302,6 +302,9 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tfinish_async(&demux);\n \t\t\treturn -1;\n \t\t}\n+\t\tif (!args->stateless_rpc)\n+\t\t\t/* Closed by pack_objects() via start_command() */\n+\t\t\tfd[1] = -1;\n \t}\n \tif (args->stateless_rpc && cmds_sent)\n \t\tpacket_flush(out);\n-- \n1.8.1.2\n"},{"id":"229281","messageId":"CACsJy8DtvaojrrQr4=uYCRse7N+SXb+WdjY+C_0FRSjYsNC6zg@mail.gmail.com","threadId":"35178","inReplyTo":"1382443823-31317-1-git-send-email-jl@opera.com","subject":"Re: [PATCH] Clear fd after closing to avoid double-close error","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-22T12:39:08Z","receivedAt":"2013-10-22T12:39:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 22, 2013 at 7:10 PM, Jens Lindström wrote:\n> From: Jens Lindstrom \n>\n> In send_pack(), clear the fd passed to pack_objects() by setting\n> it to -1, since pack_objects() closes the fd (via a call to\n> run_command()).\n>\n> Not doing so risks having git_transport_push(), caller of\n> send_pack(), closing the fd again, possibly incorrectly closing\n> some other open file.\n\nNot your itch. But if you have time you may want to fix fetch-pack\ntoo. It has the same problem. fetch-pack.c:get_pack() with\nuse_sideband == 0 passes fd[0] to start_command(), then later its\ncaller transport.c:fetch_refs_via_pack() closes the handle again.\n\n> Signed-off-by: Jens Lindström \n> ---\n> send-pack.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index 7d172ef..7def2af 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -302,6 +302,9 @@ int send_pack(struct send_pack_args *args,\n> finish_async(&demux);\n> return -1;\n\nIn this code block, there is \"close(out);\", we may need to set fd[1] = -1 too.\n\n> }\n> + if (!args->stateless_rpc)\n> + /* Closed by pack_objects() via start_command() */\n> + fd[1] = -1;\n> }\n> if (args->stateless_rpc && cmds_sent)\n> packet_flush(out);\n\nI was puzzled by this packet_flush(out) for a while because I thought\n\"out\" was already closed. Turns out when stateless_rpc is true, a new\npipe is created in pack_objects() and \"out\" is not closed. So\neverything is still good (and messy).\n\nLife would have been simpler if fd[1] was _always_ closed by\nsend_pack(), like in c20181e (start_command(), if .in/.out > 0, closes\nfile descriptors, not the callers - 2008-02-21).\n-- \nDuy\n"},{"id":"229285","messageId":"CAEef6Wx7K0KJK-dMJ2HTgdoGvVhve7=rHK8gv1gQc7GO5_K0WA@mail.gmail.com","threadId":"35178","inReplyTo":"CACsJy8DtvaojrrQr4=uYCRse7N+SXb+WdjY+C_0FRSjYsNC6zg@mail.gmail.com","subject":"Re: [PATCH] Clear fd after closing to avoid double-close error","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-10-22T13:15:35Z","receivedAt":"2013-10-22T13:15:35Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"On Tue, Oct 22, 2013 at 2:39 PM, Duy Nguyen wrote:\n\n> Not your itch. But if you have time you may want to fix fetch-pack\n> too. It has the same problem. fetch-pack.c:get_pack() with\n> use_sideband == 0 passes fd[0] to start_command(), then later its\n> caller transport.c:fetch_refs_via_pack() closes the handle again.\n\nI'll update the patch to clear that fd as well.\n\n\n>> Signed-off-by: Jens Lindström \n>> ---\n>> send-pack.c | 3 +++\n>> 1 file changed, 3 insertions(+)\n>>\n>> diff --git a/send-pack.c b/send-pack.c\n>> index 7d172ef..7def2af 100644\n>> --- a/send-pack.c\n>> +++ b/send-pack.c\n>> @@ -302,6 +302,9 @@ int send_pack(struct send_pack_args *args,\n>> finish_async(&demux);\n>> return -1;\n>\n> In this code block, there is \"close(out);\", we may need to set fd[1] = -1 too.\n\nThis block closes the fd unconditionally, I think. Either via\npack_objects() (if !args->stateless_rpc) or directly (otherwise.) So I\nguess it should always clear the fd before returning to be safe.\n\n\n>> }\n>> + if (!args->stateless_rpc)\n>> + /* Closed by pack_objects() via start_command() */\n>> + fd[1] = -1;\n>> }\n>> if (args->stateless_rpc && cmds_sent)\n>> packet_flush(out);\n>\n> I was puzzled by this packet_flush(out) for a while because I thought\n> \"out\" was already closed. Turns out when stateless_rpc is true, a new\n> pipe is created in pack_objects() and \"out\" is not closed. So\n> everything is still good (and messy).\n>\n> Life would have been simpler if fd[1] was _always_ closed by\n> send_pack(), like in c20181e (start_command(), if .in/.out > 0, closes\n> file descriptors, not the callers - 2008-02-21).\n\nIt did strike me as a bit unclear who exactly \"owned\" these file\ndescriptors. But I'm of course wholly unfamiliar with this code.\n\n/ Jens\n"},{"id":"229299","messageId":"xmqq8uxltaeh.fsf@gitster.dls.corp.google.com","threadId":"35178","inReplyTo":"CACsJy8DtvaojrrQr4=uYCRse7N+SXb+WdjY+C_0FRSjYsNC6zg@mail.gmail.com","subject":"Re: [PATCH] Clear fd after closing to avoid double-close error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-22T18:42:30Z","receivedAt":"2013-10-22T18:42:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Tue, Oct 22, 2013 at 7:10 PM, Jens Lindström wrote:\n> ...\n>> + if (!args->stateless_rpc)\n>> + /* Closed by pack_objects() via start_command() */\n>> + fd[1] = -1;\n>> }\n> ...\n> Life would have been simpler if fd[1] was _always_ closed by\n> send_pack(), like in c20181e (start_command(), if .in/.out > 0, closes\n> file descriptors, not the callers - 2008-02-21).\n\nYeah, that was also my first reaction when I saw the above three\nlines after reading the discussion that led to the diagnosis.\n"},{"id":"229315","messageId":"CAEef6Wyui5EkNoEGKfb+NHZyb3BTkHME234M1zMyjuux5yMUeg@mail.gmail.com","threadId":"35178","inReplyTo":"xmqq8uxltaeh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Clear fd after closing to avoid double-close error","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-10-23T07:52:21Z","receivedAt":"2013-10-23T07:52:21Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"On Tue, Oct 22, 2013 at 8:42 PM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n\n>> Life would have been simpler if fd[1] was _always_ closed by\n>> send_pack(), like in c20181e (start_command(), if .in/.out > 0, closes\n>> file descriptors, not the callers - 2008-02-21).\n>\n> Yeah, that was also my first reaction when I saw the above three\n> lines after reading the discussion that led to the diagnosis.\n\nIf send_pack() always closes fd[1], then I believe \"git send-pack\n--stateless-rpc --helper-status\" would die in print_helper_status(),\ncalled after send_pack(), since fd[1] would be 1, to which\nprint_helper_status() will try to write. (I don't know what either\n--stateless-rpc or --helper-status mean, other than what's obvious\nfrom the code, or if the combination of them makes any sense.)\n\nI don't really have any more time to spend on this issue, so if a more\nthorough fix is required, I'm afraid someone else will have to work on\nit.\n\n/ Jens\n"},{"id":"229347","messageId":"xmqqvc0oq8de.fsf@gitster.dls.corp.google.com","threadId":"35178","inReplyTo":"CAEef6Wyui5EkNoEGKfb+NHZyb3BTkHME234M1zMyjuux5yMUeg@mail.gmail.com","subject":"Re: [PATCH] Clear fd after closing to avoid double-close error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-23T16:06:53Z","receivedAt":"2013-10-23T16:06:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström writes:\n\n> On Tue, Oct 22, 2013 at 8:42 PM, Junio C Hamano wrote:\n>> Duy Nguyen writes:\n>\n>>> Life would have been simpler if fd[1] was _always_ closed by\n>>> send_pack(), like in c20181e (start_command(), if .in/.out > 0, closes\n>>> file descriptors, not the callers - 2008-02-21).\n>>\n>> Yeah, that was also my first reaction when I saw the above three\n>> lines after reading the discussion that led to the diagnosis.\n>\n> If send_pack() always closes fd[1], then I believe \"git send-pack\n> --stateless-rpc --helper-status\" would die in print_helper_status(),\n> called after send_pack(), since fd[1] would be 1, to which\n> print_helper_status() will try to write.\n\nAh, I obviously did not look far enough. Of course we could dup(2)\nthe fd=1 to code it around, but it is not clear to me if it is worth\nit---your solution (v2) is clearer, so let's queue it with Acks we\nsaw from Peff and Duy.\n\nThanks.\n"}]}