On Thu, Jul 07, 2022 at 04:17:08PM +0100, Ramsay Jones wrote:
> > 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
