[PATCH] credential-cache: interpret an ECONNRESET as on EOF

[PATCH] credential-cache: interpret an ECONNRESET as on EOF

[Ramsay Jones]

Since commit 612c49e94d ("credential-cache: add tests for XDG
functionality", 17-03-2017), the cygwin build has been failing all the
new tests added by that commit. In particular, the 'git credential-cache
exit' command, as part of the test cleanup code, has been die-ing with
the message:

    fatal: read error from cache daemon: Connection reset by peer

As this git command is part of an && chain in a 'test_when_finished'
call, the remaining test cleanup is not happening, so practically all
remaining tests fail due to the unexpected presence of various socket
files and directories.

A simple means of getting the tests to pass, is to simply ignore the
failure of 'git credential-cache exit' command and make sure all test
cleanup is done. For example, the diff for test #12 would look like:

    diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
    index fd92533ac..87e5001bb 100755
    --- a/t/t0301-credential-cache.sh
    +++ b/t/t0301-credential-cache.sh
    @@ -17,7 +17,7 @@ helper_test cache

     test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
            test_when_finished "
    -               git credential-cache exit &&
    +               (git credential-cache exit || :) &&
                    rmdir -p .cache/git/credential/
            " &&
            test_path_is_missing "$HOME/.git-credential-cache" &&

... and so on for all remaining tests. While this does indeed make all
tests pass, it is not really a solution.

As an aside, while looking to debug this issue, I added the '--debug'
option to the invocation of the 'git-credential-cache--daemon' child
process (see the spawn_daemon() function). This not only fixed the tests,
but also stopped git-credential-cache exiting with a failure. Since the
only effect of passing '--debug' was to suppress the redirection of stderr
to the bit-bucket (/dev/null), I have no idea why this seems to fix the
protocol interaction between git and git-credential-cache--daemon. (I
did think that maybe it was a timing issue, so I tried sleeping before
reading from the daemon on Linux, but that only slowed down the tests!)

All descriptions of the "Connection reset by peer" error, that I could
find, say that the peer had destroyed the connection before the client
attempted to perform I/O on the connection. Since the daemon does not
respond to an "exit" message from the client, it just closes the socket
and deletes the socket file (via the atexit handler), it seems that the
expected result is for the client to receive an EOF.  Indeed, this is
exactly what seems to be happening on Linux. Also a comment in
credential-cache--daemon.c reads:

    else if (!strcmp(action.buf, "exit")) {
            /*
             * It's important that we clean up our socket first, and then
             * signal the client only once we have finished the cleanup.
             * Calling exit() directly does this, because we clean up in
             * our atexit() handler, and then signal the client when our
             * process actually ends, which closes the socket and gives
             * them EOF.
             */
            exit(0);
    }

On cygwin this is not the case, at least when not passing --debug to the
daemon, and the read following the "exit" gets an error with errno set
to ECONNRESET.

In order to suppress the fatal exit in this case, check the read error
for an ECONNRESET and return as if no data was read from the daemon.
This effectively converts an ECONNRESET into an EOF.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index 91550bfb0..1cccc3a0b 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -25,7 +25,7 @@ static int send_request(const char *socket, const struct strbuf *out)
 		int r;
 
 		r = read_in_full(fd, in, sizeof(in));
-		if (r == 0)
+		if (r == 0 || (r < 0 && errno == ECONNRESET))
 			break;
 		if (r < 0)
 			die_errno("read error from cache daemon");
-- 
2.13.0

Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF

[Jeff King]

On Thu, Jul 27, 2017 at 02:08:40AM +0100, Ramsay Jones wrote:

> In order to suppress the fatal exit in this case, check the read error
> for an ECONNRESET and return as if no data was read from the daemon.
> This effectively converts an ECONNRESET into an EOF.

Yeah, I think this is a perfectly reasonable thing to do.

I'm not sure if we'd _ever_ see ECONNRESET on Linux. The only time I've
seen it on Linux (and I coincidentally fixed a bug just like this a week
or two ago, so it's fresh in my mind): if read() would return EOF but
there's outstanding data from a previous write, you get ECONNRESET.
Which is obviously going to be totally racy, since "outstanding" across
a TCP connection involves more than just the local kernel.

That shouldn't happen in this 'exit' case. But even if it does, there's
really nothing the client should do differently anyway. We're waiting
for the other side to exit, and they did. Hooray.

Though your change:

> diff --git a/credential-cache.c b/credential-cache.c
> index 91550bfb0..1cccc3a0b 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -25,7 +25,7 @@ static int send_request(const char *socket, const struct strbuf *out)
>  		int r;
>  
>  		r = read_in_full(fd, in, sizeof(in));
> -		if (r == 0)
> +		if (r == 0 || (r < 0 && errno == ECONNRESET))
>  			break;
>  		if (r < 0)
>  			die_errno("read error from cache daemon");

...is in the generic send_request(). Which means that it hits all of the
commands. So if we were to send a credential and the server
crashed midway through reading it, we'd get ECONNRESET. And instead of
reporting an error, we'd quietly assume the server had no response. But
again, I don't think that's really different than the EOF behavior. The
server could read our whole request and then crash, and we'd mistakenly
think it had nothing to say.

So I don't think this really changes the robustness much. If we did want
to address those cases, we'd require actual end-of-record framing in the
protocol. But I don't think it's worth caring too much about (this is,
after all, a local and internal socket between two relatively simple
programs).

Which is all a verbose way of saying that your patch looks good to me.
Thanks for digging up the root cause of the test failures.

-Peff

Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF

[Ramsay Jones]

On 27/07/17 15:17, Jeff King wrote:
> On Thu, Jul 27, 2017 at 02:08:40AM +0100, Ramsay Jones wrote:
> 
>> In order to suppress the fatal exit in this case, check the read error
>> for an ECONNRESET and return as if no data was read from the daemon.
>> This effectively converts an ECONNRESET into an EOF.
> 
> Yeah, I think this is a perfectly reasonable thing to do.
> 
> I'm not sure if we'd _ever_ see ECONNRESET on Linux. The only time I've
> seen it on Linux (and I coincidentally fixed a bug just like this a week
> or two ago, so it's fresh in my mind): if read() would return EOF but
> there's outstanding data from a previous write, you get ECONNRESET.
> Which is obviously going to be totally racy, since "outstanding" across
> a TCP connection involves more than just the local kernel.
> 
> That shouldn't happen in this 'exit' case. But even if it does, there's
> really nothing the client should do differently anyway. We're waiting
> for the other side to exit, and they did. Hooray.
> 
> Though your change:
> 
>> diff --git a/credential-cache.c b/credential-cache.c
>> index 91550bfb0..1cccc3a0b 100644
>> --- a/credential-cache.c
>> +++ b/credential-cache.c
>> @@ -25,7 +25,7 @@ static int send_request(const char *socket, const struct strbuf *out)
>>  		int r;
>>  
>>  		r = read_in_full(fd, in, sizeof(in));
>> -		if (r == 0)
>> +		if (r == 0 || (r < 0 && errno == ECONNRESET))
>>  			break;
>>  		if (r < 0)
>>  			die_errno("read error from cache daemon");
> 
> ...is in the generic send_request(). Which means that it hits all of the
> commands. So if we were to send a credential and the server
> crashed midway through reading it, we'd get ECONNRESET. And instead of
> reporting an error, we'd quietly assume the server had no response. But
> again, I don't think that's really different than the EOF behavior. The
> server could read our whole request and then crash, and we'd mistakenly
> think it had nothing to say.

yep, I did think about adding a FLAG_EXIT (or somesuch) and passing
it down through the call stack to send_request() so that I could do
the check only for the 'exit' command. I decided against it in the
end, obviously! ;-)

> So I don't think this really changes the robustness much. If we did want
> to address those cases, we'd require actual end-of-record framing in the
> protocol. But I don't think it's worth caring too much about (this is,
> after all, a local and internal socket between two relatively simple
> programs).

Adding the framing to the protocol was actually my first (very fleeting)
idea. However, I didn't want to get into the 'supporting old/new client
and server combos' problem.

> Which is all a verbose way of saying that your patch looks good to me.
> Thanks for digging up the root cause of the test failures.

Thanks!

ATB,
Ramsay Jones

Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF

[Jeff King]

On Thu, Jul 27, 2017 at 03:42:36PM +0100, Ramsay Jones wrote:

> yep, I did think about adding a FLAG_EXIT (or somesuch) and passing
> it down through the call stack to send_request() so that I could do
> the check only for the 'exit' command. I decided against it in the
> end, obviously! ;-)

I had the same thought. I don't think it's worth the trouble, either.

> > So I don't think this really changes the robustness much. If we did want
> > to address those cases, we'd require actual end-of-record framing in the
> > protocol. But I don't think it's worth caring too much about (this is,
> > after all, a local and internal socket between two relatively simple
> > programs).
> 
> Adding the framing to the protocol was actually my first (very fleeting)
> idea. However, I didn't want to get into the 'supporting old/new client
> and server combos' problem.

One nice thing about this protocol is that it's just between the caching
client and server, which ship together and which run for a default of 5
minutes. I think it would probably be OK to just assume you have a
matched pair, and change the protocol as you like.

So I'd be fine if somebody wanted to tackle this, but I don't think it
matters that much. After all, it's proxying requests that came over the
credential helper protocol, which also isn't framed. And changing that
one _would_ be a compatibility problem.

-Peff

Re: [PATCH] credential-cache: interpret an ECONNRESET as on EOF

[Ramsay Jones]

Hi Junio,

I just noticed a typo in the subject line for this patch. It should
read '... ECONNRESET as _an_ EOF' not '... ECONNRESET as _on_ EOF'.

[complete patch snipped!]

Thanks!

ATB,
Ramsay Jones