{"thread":{"id":"51511","subject":"[PATCH] Close transport helper on protocol error","startedAt":"2019-07-22T21:23:12Z","lastAt":"2019-07-23T21:11:01Z","messageCount":4,"participants":["thibault.jamet@gmail.com","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"379160","messageId":"20190722212250.44011-1-thibault.jamet@gmail.com","threadId":"51511","inReplyTo":null,"subject":"[PATCH] Close transport helper on protocol error","fromName":"","fromEmail":"thibault.jamet@gmail.com","sentAt":"2019-07-22T21:22:50Z","receivedAt":"2019-07-22T21:23:12Z","isPatch":true,"sender":{"key":"thibault.jamet@gmail.com","avatar":null},"body":"From: Thibault Jamet <thibault.jamet@gmail.com>\n\nWe have noticed that in some cases, when the transport is not fully\ncompliant with the protocol, git exits with a status of 128 without\nclosing the transport.\n\nThis usually does not have consequences in a standard unix\nenvironment as the process gets then attached to the init one which will\nthen take care of closing properly the remaining transport.\n\nWe remarkably noticed this behaviour on GitHub Enterprise v2.14.24 when\na repository has been migrated to another GitHub Enterprise instance.\nThe output of the transport is then:\n\nremote: Repository `org/repo' is disabled. Please ask the owner to check their account.\nfatal: unable to access 'https://github.example.com/org/repo/': The requested URL returned error: 403\n\nand the code exits inside function get_refs_list\n\nIn container contexts, there might not be such an init process\nto take care of terminating the transport process and hence they remain\nas zombies, as mentioned in\nhttps://github.com/rancher/rancher/issues/13858 or\nhttps://github.com/coreos/clair/issues/441.\n\nAlthough there is a work-around running an init inside the container,\nclean-up the processes created at the time git exits.\n\nSigned-off-by: Thibault Jamet <thibault.jamet@gmail.com>\n---\n transport-helper.c | 44 +++++++++++++++++++++++++++++++++-----------\n 1 file changed, 33 insertions(+), 11 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cec83bd663..34caa75a72 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -101,6 +101,12 @@ static void do_take_over(struct transport *transport)\n \n static void standard_options(struct transport *t);\n \n+static int disconnect_helper(struct transport *transport);\n+\n+static int disconnect_helper_data(struct helper_data *data);\n+\n+static int release_helper(struct transport *transport);\n+\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n@@ -155,8 +161,10 @@ static struct child_process *get_helper(struct transport *transport)\n \twhile (1) {\n \t\tconst char *capname, *arg;\n \t\tint mandatory = 0;\n-\t\tif (recvline(data, &buf))\n+\t\tif (recvline(data, &buf)){\n+\t\t\trelease_helper(transport);\n \t\t\texit(128);\n+\t\t}\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n@@ -215,10 +223,14 @@ static struct child_process *get_helper(struct transport *transport)\n \n static int disconnect_helper(struct transport *transport)\n {\n-\tstruct helper_data *data = transport->data;\n+\treturn disconnect_helper_data(transport->data);\n+}\n+\n+static int disconnect_helper_data(struct helper_data *data)\n+{\n \tint res = 0;\n \n-\tif (data->helper) {\n+\tif (data && data->helper) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Disconnecting.\\n\");\n \t\tif (!data->no_disconnect_req) {\n@@ -261,8 +273,10 @@ static int strbuf_set_helper_option(struct helper_data *data,\n \tint ret;\n \n \tsendline(data, buf);\n-\tif (recvline(data, buf))\n+\tif (recvline(data, buf)) {\n+\t\tdisconnect_helper_data(data);\n \t\texit(128);\n+\t}\n \n \tif (!strcmp(buf->buf, \"ok\"))\n \t\tret = 0;\n@@ -366,10 +380,12 @@ static void standard_options(struct transport *t)\n static int release_helper(struct transport *transport)\n {\n \tint res = 0;\n-\tstruct helper_data *data = transport->data;\n-\trefspec_clear(&data->rs);\n-\tres = disconnect_helper(transport);\n-\tfree(transport->data);\n+\tif (transport){\n+\t\tstruct helper_data *data = transport->data;\n+\t\trefspec_clear(&data->rs);\n+\t\tres = disconnect_helper(transport);\n+\t\tfree(transport->data);\n+\t}\n \treturn res;\n }\n \n@@ -394,8 +410,10 @@ static int fetch_with_fetch(struct transport *transport,\n \tsendline(data, &buf);\n \n \twhile (1) {\n-\t\tif (recvline(data, &buf))\n+\t\tif (recvline(data, &buf)){\n+\t\t\trelease_helper(transport);\n \t\t\texit(128);\n+\t\t}\n \n \t\tif (starts_with(buf.buf, \"lock \")) {\n \t\t\tconst char *name = buf.buf + 5;\n@@ -561,8 +579,10 @@ static int run_connect(struct transport *transport, struct strbuf *cmdbuf)\n \tsetvbuf(input, NULL, _IONBF, 0);\n \n \tsendline(data, cmdbuf);\n-\tif (recvline_fh(input, cmdbuf))\n+\tif (recvline_fh(input, cmdbuf)){\n+\t\trelease_helper(transport);\n \t\texit(128);\n+\t}\n \n \tif (!strcmp(cmdbuf->buf, \"\")) {\n \t\tdata->no_disconnect_req = 1;\n@@ -1074,8 +1094,10 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n \n \twhile (1) {\n \t\tchar *eov, *eon;\n-\t\tif (recvline(data, &buf))\n+\t\tif (recvline(data, &buf)){\n+\t\t\trelease_helper(transport);\n \t\t\texit(128);\n+\t\t}\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n-- \n2.21.0\n\n"},{"id":"379174","messageId":"xmqq5znsu04p.fsf@gitster-ct.c.googlers.com","threadId":"51511","inReplyTo":"20190722212250.44011-1-thibault.jamet@gmail.com","subject":"Re: [PATCH] Close transport helper on protocol error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-23T17:33:10Z","receivedAt":"2019-07-23T17:33:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"thibault.jamet@gmail.com writes:\n\n> Subject: Re: [PATCH] Close transport helper on protocol error\n\nPerhaps\n\n    Subject: transport: close helper on protocol error\n\n> +static int disconnect_helper(struct transport *transport);\n> +\n> +static int disconnect_helper_data(struct helper_data *data);\n\nEven after reading it twice, disconnect_helper_data() does not ring\nthe \"this is to disconnect the helper process, based on what is\ncontained in a helper_data instance\" bell, which you wanted to ring.\nIt sounds like it is trying to disconnect \"helper_data\" from\nsomething unsaid.\n\nI think the root cause of this awkwardness is because this split of\nthe function into two is suboptimal.  There is only one existing\ncaller of disconnect_helper() and it passes transport->data (and the\n\"data\" is of type helper_data).  As it is a file-scope static\nfunction, why not just change the type of the parameter from the\nwhole transport to just helper_data, without introducing the new\nfunction to hold the bulk of the original code?\n\n> +static int release_helper(struct transport *transport);\n> +\n>  static struct child_process *get_helper(struct transport *transport)\n>  {\n>  \tstruct helper_data *data = transport->data;\n> @@ -155,8 +161,10 @@ static struct child_process *get_helper(struct transport *transport)\n>  \twhile (1) {\n>  \t\tconst char *capname, *arg;\n>  \t\tint mandatory = 0;\n> -\t\tif (recvline(data, &buf))\n> +\t\tif (recvline(data, &buf)){\n> +\t\t\trelease_helper(transport);\n>  \t\t\texit(128);\n> +\t\t}\n\nThis, together with other exit(128) we see in this patch now have\nrelease_helepr() in front of it, which is in line with what the log\nmessage claims that the patch does.\n\nI however wonder if we want to do a bit more, perhaps with atexit().\nI am not hinting-suggesting to do so (as you said, if the init\nprocess ought to take care of the zombies, the patch under review\nmight already be unneeded, and atexit() makes things even worse),\nbut having trouble to convince that this patch stops at the right\nplace.\n\nThanks.\n"},{"id":"379180","messageId":"20190723194009.GB3879@sigill.intra.peff.net","threadId":"51511","inReplyTo":"xmqq5znsu04p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Close transport helper on protocol error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-23T19:40:10Z","receivedAt":"2019-07-23T19:40:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 23, 2019 at 10:33:10AM -0700, Junio C Hamano wrote:\n\n> > +\t\tif (recvline(data, &buf)){\n> > +\t\t\trelease_helper(transport);\n> >  \t\t\texit(128);\n> > +\t\t}\n> \n> This, together with other exit(128) we see in this patch now have\n> release_helepr() in front of it, which is in line with what the log\n> message claims that the patch does.\n> \n> I however wonder if we want to do a bit more, perhaps with atexit().\n> I am not hinting-suggesting to do so (as you said, if the init\n> process ought to take care of the zombies, the patch under review\n> might already be unneeded, and atexit() makes things even worse),\n> but having trouble to convince that this patch stops at the right\n> place.\n\nI was just writing a similar comment when I read this. It probably fixes\nthe particular case the OP saw, but Git is quite happy to die() in\nvarious code-paths when it encounters an error.\n\nRather than try to annotate every possible exit, atexit() might be a\nbetter solution. But isn't this a more general problem even than that?\nLots of parts of Git may spawn a sub-process and assume that the\nchildren will be reaped automatically (as long as they do exit, but that\nis usually guaranteed by us closing their input pipes when we ourselves\nexit).\n\nSo I think you'd have to atexit() quite a few places. Or possibly\ninstrument run_command() to do so, though it might need some extra\nannotation to mark whether a particular sub-process should be waited for\n(there is some prior art in the child_process.clean_on_exit option).\n\nAt which point I do wonder if this is better handled by a wrapper\nprocess which simply reaps everything. And indeed, people have already\ncome up with similar solutions for containers:\n\n  https://github.com/Yelp/dumb-init\n\nSo I dunno. I am not really opposed to this patch, as it is just adding\nsome extra cleanup. But it seems like it is really hitting the tip of\nthe iceberg, and I'm not sure it's an iceberg I'd like to get to the\nbottom of.\n\n-Peff\n"},{"id":"379189","messageId":"xmqqimrssbhe.fsf@gitster-ct.c.googlers.com","threadId":"51511","inReplyTo":"20190723194009.GB3879@sigill.intra.peff.net","subject":"Re: [PATCH] Close transport helper on protocol error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-23T21:10:53Z","receivedAt":"2019-07-23T21:11:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ...\n> At which point I do wonder if this is better handled by a wrapper\n> process which simply reaps everything. And indeed, people have already\n> come up with similar solutions for containers:\n>\n>   https://github.com/Yelp/dumb-init\n>\n> So I dunno. I am not really opposed to this patch, as it is just adding\n> some extra cleanup. But it seems like it is really hitting the tip of\n> the iceberg, and I'm not sure it's an iceberg I'd like to get to the\n> bottom of.\n\nThanks for stating what I had on mind a lot better than I said ;-)\n"}]}