{"thread":{"id":"46476","subject":"[PATCH] credential-cache: interpret an ECONNRESET as on EOF","startedAt":"2017-07-27T01:08:50Z","lastAt":"2017-07-27T15:53:00Z","messageCount":5,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"325177","messageId":"ab9124d9-6630-49f3-d645-2f4251f88764@ramsayjones.plus.com","threadId":"46476","inReplyTo":null,"subject":"[PATCH] credential-cache: interpret an ECONNRESET as on EOF","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-27T01:08:40Z","receivedAt":"2017-07-27T01:08:50Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSince commit 612c49e94d (\"credential-cache: add tests for XDG\nfunctionality\", 17-03-2017), the cygwin build has been failing all the\nnew tests added by that commit. In particular, the 'git credential-cache\nexit' command, as part of the test cleanup code, has been die-ing with\nthe message:\n\n    fatal: read error from cache daemon: Connection reset by peer\n\nAs this git command is part of an && chain in a 'test_when_finished'\ncall, the remaining test cleanup is not happening, so practically all\nremaining tests fail due to the unexpected presence of various socket\nfiles and directories.\n\nA simple means of getting the tests to pass, is to simply ignore the\nfailure of 'git credential-cache exit' command and make sure all test\ncleanup is done. For example, the diff for test #12 would look like:\n\n    diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\n    index fd92533ac..87e5001bb 100755\n    --- a/t/t0301-credential-cache.sh\n    +++ b/t/t0301-credential-cache.sh\n    @@ -17,7 +17,7 @@ helper_test cache\n\n     test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '\n            test_when_finished \"\n    -               git credential-cache exit &&\n    +               (git credential-cache exit || :) &&\n                    rmdir -p .cache/git/credential/\n            \" &&\n            test_path_is_missing \"$HOME/.git-credential-cache\" &&\n\n... and so on for all remaining tests. While this does indeed make all\ntests pass, it is not really a solution.\n\nAs an aside, while looking to debug this issue, I added the '--debug'\noption to the invocation of the 'git-credential-cache--daemon' child\nprocess (see the spawn_daemon() function). This not only fixed the tests,\nbut also stopped git-credential-cache exiting with a failure. Since the\nonly effect of passing '--debug' was to suppress the redirection of stderr\nto the bit-bucket (/dev/null), I have no idea why this seems to fix the\nprotocol interaction between git and git-credential-cache--daemon. (I\ndid think that maybe it was a timing issue, so I tried sleeping before\nreading from the daemon on Linux, but that only slowed down the tests!)\n\nAll descriptions of the \"Connection reset by peer\" error, that I could\nfind, say that the peer had destroyed the connection before the client\nattempted to perform I/O on the connection. Since the daemon does not\nrespond to an \"exit\" message from the client, it just closes the socket\nand deletes the socket file (via the atexit handler), it seems that the\nexpected result is for the client to receive an EOF.  Indeed, this is\nexactly what seems to be happening on Linux. Also a comment in\ncredential-cache--daemon.c reads:\n\n    else if (!strcmp(action.buf, \"exit\")) {\n            /*\n             * It's important that we clean up our socket first, and then\n             * signal the client only once we have finished the cleanup.\n             * Calling exit() directly does this, because we clean up in\n             * our atexit() handler, and then signal the client when our\n             * process actually ends, which closes the socket and gives\n             * them EOF.\n             */\n            exit(0);\n    }\n\nOn cygwin this is not the case, at least when not passing --debug to the\ndaemon, and the read following the \"exit\" gets an error with errno set\nto ECONNRESET.\n\nIn order to suppress the fatal exit in this case, check the read error\nfor an ECONNRESET and return as if no data was read from the daemon.\nThis effectively converts an ECONNRESET into an EOF.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n credential-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex 91550bfb0..1cccc3a0b 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -25,7 +25,7 @@ static int send_request(const char *socket, const struct strbuf *out)\n \t\tint r;\n \n \t\tr = read_in_full(fd, in, sizeof(in));\n-\t\tif (r == 0)\n+\t\tif (r == 0 || (r < 0 && errno == ECONNRESET))\n \t\t\tbreak;\n \t\tif (r < 0)\n \t\t\tdie_errno(\"read error from cache daemon\");\n-- \n2.13.0\n"},{"id":"325184","messageId":"20170727141723.q7eeeajrdkwext5z@sigill.intra.peff.net","threadId":"46476","inReplyTo":"ab9124d9-6630-49f3-d645-2f4251f88764@ramsayjones.plus.com","subject":"Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-27T14:17:23Z","receivedAt":"2017-07-27T14:17:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 27, 2017 at 02:08:40AM +0100, Ramsay Jones wrote:\n\n> In order to suppress the fatal exit in this case, check the read error\n> for an ECONNRESET and return as if no data was read from the daemon.\n> This effectively converts an ECONNRESET into an EOF.\n\nYeah, I think this is a perfectly reasonable thing to do.\n\nI'm not sure if we'd _ever_ see ECONNRESET on Linux. The only time I've\nseen it on Linux (and I coincidentally fixed a bug just like this a week\nor two ago, so it's fresh in my mind): if read() would return EOF but\nthere's outstanding data from a previous write, you get ECONNRESET.\nWhich is obviously going to be totally racy, since \"outstanding\" across\na TCP connection involves more than just the local kernel.\n\nThat shouldn't happen in this 'exit' case. But even if it does, there's\nreally nothing the client should do differently anyway. We're waiting\nfor the other side to exit, and they did. Hooray.\n\nThough your change:\n\n> diff --git a/credential-cache.c b/credential-cache.c\n> index 91550bfb0..1cccc3a0b 100644\n> --- a/credential-cache.c\n> +++ b/credential-cache.c\n> @@ -25,7 +25,7 @@ static int send_request(const char *socket, const struct strbuf *out)\n>  \t\tint r;\n>  \n>  \t\tr = read_in_full(fd, in, sizeof(in));\n> -\t\tif (r == 0)\n> +\t\tif (r == 0 || (r < 0 && errno == ECONNRESET))\n>  \t\t\tbreak;\n>  \t\tif (r < 0)\n>  \t\t\tdie_errno(\"read error from cache daemon\");\n\n...is in the generic send_request(). Which means that it hits all of the\ncommands. So if we were to send a credential and the server\ncrashed midway through reading it, we'd get ECONNRESET. And instead of\nreporting an error, we'd quietly assume the server had no response. But\nagain, I don't think that's really different than the EOF behavior. The\nserver could read our whole request and then crash, and we'd mistakenly\nthink it had nothing to say.\n\nSo I don't think this really changes the robustness much. If we did want\nto address those cases, we'd require actual end-of-record framing in the\nprotocol. But I don't think it's worth caring too much about (this is,\nafter all, a local and internal socket between two relatively simple\nprograms).\n\nWhich is all a verbose way of saying that your patch looks good to me.\nThanks for digging up the root cause of the test failures.\n\n-Peff\n"},{"id":"325187","messageId":"6574cd23-5d95-c23c-8179-755798472a9d@ramsayjones.plus.com","threadId":"46476","inReplyTo":"20170727141723.q7eeeajrdkwext5z@sigill.intra.peff.net","subject":"Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-27T14:42:36Z","receivedAt":"2017-07-27T14:42:46Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 27/07/17 15:17, Jeff King wrote:\n> On Thu, Jul 27, 2017 at 02:08:40AM +0100, Ramsay Jones wrote:\n> \n>> In order to suppress the fatal exit in this case, check the read error\n>> for an ECONNRESET and return as if no data was read from the daemon.\n>> This effectively converts an ECONNRESET into an EOF.\n> \n> Yeah, I think this is a perfectly reasonable thing to do.\n> \n> I'm not sure if we'd _ever_ see ECONNRESET on Linux. The only time I've\n> seen it on Linux (and I coincidentally fixed a bug just like this a week\n> or two ago, so it's fresh in my mind): if read() would return EOF but\n> there's outstanding data from a previous write, you get ECONNRESET.\n> Which is obviously going to be totally racy, since \"outstanding\" across\n> a TCP connection involves more than just the local kernel.\n> \n> That shouldn't happen in this 'exit' case. But even if it does, there's\n> really nothing the client should do differently anyway. We're waiting\n> for the other side to exit, and they did. Hooray.\n> \n> Though your change:\n> \n>> diff --git a/credential-cache.c b/credential-cache.c\n>> index 91550bfb0..1cccc3a0b 100644\n>> --- a/credential-cache.c\n>> +++ b/credential-cache.c\n>> @@ -25,7 +25,7 @@ static int send_request(const char *socket, const struct strbuf *out)\n>>  \t\tint r;\n>>  \n>>  \t\tr = read_in_full(fd, in, sizeof(in));\n>> -\t\tif (r == 0)\n>> +\t\tif (r == 0 || (r < 0 && errno == ECONNRESET))\n>>  \t\t\tbreak;\n>>  \t\tif (r < 0)\n>>  \t\t\tdie_errno(\"read error from cache daemon\");\n> \n> ...is in the generic send_request(). Which means that it hits all of the\n> commands. So if we were to send a credential and the server\n> crashed midway through reading it, we'd get ECONNRESET. And instead of\n> reporting an error, we'd quietly assume the server had no response. But\n> again, I don't think that's really different than the EOF behavior. The\n> server could read our whole request and then crash, and we'd mistakenly\n> think it had nothing to say.\n\nyep, I did think about adding a FLAG_EXIT (or somesuch) and passing\nit down through the call stack to send_request() so that I could do\nthe check only for the 'exit' command. I decided against it in the\nend, obviously! ;-)\n\n> So I don't think this really changes the robustness much. If we did want\n> to address those cases, we'd require actual end-of-record framing in the\n> protocol. But I don't think it's worth caring too much about (this is,\n> after all, a local and internal socket between two relatively simple\n> programs).\n\nAdding the framing to the protocol was actually my first (very fleeting)\nidea. However, I didn't want to get into the 'supporting old/new client\nand server combos' problem.\n\n> Which is all a verbose way of saying that your patch looks good to me.\n> Thanks for digging up the root cause of the test failures.\n\nThanks!\n\nATB,\nRamsay Jones\n\n"},{"id":"325188","messageId":"20170727144725.a72cybli746ipiko@sigill.intra.peff.net","threadId":"46476","inReplyTo":"6574cd23-5d95-c23c-8179-755798472a9d@ramsayjones.plus.com","subject":"Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-27T14:47:25Z","receivedAt":"2017-07-27T14:47:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 27, 2017 at 03:42:36PM +0100, Ramsay Jones wrote:\n\n> yep, I did think about adding a FLAG_EXIT (or somesuch) and passing\n> it down through the call stack to send_request() so that I could do\n> the check only for the 'exit' command. I decided against it in the\n> end, obviously! ;-)\n\nI had the same thought. I don't think it's worth the trouble, either.\n\n> > So I don't think this really changes the robustness much. If we did want\n> > to address those cases, we'd require actual end-of-record framing in the\n> > protocol. But I don't think it's worth caring too much about (this is,\n> > after all, a local and internal socket between two relatively simple\n> > programs).\n> \n> Adding the framing to the protocol was actually my first (very fleeting)\n> idea. However, I didn't want to get into the 'supporting old/new client\n> and server combos' problem.\n\nOne nice thing about this protocol is that it's just between the caching\nclient and server, which ship together and which run for a default of 5\nminutes. I think it would probably be OK to just assume you have a\nmatched pair, and change the protocol as you like.\n\nSo I'd be fine if somebody wanted to tackle this, but I don't think it\nmatters that much. After all, it's proxying requests that came over the\ncredential helper protocol, which also isn't framed. And changing that\none _would_ be a compatibility problem.\n\n-Peff\n"},{"id":"325191","messageId":"ce4630ad-78e9-3163-1582-773b0e9fde02@ramsayjones.plus.com","threadId":"46476","inReplyTo":"ab9124d9-6630-49f3-d645-2f4251f88764@ramsayjones.plus.com","subject":"Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-27T15:52:53Z","receivedAt":"2017-07-27T15:53:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Junio,\n\nI just noticed a typo in the subject line for this patch. It should\nread '... ECONNRESET as _an_ EOF' not '... ECONNRESET as _on_ EOF'.\n\n[complete patch snipped!]\n\nThanks!\n\nATB,\nRamsay Jones\n"}]}