# [PATCH 0/3] credential-cache: give daemon a predictable cwd 8 messages from 2016-02-23 to 2016-02-23. Participants: Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/41480 ## Jeff King, 2016-02-23 07:14 Subject: [PATCH 0/3] credential-cache: give daemon a predictable cwd Message-ID: <20160223071427.GA7489@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160223071427.GA7489%40sigill.intra.peff.net ``` The credential-cache--daemon may live forever in whatever directory it happened to be spawned in (in practice, probably the root of some git repo's work tree). This can prevent umount from unmounting a filesystem that contains that directory, even though credential-cache doesn't care one way or the other. Jon Griffiths sent a patch to me and to the list, but it seems that vger blocks all mail from yahoo, including him. Eep. So he and I had some off-list back-and-forth, and ended up at this series, which I agreed to pass along. I've made a few minor tweaks from the last version he sent me. Hopefully nothing he disagrees with, but of course he has no means to complain to the list if so. :) [1/3]: credential-cache--daemon: refactor check_socket_directory [2/3]: credential-cache--daemon: disallow relative socket path [3/3]: credential-cache--daemon: change to the socket dir on startup -Peff ``` ## Jeff King, 2016-02-23 07:15 Subject: [PATCH 1/3] credential-cache--daemon: refactor check_socket_directory Message-ID: <20160223071515.GA8395@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160223071515.GA8395%40sigill.intra.peff.net In-Reply-To: <20160223071427.GA7489@sigill.intra.peff.net> ``` From: Jon Griffiths This function does an early return, and therefore has to repeat its cleanup. We can stick the later bit of the function into an "else" and avoid duplicating the shared part (which will get bigger in a future patch). Let's also rename the function to init_socket_directory. It not only checks the directory but also creates it. Saying "init" is more accurate. Signed-off-by: Jon Griffiths Signed-off-by: Jeff King --- credential-cache--daemon.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index cc65a9c..3403f48 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -215,7 +215,7 @@ static const char permissions_advice[] = "users may be able to read your cached credentials. Consider running:\n" "\n" " chmod 0700 %s"; -static void check_socket_directory(const char *path) +static void init_socket_directory(const char *path) { struct stat st; char *path_copy = xstrdup(path); @@ -224,20 +224,18 @@ static void check_socket_directory(const char *path) if (!stat(dir, &st)) { if (st.st_mode & 077) die(permissions_advice, dir); - free(path_copy); - return; + } else { + /* + * We must be sure to create the directory with the correct mode, + * not just chmod it after the fact; otherwise, there is a race + * condition in which somebody can chdir to it, sleep, then try to open + * our protected socket. + */ + if (safe_create_leading_directories_const(dir) < 0) + die_errno("unable to create directories for '%s'", dir); + if (mkdir(dir, 0700) < 0) + die_errno("unable to mkdir '%s'", dir); } - - /* - * We must be sure to create the directory with the correct mode, - * not just chmod it after the fact; otherwise, there is a race - * condition in which somebody can chdir to it, sleep, then try to open - * our protected socket. - */ - if (safe_create_leading_directories_const(dir) < 0) - die_errno("unable to create directories for '%s'", dir); - if (mkdir(dir, 0700) < 0) - die_errno("unable to mkdir '%s'", dir); free(path_copy); } @@ -264,7 +262,7 @@ int main(int argc, const char **argv) if (!socket_path) usage_with_options(usage, options); - check_socket_directory(socket_path); + init_socket_directory(socket_path); register_tempfile(&socket_file, socket_path); if (ignore_sighup) -- 2.7.2.645.g4e1306c ``` ## Jeff King, 2016-02-23 07:15 Subject: [PATCH 2/3] credential-cache--daemon: disallow relative socket path Message-ID: <20160223071541.GB8395@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160223071541.GB8395%40sigill.intra.peff.net In-Reply-To: <20160223071427.GA7489@sigill.intra.peff.net> ``` From: Jon Griffiths Relative socket paths are dangerous since the user cannot generally control when the daemon starts (initially, after a timeout, kill or crash). Since the daemon creates but does not delete the socket directory, this could lead to spurious directory creation relative to the users cwd. Suggested-by: Jeff King Signed-off-by: Jon Griffiths Signed-off-by: Jeff King --- Documentation/git-credential-cache.txt | 2 +- credential-cache--daemon.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt index 89b7306..96208f8 100644 --- a/Documentation/git-credential-cache.txt +++ b/Documentation/git-credential-cache.txt @@ -36,7 +36,7 @@ OPTIONS cache daemon if one is not started). Defaults to `~/.git-credential-cache/socket`. If your home directory is on a network-mounted filesystem, you may need to change this to a - local filesystem. + local filesystem. You must specify an absolute path. CONTROLLING THE DAEMON ---------------------- diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index 3403f48..7cfcd37 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -262,6 +262,9 @@ int main(int argc, const char **argv) if (!socket_path) usage_with_options(usage, options); + if (!is_absolute_path(socket_path)) + die("socket directory must be an absolute path"); + init_socket_directory(socket_path); register_tempfile(&socket_file, socket_path); -- 2.7.2.645.g4e1306c ``` ## Jeff King, 2016-02-23 07:16 Subject: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup Message-ID: <20160223071604.GC8395@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160223071604.GC8395%40sigill.intra.peff.net In-Reply-To: <20160223071427.GA7489@sigill.intra.peff.net> ``` From: Jon Griffiths Changing to the socket path stops the daemon holding open the directory the user was in when it was started, preventing umount from working. We're already holding open a socket in that directory, so there's no downside. Thanks-to: Jeff King Signed-off-by: Jon Griffiths Signed-off-by: Jeff King --- credential-cache--daemon.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index 7cfcd37..776f315 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -236,6 +236,14 @@ static void init_socket_directory(const char *path) if (mkdir(dir, 0700) < 0) die_errno("unable to mkdir '%s'", dir); } + + /* + * We don't actually care what our cwd is; we chdir here just to + * be a friendly daemon and avoid tying up our original cwd. + * If this fails, it's OK to just continue without that benefit. + */ + chdir(dir); + free(path_copy); } -- 2.7.2.645.g4e1306c ``` ## Junio C Hamano, 2016-02-23 21:06 Subject: Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup Message-ID: URL: https://gitlist.dev/e/xmqqfuwjyw59.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160223071604.GC8395@sigill.intra.peff.net> ``` Jeff King writes: > + /* > + * We don't actually care what our cwd is; we chdir here just to > + * be a friendly daemon and avoid tying up our original cwd. > + * If this fails, it's OK to just continue without that benefit. > + */ > + chdir(dir); I fully do agree with this comment, but my copy of gcc and system headers doesn't, unfortunately. credential-cache--daemon.c: In function 'init_socket_directory': credential-cache--daemon.c:245:7: error: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Werror=unused-result] chdir(dir); ^ cc1: all warnings being treated as errors make: *** [credential-cache--daemon.o] Error 1 ``` ## Jeff King, 2016-02-23 21:07 Subject: Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup Message-ID: <20160223210725.GA25854@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160223210725.GA25854%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Feb 23, 2016 at 01:06:10PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > + /* > > + * We don't actually care what our cwd is; we chdir here just to > > + * be a friendly daemon and avoid tying up our original cwd. > > + * If this fails, it's OK to just continue without that benefit. > > + */ > > + chdir(dir); > > I fully do agree with this comment, but my copy of gcc and system > headers doesn't, unfortunately. > > credential-cache--daemon.c: In function 'init_socket_directory': > credential-cache--daemon.c:245:7: error: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Werror=unused-result] > chdir(dir); > ^ > cc1: all warnings being treated as errors > make: *** [credential-cache--daemon.o] Error 1 Ugh. Is: (void)chdir(dir); enough? Or do we have to do: if (chdir(dir)) ; /* nothing */ ? -Peff ``` ## Junio C Hamano, 2016-02-23 21:09 Subject: Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup Message-ID: URL: https://gitlist.dev/e/xmqqbn77yvzb.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160223210725.GA25854@sigill.intra.peff.net> ``` Jeff King writes: > On Tue, Feb 23, 2016 at 01:06:10PM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > + /* >> > + * We don't actually care what our cwd is; we chdir here just to >> > + * be a friendly daemon and avoid tying up our original cwd. >> > + * If this fails, it's OK to just continue without that benefit. >> > + */ >> > + chdir(dir); >> >> I fully do agree with this comment, but my copy of gcc and system >> headers doesn't, unfortunately. >> >> credential-cache--daemon.c: In function 'init_socket_directory': >> credential-cache--daemon.c:245:7: error: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Werror=unused-result] >> chdir(dir); >> ^ >> cc1: all warnings being treated as errors >> make: *** [credential-cache--daemon.o] Error 1 > > Ugh. Is: > > (void)chdir(dir); > > enough? Or do we have to do: > > if (chdir(dir)) > ; /* nothing */ > > ? > > -Peff I tentatively squashed this, which I think reads better. diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index 9a3a7a3..6b00ee0 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -237,12 +237,13 @@ static void init_socket_directory(const char *path) die_errno("unable to mkdir '%s'", dir); } - /* - * We don't actually care what our cwd is; we chdir here just to - * be a friendly daemon and avoid tying up our original cwd. - * If this fails, it's OK to just continue without that benefit. - */ - chdir(dir); + if (chdir(dir)) + /* + * We don't actually care what our cwd is; we chdir here just to + * be a friendly daemon and avoid tying up our original cwd. + * If this fails, it's OK to just continue without that benefit. + */ + ; free(path_copy); } ``` ## Jeff King, 2016-02-23 21:10 Subject: Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup Message-ID: <20160223211046.GA25905@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160223211046.GA25905%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Feb 23, 2016 at 01:09:44PM -0800, Junio C Hamano wrote: > I tentatively squashed this, which I think reads better. > > diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c > index 9a3a7a3..6b00ee0 100644 > --- a/credential-cache--daemon.c > +++ b/credential-cache--daemon.c > @@ -237,12 +237,13 @@ static void init_socket_directory(const char *path) > die_errno("unable to mkdir '%s'", dir); > } > > - /* > - * We don't actually care what our cwd is; we chdir here just to > - * be a friendly daemon and avoid tying up our original cwd. > - * If this fails, it's OK to just continue without that benefit. > - */ > - chdir(dir); > + if (chdir(dir)) > + /* > + * We don't actually care what our cwd is; we chdir here just to > + * be a friendly daemon and avoid tying up our original cwd. > + * If this fails, it's OK to just continue without that benefit. > + */ > + ; > > free(path_copy); > } That looks great, thanks. -Peff ```