{"thread":{"id":"35179","subject":"[PATCH v2] Clear fd after closing to avoid double-close error","startedAt":"2013-10-22T13:36:02Z","lastAt":"2013-10-23T13:00:01Z","messageCount":3,"participants":["Jens Lindström","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"229287","messageId":"1382448962-31782-1-git-send-email-jl@opera.com","threadId":"35179","inReplyTo":null,"subject":"[PATCH v2] Clear fd after closing to avoid double-close error","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2013-10-22T13:36:02Z","receivedAt":"2013-10-22T13:36:02Z","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()). Likewise, in get_pack(), clear the fd passed 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; or similarly with fetch_refs_from_pack(),\nindirect caller of get_pack().\n\nSigned-off-by: Jens Lindström \n---\n fetch-pack.c | 4 ++++\n send-pack.c | 4 ++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f5d99c1..29b711a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -776,6 +776,10 @@ static int get_pack(struct fetch_pack_args *args,\n \t\tclose(cmd.out);\n \t}\n \n+\tif (!use_sideband)\n+\t\t/* Closed by start_command() */\n+\t\txd[0] = -1;\n+\n \tret = finish_command(&cmd);\n \tif (!ret || (args->check_self_contained_and_connected && ret == 1))\n \t\targs->self_contained_and_connected =\ndiff --git a/send-pack.c b/send-pack.c\nindex 7d172ef..edbfd07 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -300,8 +300,12 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tshutdown(fd[0], SHUT_WR);\n \t\t\tif (use_sideband)\n \t\t\t\tfinish_async(&demux);\n+\t\t\tfd[1] = -1;\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":"229292","messageId":"20131022152948.GA21940@sigill.intra.peff.net","threadId":"35179","inReplyTo":"1382448962-31782-1-git-send-email-jl@opera.com","subject":"Re: [PATCH v2] Clear fd after closing to avoid double-close error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-22T15:29:48Z","receivedAt":"2013-10-22T15:29:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 22, 2013 at 03:36:02PM +0200, Jens Lindström wrote:\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()). Likewise, in get_pack(), clear the fd passed 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; or similarly with fetch_refs_from_pack(),\n> indirect caller of get_pack().\n\nThanks, this looks like the right thing to do.\n\nYou had mentioned earlier in the thread the possibility of just dropping\nthe close() in git_transport_push. I agree that it is not worth going\nthat route, as it serves as a safety check to avoid leaking in early\nreturns (and indeed, in both the fetch and push cases, we can return\nearly without spawning a subprocess when the receiver already has all of\nthe necessary objects).\n\n> Signed-off-by: Jens Lindström \n\nAcked-by: Jeff King \n\n-Peff\n"},{"id":"229324","messageId":"CACsJy8Dqjfnu856OCdBpnwNyeOeE5p8dKTFe_KNKy8O0t2vSYQ@mail.gmail.com","threadId":"35179","inReplyTo":"1382448962-31782-1-git-send-email-jl@opera.com","subject":"Re: [PATCH v2] Clear fd after closing to avoid double-close error","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-23T13:00:01Z","receivedAt":"2013-10-23T13:00:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 22, 2013 at 8:36 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()). Likewise, in get_pack(), clear the fd passed 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; or similarly with fetch_refs_from_pack(),\n> indirect caller of get_pack().\n>\n> Signed-off-by: Jens Lindström \n> ---\n> fetch-pack.c | 4 ++++\n> send-pack.c | 4 ++++\n> 2 files changed, 8 insertions(+)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f5d99c1..29b711a 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -776,6 +776,10 @@ static int get_pack(struct fetch_pack_args *args,\n> close(cmd.out);\n> }\n>\n> + if (!use_sideband)\n> + /* Closed by start_command() */\n> + xd[0] = -1;\n> +\n\nFurther up demux.out still holds this handle. But we don't actually\nstart_async when use_sideband is false so demux.out can't be used\nanywhere else. So it's good.\n\nAcked-by: me\n\n> ret = finish_command(&cmd);\n> if (!ret || (args->check_self_contained_and_connected && ret == 1))\n> args->self_contained_and_connected =\n> diff --git a/send-pack.c b/send-pack.c\n> index 7d172ef..edbfd07 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -300,8 +300,12 @@ int send_pack(struct send_pack_args *args,\n> shutdown(fd[0], SHUT_WR);\n> if (use_sideband)\n> finish_async(&demux);\n> + fd[1] = -1;\n> return -1;\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> 1.8.1.2\n>\n\n\n\n-- \nDuy\n"}]}