Re: t0301-credential-cache test failure on cygwin

From: Jeff King <peff@peff.net>
Date: Jul 7, 2022, 18:19 UTC
Message-ID: <YscjxOWqf7GUvnps@coredump.intra.peff.net>
In-Reply-To: <4529b11a-e514-6676-f427-ffaec484e8f1@ramsayjones.plus.com>

On Thu, Jul 07, 2022 at 04:17:08PM +0100, Ramsay Jones wrote:

Show 13 quoted lines

> > If this codepath was written like this (i.e. [PATCH 1C]) from the
> > beginning, I would have found it very sensible (i.e. instead of
> > caling exit() in the middle of the infinite client serving loop,
> > exiting the loop cleanly is easier to follow and maintain), even if
> > we didn't know the issue on Cygwin you investigated.
> 
> Yep, apart from the variable name, I quite like the approach taken by
> the 1C patch.
> [...]
> Also, I would like to understand why the code is written as it is
> currently. I'm sure there must be a good reason - I just don't know
> what it is! I suspect (ie I'm guessing), it has something to do with
> operating in a high contention context [TOCTOU on socket?] ... dunno. ;-)

I wrote a longer reply in the thread, but just to be clear here: your 1C will indeed introduce a race.

IMHO it is not worth switching away from the current code which calls exit() to return up the stock. But if you wanted to do so without introducing a race, I think you could call delete_tempfile() before closing any streams, like this (on top of your 1C):

diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index fb9b1e04a6..9b9cc1b70e 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -131,6 +131,7 @@ static int serve_one_client(FILE *in, FILE *out)
 		}
 	}
 	else if (!strcmp(action.buf, "exit")) {
+		delete_tempfile(&socket_file);
 		/* stop serving clients */
 		serve = 0;
 	}

but you'd have to make the socket_file struct globally available. And of
course it does not fix your cygwin problem, which I believe is mutually
exclusive with keeping the race-free ordering. ;)

-Peff