{"thread":{"id":"41480","subject":"[PATCH 0/3] credential-cache: give daemon a predictable cwd","startedAt":"2016-02-23T07:14:27Z","lastAt":"2016-02-23T21:10:46Z","messageCount":8,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"278980","messageId":"20160223071427.GA7489@sigill.intra.peff.net","threadId":"41480","inReplyTo":null,"subject":"[PATCH 0/3] credential-cache: give daemon a predictable cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-23T07:14:27Z","receivedAt":"2016-02-23T07:14:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The credential-cache--daemon may live forever in whatever directory it\nhappened to be spawned in (in practice, probably the root of some git\nrepo's work tree). This can prevent umount from unmounting a filesystem\nthat contains that directory, even though credential-cache doesn't care\none way or the other.\n\nJon Griffiths sent a patch to me and to the list, but it seems that vger\nblocks all mail from yahoo, including him. Eep. So he and I had some\noff-list back-and-forth, and ended up at this series, which I agreed to\npass along.\n\nI've made a few minor tweaks from the last version he sent me. Hopefully\nnothing he disagrees with, but of course he has no means to complain to\nthe list if so. :)\n\n [1/3]: credential-cache--daemon: refactor check_socket_directory\n [2/3]: credential-cache--daemon: disallow relative socket path\n [3/3]: credential-cache--daemon: change to the socket dir on startup\n\n-Peff\n"},{"id":"278981","messageId":"20160223071515.GA8395@sigill.intra.peff.net","threadId":"41480","inReplyTo":"20160223071427.GA7489@sigill.intra.peff.net","subject":"[PATCH 1/3] credential-cache--daemon: refactor check_socket_directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-23T07:15:15Z","receivedAt":"2016-02-23T07:15:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jon Griffiths \n\nThis function does an early return, and therefore has to\nrepeat its cleanup. We can stick the later bit of the\nfunction into an \"else\" and avoid duplicating the shared\npart (which will get bigger in a future patch).\n\nLet's also rename the function to init_socket_directory. It\nnot only checks the directory but also creates it. Saying\n\"init\" is more accurate.\n\nSigned-off-by: Jon Griffiths \nSigned-off-by: Jeff King \n---\n credential-cache--daemon.c | 28 +++++++++++++---------------\n 1 file changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex cc65a9c..3403f48 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -215,7 +215,7 @@ static const char permissions_advice[] =\n \"users may be able to read your cached credentials. Consider running:\\n\"\n \"\\n\"\n \"\tchmod 0700 %s\";\n-static void check_socket_directory(const char *path)\n+static void init_socket_directory(const char *path)\n {\n \tstruct stat st;\n \tchar *path_copy = xstrdup(path);\n@@ -224,20 +224,18 @@ static void check_socket_directory(const char *path)\n \tif (!stat(dir, &st)) {\n \t\tif (st.st_mode & 077)\n \t\t\tdie(permissions_advice, dir);\n-\t\tfree(path_copy);\n-\t\treturn;\n+\t} else {\n+\t\t/*\n+\t\t * We must be sure to create the directory with the correct mode,\n+\t\t * not just chmod it after the fact; otherwise, there is a race\n+\t\t * condition in which somebody can chdir to it, sleep, then try to open\n+\t\t * our protected socket.\n+\t\t */\n+\t\tif (safe_create_leading_directories_const(dir) < 0)\n+\t\t\tdie_errno(\"unable to create directories for '%s'\", dir);\n+\t\tif (mkdir(dir, 0700) < 0)\n+\t\t\tdie_errno(\"unable to mkdir '%s'\", dir);\n \t}\n-\n-\t/*\n-\t * We must be sure to create the directory with the correct mode,\n-\t * not just chmod it after the fact; otherwise, there is a race\n-\t * condition in which somebody can chdir to it, sleep, then try to open\n-\t * our protected socket.\n-\t */\n-\tif (safe_create_leading_directories_const(dir) < 0)\n-\t\tdie_errno(\"unable to create directories for '%s'\", dir);\n-\tif (mkdir(dir, 0700) < 0)\n-\t\tdie_errno(\"unable to mkdir '%s'\", dir);\n \tfree(path_copy);\n }\n \n@@ -264,7 +262,7 @@ int main(int argc, const char **argv)\n \tif (!socket_path)\n \t\tusage_with_options(usage, options);\n \n-\tcheck_socket_directory(socket_path);\n+\tinit_socket_directory(socket_path);\n \tregister_tempfile(&socket_file, socket_path);\n \n \tif (ignore_sighup)\n-- \n2.7.2.645.g4e1306c\n"},{"id":"278982","messageId":"20160223071541.GB8395@sigill.intra.peff.net","threadId":"41480","inReplyTo":"20160223071427.GA7489@sigill.intra.peff.net","subject":"[PATCH 2/3] credential-cache--daemon: disallow relative socket path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-23T07:15:41Z","receivedAt":"2016-02-23T07:15:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jon Griffiths \n\nRelative socket paths are dangerous since the user cannot generally\ncontrol when the daemon starts (initially, after a timeout, kill or\ncrash). Since the daemon creates but does not delete the socket\ndirectory, this could lead to spurious directory creation relative\nto the users cwd.\n\nSuggested-by: Jeff King \nSigned-off-by: Jon Griffiths \nSigned-off-by: Jeff King \n---\n Documentation/git-credential-cache.txt | 2 +-\n credential-cache--daemon.c | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt\nindex 89b7306..96208f8 100644\n--- a/Documentation/git-credential-cache.txt\n+++ b/Documentation/git-credential-cache.txt\n@@ -36,7 +36,7 @@ OPTIONS\n \tcache daemon if one is not started). Defaults to\n \t`~/.git-credential-cache/socket`. If your home directory is on a\n \tnetwork-mounted filesystem, you may need to change this to a\n-\tlocal filesystem.\n+\tlocal filesystem. You must specify an absolute path.\n \n CONTROLLING THE DAEMON\n ----------------------\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 3403f48..7cfcd37 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -262,6 +262,9 @@ int main(int argc, const char **argv)\n \tif (!socket_path)\n \t\tusage_with_options(usage, options);\n \n+\tif (!is_absolute_path(socket_path))\n+\t\tdie(\"socket directory must be an absolute path\");\n+\n \tinit_socket_directory(socket_path);\n \tregister_tempfile(&socket_file, socket_path);\n \n-- \n2.7.2.645.g4e1306c\n"},{"id":"278983","messageId":"20160223071604.GC8395@sigill.intra.peff.net","threadId":"41480","inReplyTo":"20160223071427.GA7489@sigill.intra.peff.net","subject":"[PATCH 3/3] credential-cache--daemon: change to the socket dir on startup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-23T07:16:04Z","receivedAt":"2016-02-23T07:16:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jon Griffiths \n\nChanging to the socket path stops the daemon holding open\nthe directory the user was in when it was started,\npreventing umount from working. We're already holding open a\nsocket in that directory, so there's no downside.\n\nThanks-to: Jeff King \nSigned-off-by: Jon Griffiths \nSigned-off-by: Jeff King \n---\n credential-cache--daemon.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 7cfcd37..776f315 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -236,6 +236,14 @@ static void init_socket_directory(const char *path)\n \t\tif (mkdir(dir, 0700) < 0)\n \t\t\tdie_errno(\"unable to mkdir '%s'\", dir);\n \t}\n+\n+\t/*\n+\t * We don't actually care what our cwd is; we chdir here just to\n+\t * be a friendly daemon and avoid tying up our original cwd.\n+\t * If this fails, it's OK to just continue without that benefit.\n+\t */\n+\tchdir(dir);\n+\n \tfree(path_copy);\n }\n \n-- \n2.7.2.645.g4e1306c\n"},{"id":"279072","messageId":"xmqqfuwjyw59.fsf@gitster.mtv.corp.google.com","threadId":"41480","inReplyTo":"20160223071604.GC8395@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-23T21:06:10Z","receivedAt":"2016-02-23T21:06:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +\t/*\n> +\t * We don't actually care what our cwd is; we chdir here just to\n> +\t * be a friendly daemon and avoid tying up our original cwd.\n> +\t * If this fails, it's OK to just continue without that benefit.\n> +\t */\n> +\tchdir(dir);\n\nI fully do agree with this comment, but my copy of gcc and system\nheaders doesn't, unfortunately.\n\ncredential-cache--daemon.c: In function 'init_socket_directory':\ncredential-cache--daemon.c:245:7: error: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Werror=unused-result]\n chdir(dir);\n ^\ncc1: all warnings being treated as errors\nmake: *** [credential-cache--daemon.o] Error 1\n"},{"id":"279073","messageId":"20160223210725.GA25854@sigill.intra.peff.net","threadId":"41480","inReplyTo":"xmqqfuwjyw59.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-23T21:07:26Z","receivedAt":"2016-02-23T21:07:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 23, 2016 at 01:06:10PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +\t/*\n> > +\t * We don't actually care what our cwd is; we chdir here just to\n> > +\t * be a friendly daemon and avoid tying up our original cwd.\n> > +\t * If this fails, it's OK to just continue without that benefit.\n> > +\t */\n> > +\tchdir(dir);\n> \n> I fully do agree with this comment, but my copy of gcc and system\n> headers doesn't, unfortunately.\n> \n> credential-cache--daemon.c: In function 'init_socket_directory':\n> credential-cache--daemon.c:245:7: error: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Werror=unused-result]\n> chdir(dir);\n> ^\n> cc1: all warnings being treated as errors\n> make: *** [credential-cache--daemon.o] Error 1\n\nUgh. Is:\n\n (void)chdir(dir);\n\nenough? Or do we have to do:\n\n if (chdir(dir))\n\t; /* nothing */\n\n?\n\n-Peff\n"},{"id":"279074","messageId":"xmqqbn77yvzb.fsf@gitster.mtv.corp.google.com","threadId":"41480","inReplyTo":"20160223210725.GA25854@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-23T21:09:44Z","receivedAt":"2016-02-23T21:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 23, 2016 at 01:06:10PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > +\t/*\n>> > +\t * We don't actually care what our cwd is; we chdir here just to\n>> > +\t * be a friendly daemon and avoid tying up our original cwd.\n>> > +\t * If this fails, it's OK to just continue without that benefit.\n>> > +\t */\n>> > +\tchdir(dir);\n>> \n>> I fully do agree with this comment, but my copy of gcc and system\n>> headers doesn't, unfortunately.\n>> \n>> credential-cache--daemon.c: In function 'init_socket_directory':\n>> credential-cache--daemon.c:245:7: error: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Werror=unused-result]\n>> chdir(dir);\n>> ^\n>> cc1: all warnings being treated as errors\n>> make: *** [credential-cache--daemon.o] Error 1\n>\n> Ugh. Is:\n>\n> (void)chdir(dir);\n>\n> enough? Or do we have to do:\n>\n> if (chdir(dir))\n> \t; /* nothing */\n>\n> ?\n>\n> -Peff\n\nI tentatively squashed this, which I think reads better.\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 9a3a7a3..6b00ee0 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -237,12 +237,13 @@ static void init_socket_directory(const char *path)\n \t\t\tdie_errno(\"unable to mkdir '%s'\", dir);\n \t}\n \n-\t/*\n-\t * We don't actually care what our cwd is; we chdir here just to\n-\t * be a friendly daemon and avoid tying up our original cwd.\n-\t * If this fails, it's OK to just continue without that benefit.\n-\t */\n-\tchdir(dir);\n+\tif (chdir(dir))\n+\t\t/*\n+\t\t * We don't actually care what our cwd is; we chdir here just to\n+\t\t * be a friendly daemon and avoid tying up our original cwd.\n+\t\t * If this fails, it's OK to just continue without that benefit.\n+\t\t */\n+\t\t;\n \n \tfree(path_copy);\n }\n"},{"id":"279075","messageId":"20160223211046.GA25905@sigill.intra.peff.net","threadId":"41480","inReplyTo":"xmqqbn77yvzb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-23T21:10:46Z","receivedAt":"2016-02-23T21:10:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 23, 2016 at 01:09:44PM -0800, Junio C Hamano wrote:\n\n> I tentatively squashed this, which I think reads better.\n> \n> diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\n> index 9a3a7a3..6b00ee0 100644\n> --- a/credential-cache--daemon.c\n> +++ b/credential-cache--daemon.c\n> @@ -237,12 +237,13 @@ static void init_socket_directory(const char *path)\n> \t\t\tdie_errno(\"unable to mkdir '%s'\", dir);\n> \t}\n> \n> -\t/*\n> -\t * We don't actually care what our cwd is; we chdir here just to\n> -\t * be a friendly daemon and avoid tying up our original cwd.\n> -\t * If this fails, it's OK to just continue without that benefit.\n> -\t */\n> -\tchdir(dir);\n> +\tif (chdir(dir))\n> +\t\t/*\n> +\t\t * We don't actually care what our cwd is; we chdir here just to\n> +\t\t * be a friendly daemon and avoid tying up our original cwd.\n> +\t\t * If this fails, it's OK to just continue without that benefit.\n> +\t\t */\n> +\t\t;\n> \n> \tfree(path_copy);\n> }\n\nThat looks great, thanks.\n\n-Peff\n"}]}