threads / patch / 37556

patchcredential-cache: close stderr in daemon process

Subject: [PATCH] credential-cache: close stderr in daemon process

## tl;dr

4 messages between Sep 14, 2014 and Sep 16, 2014. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jeff King· Sep 14, 2014, 07:35 UTC · lore

If the stderr of "git credential-cache" is redirected to a pipe, the reader on the other end of a pipe may be surprised that the pipe remains open long after the process exits. This happens because we may auto-spawn a daemon which is long-lived, and which keeps stderr open.

We can solve this by redirecting the daemon's stderr to /dev/null once we are ready to go into our event loop. We would not want to do so before then, because we may want to report errors about the setup (e.g., failure to establish the listening socket).

This does mean that we will not report errors we encounter for specific clients. That's acceptable, as such errors should be rare (e.g., clients sending buggy requests). However, we also provide an escape hatch: if you want to see these later messages, you can provide the "--debug" option to keep stderr open.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-credential-cache--daemon.txt |  6 +++++-
 credential-cache--daemon.c                     | 23 +++++++++++++++++++----
 2 files changed, 24 insertions(+), 5 deletions(-)
Show changes to 2 files +24 −5

Documentation/git-credential-cache--daemon.txt, credential-cache--daemon.c

diff --git a/Documentation/git-credential-cache--daemon.txt b/Documentation/git-credential-cache--daemon.txt
index d15db42..7051c6b 100644
--- a/Documentation/git-credential-cache--daemon.txt
+++ b/Documentation/git-credential-cache--daemon.txt
@@ -8,7 +8,7 @@ git-credential-cache--daemon - Temporarily store user credentials in memory
 SYNOPSIS
 --------
 [verse]
-git credential-cache--daemon <socket>
+git credential-cache--daemon [--debug] <socket>
 
 DESCRIPTION
 -----------
@@ -21,6 +21,10 @@ for `git-credential-cache` clients. Clients may store and retrieve
 credentials. Each credential is held for a timeout specified by the
 client; once no credentials are held, the daemon exits.
 
+If the `--debug` option is specified, the daemon does not close its
+stderr stream, and may output extra diagnostics to it even after it has
+begun listening for clients.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 3b370ca..c07a67c 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -2,6 +2,7 @@
 #include "credential.h"
 #include "unix-socket.h"
 #include "sigchain.h"
+#include "parse-options.h"
 
 static const char *socket_path;
 
@@ -201,7 +202,7 @@ static int serve_cache_loop(int fd)
 	return 1;
 }
 
-static void serve_cache(const char *socket_path)
+static void serve_cache(const char *socket_path, int debug)
 {
 	int fd;
 
@@ -211,6 +212,8 @@ static void serve_cache(const char *socket_path)
 
 	printf("ok\n");
 	fclose(stdout);
+	if (!debug)
+		freopen("/dev/null", "w", stderr);
 
 	while (serve_cache_loop(fd))
 		; /* nothing */
@@ -252,16 +255,28 @@ static void check_socket_directory(const char *path)
 
 int main(int argc, const char **argv)
 {
-	socket_path = argv[1];
+	static const char *usage[] = {
+		"git-credential-cache--daemon [opts] <socket_path>",
+		NULL
+	};
+	int debug = 0;
+	const struct option options[] = {
+		OPT_BOOL(0, "debug", &debug,
+			 N_("print debugging messages to stderr")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, NULL, options, usage, 0);
+	socket_path = argv[0];
 
 	if (!socket_path)
-		die("usage: git-credential-cache--daemon <socket_path>");
+		usage_with_options(usage, options);
 	check_socket_directory(socket_path);
 
 	atexit(cleanup_socket);
 	sigchain_push_common(cleanup_socket_on_signal);
 
-	serve_cache(socket_path);
+	serve_cache(socket_path, debug);
 
 	return 0;
 }
-- 
2.1.0.373.g91ca799
Junio C Hamano· Sep 15, 2014, 21:38 UTC · re: Jeff King · lore

Re: [PATCH] credential-cache: close stderr in daemon process

Jeff King <peff@peff.net> writes:
> +	if (!debug)
> +		freopen("/dev/null", "w", stderr);
I am getting this:

credential-cache--daemon.c:216:10: error: ignoring return value of 'freopen', declared with attribute warn_unused_result [-Werror=unused-result]

which is somewhat irritating. Even though I am not irritated by this code, but by the compiler and glibc headers, this is apparently the only offending one, so we may want to fix it anyway.

Jeff King· Sep 16, 2014, 00:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] credential-cache: close stderr in daemon process

On Mon, Sep 15, 2014 at 02:38:11PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > +	if (!debug)
> > +		freopen("/dev/null", "w", stderr);
> 
> I am getting this:
> 
> credential-cache--daemon.c:216:10: error: ignoring return value of
> 'freopen', declared with attribute warn_unused_result
> [-Werror=unused-result]

Hmph, my glibc does not seem to mark freopen. It's probably sane to check it for error, as we would otherwise probably segfault on the next call to "fprintf(stderr)".

>From my reading of freopen(3posix), I do not think we need to do

anything like "stderr = freopen(..., stderr);". The filehandle is repointed, not re-allocated. And anyway, that is the assumption all of our other freopen calls make. :)

> which is somewhat irritating.  Even though I am not irritated by
> this code, but by the compiler and glibc headers, this is apparently
> the only offending one, so we may want to fix it anyway.
Squash this in?
Show changes to credential-cache--daemon.c +4 −2
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index c07a67c..c2f0049 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -212,8 +212,10 @@ static void serve_cache(const char *socket_path, int debug)
 
 	printf("ok\n");
 	fclose(stdout);
-	if (!debug)
-		freopen("/dev/null", "w", stderr);
+	if (!debug) {
+		if (!freopen("/dev/null", "w", stderr))
+			die_errno("unable to point stderr to /dev/null");
+	}
 
 	while (serve_cache_loop(fd))
 		; /* nothing */
Junio C Hamano· Sep 16, 2014, 18:10 UTC · re: Jeff King · lore

Re: [PATCH] credential-cache: close stderr in daemon process

Jeff King <peff@peff.net> writes:
> Squash this in?

Yeah, I did a crude one without _errno() while sending the report; will replace.

Thanks.
Show 18 quoted lines
>
> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
> index c07a67c..c2f0049 100644
> --- a/credential-cache--daemon.c
> +++ b/credential-cache--daemon.c
> @@ -212,8 +212,10 @@ static void serve_cache(const char *socket_path, int debug)
>  
>  	printf("ok\n");
>  	fclose(stdout);
> -	if (!debug)
> -		freopen("/dev/null", "w", stderr);
> +	if (!debug) {
> +		if (!freopen("/dev/null", "w", stderr))
> +			die_errno("unable to point stderr to /dev/null");
> +	}
>  
>  	while (serve_cache_loop(fd))
>  		; /* nothing */

← back to recent threads