{"thread":{"id":"37556","subject":"[PATCH] credential-cache: close stderr in daemon process","startedAt":"2014-09-14T07:35:06Z","lastAt":"2014-09-16T18:10:29Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249379","messageId":"20140914073506.GA19667@peff.net","threadId":"37556","inReplyTo":null,"subject":"[PATCH] credential-cache: close stderr in daemon process","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-14T07:35:06Z","receivedAt":"2014-09-14T07:35:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the stderr of \"git credential-cache\" is redirected to a\npipe, the reader on the other end of a pipe may be surprised\nthat the pipe remains open long after the process exits.\nThis happens because we may auto-spawn a daemon which is\nlong-lived, and which keeps stderr open.\n\nWe can solve this by redirecting the daemon's stderr to\n/dev/null once we are ready to go into our event loop. We\nwould not want to do so before then, because we may want to\nreport errors about the setup (e.g., failure to establish\nthe listening socket).\n\nThis does mean that we will not report errors we encounter\nfor specific clients. That's acceptable, as such errors\nshould be rare (e.g., clients sending buggy requests).\nHowever, we also provide an escape hatch: if you want to see\nthese later messages, you can provide the \"--debug\" option\nto keep stderr open.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-credential-cache--daemon.txt |  6 +++++-\n credential-cache--daemon.c                     | 23 +++++++++++++++++++----\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-credential-cache--daemon.txt b/Documentation/git-credential-cache--daemon.txt\nindex d15db42..7051c6b 100644\n--- a/Documentation/git-credential-cache--daemon.txt\n+++ b/Documentation/git-credential-cache--daemon.txt\n@@ -8,7 +8,7 @@ git-credential-cache--daemon - Temporarily store user credentials in memory\n SYNOPSIS\n --------\n [verse]\n-git credential-cache--daemon <socket>\n+git credential-cache--daemon [--debug] <socket>\n \n DESCRIPTION\n -----------\n@@ -21,6 +21,10 @@ for `git-credential-cache` clients. Clients may store and retrieve\n credentials. Each credential is held for a timeout specified by the\n client; once no credentials are held, the daemon exits.\n \n+If the `--debug` option is specified, the daemon does not close its\n+stderr stream, and may output extra diagnostics to it even after it has\n+begun listening for clients.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 3b370ca..c07a67c 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -2,6 +2,7 @@\n #include \"credential.h\"\n #include \"unix-socket.h\"\n #include \"sigchain.h\"\n+#include \"parse-options.h\"\n \n static const char *socket_path;\n \n@@ -201,7 +202,7 @@ static int serve_cache_loop(int fd)\n \treturn 1;\n }\n \n-static void serve_cache(const char *socket_path)\n+static void serve_cache(const char *socket_path, int debug)\n {\n \tint fd;\n \n@@ -211,6 +212,8 @@ static void serve_cache(const char *socket_path)\n \n \tprintf(\"ok\\n\");\n \tfclose(stdout);\n+\tif (!debug)\n+\t\tfreopen(\"/dev/null\", \"w\", stderr);\n \n \twhile (serve_cache_loop(fd))\n \t\t; /* nothing */\n@@ -252,16 +255,28 @@ static void check_socket_directory(const char *path)\n \n int main(int argc, const char **argv)\n {\n-\tsocket_path = argv[1];\n+\tstatic const char *usage[] = {\n+\t\t\"git-credential-cache--daemon [opts] <socket_path>\",\n+\t\tNULL\n+\t};\n+\tint debug = 0;\n+\tconst struct option options[] = {\n+\t\tOPT_BOOL(0, \"debug\", &debug,\n+\t\t\t N_(\"print debugging messages to stderr\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options, usage, 0);\n+\tsocket_path = argv[0];\n \n \tif (!socket_path)\n-\t\tdie(\"usage: git-credential-cache--daemon <socket_path>\");\n+\t\tusage_with_options(usage, options);\n \tcheck_socket_directory(socket_path);\n \n \tatexit(cleanup_socket);\n \tsigchain_push_common(cleanup_socket_on_signal);\n \n-\tserve_cache(socket_path);\n+\tserve_cache(socket_path, debug);\n \n \treturn 0;\n }\n-- \n2.1.0.373.g91ca799\n"},{"id":"249460","messageId":"xmqqoaug4la4.fsf@gitster.dls.corp.google.com","threadId":"37556","inReplyTo":"20140914073506.GA19667@peff.net","subject":"Re: [PATCH] credential-cache: close stderr in daemon process","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-15T21:38:11Z","receivedAt":"2014-09-15T21:38:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +\tif (!debug)\n> +\t\tfreopen(\"/dev/null\", \"w\", stderr);\n\nI am getting this:\n\ncredential-cache--daemon.c:216:10: error: ignoring return value of\n'freopen', declared with attribute warn_unused_result\n[-Werror=unused-result]\n\nwhich is somewhat irritating.  Even though I am not irritated by\nthis code, but by the compiler and glibc headers, this is apparently\nthe only offending one, so we may want to fix it anyway.\n"},{"id":"249498","messageId":"20140916002825.GD5019@peff.net","threadId":"37556","inReplyTo":"xmqqoaug4la4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] credential-cache: close stderr in daemon process","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-16T00:28:25Z","receivedAt":"2014-09-16T00:28:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 15, 2014 at 02:38:11PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +\tif (!debug)\n> > +\t\tfreopen(\"/dev/null\", \"w\", stderr);\n> \n> I am getting this:\n> \n> credential-cache--daemon.c:216:10: error: ignoring return value of\n> 'freopen', declared with attribute warn_unused_result\n> [-Werror=unused-result]\n\nHmph, my glibc does not seem to mark freopen. It's probably sane to\ncheck it for error, as we would otherwise probably segfault on the next\ncall to \"fprintf(stderr)\".\n\n>From my reading of freopen(3posix), I do not think we need to do\nanything like \"stderr = freopen(..., stderr);\". The filehandle is\nrepointed, not re-allocated. And anyway, that is the assumption all of\nour other freopen calls make. :)\n\n> which is somewhat irritating.  Even though I am not irritated by\n> this code, but by the compiler and glibc headers, this is apparently\n> the only offending one, so we may want to fix it anyway.\n\nSquash this in?\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex c07a67c..c2f0049 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -212,8 +212,10 @@ static void serve_cache(const char *socket_path, int debug)\n \n \tprintf(\"ok\\n\");\n \tfclose(stdout);\n-\tif (!debug)\n-\t\tfreopen(\"/dev/null\", \"w\", stderr);\n+\tif (!debug) {\n+\t\tif (!freopen(\"/dev/null\", \"w\", stderr))\n+\t\t\tdie_errno(\"unable to point stderr to /dev/null\");\n+\t}\n \n \twhile (serve_cache_loop(fd))\n \t\t; /* nothing */\n"},{"id":"249510","messageId":"xmqqtx47308a.fsf@gitster.dls.corp.google.com","threadId":"37556","inReplyTo":"20140916002825.GD5019@peff.net","subject":"Re: [PATCH] credential-cache: close stderr in daemon process","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-16T18:10:29Z","receivedAt":"2014-09-16T18:10:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Squash this in?\n\nYeah, I did a crude one without _errno() while sending the report;\nwill replace.\n\nThanks.\n\n>\n> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\n> index c07a67c..c2f0049 100644\n> --- a/credential-cache--daemon.c\n> +++ b/credential-cache--daemon.c\n> @@ -212,8 +212,10 @@ static void serve_cache(const char *socket_path, int debug)\n>  \n>  \tprintf(\"ok\\n\");\n>  \tfclose(stdout);\n> -\tif (!debug)\n> -\t\tfreopen(\"/dev/null\", \"w\", stderr);\n> +\tif (!debug) {\n> +\t\tif (!freopen(\"/dev/null\", \"w\", stderr))\n> +\t\t\tdie_errno(\"unable to point stderr to /dev/null\");\n> +\t}\n>  \n>  \twhile (serve_cache_loop(fd))\n>  \t\t; /* nothing */\n"}]}