[PATCH 0/3] credential-cache: give daemon a predictable cwd

[PATCH 0/3] credential-cache: give daemon a predictable cwd

[Jeff King]

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

[PATCH 1/3] credential-cache--daemon: refactor check_socket_directory

[Jeff King]

From: Jon Griffiths <jon_p_griffiths@yahoo.com>

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 <jon_p_griffiths@yahoo.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 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

[PATCH 2/3] credential-cache--daemon: disallow relative socket path

[Jeff King]

From: Jon Griffiths <jon_p_griffiths@yahoo.com>

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 <peff@peff.net>
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 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

[PATCH 3/3] credential-cache--daemon: change to the socket dir on startup

[Jeff King]

From: Jon Griffiths <jon_p_griffiths@yahoo.com>

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 <peff@peff.net>
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 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

Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup

[Junio C Hamano]

Jeff King <peff@peff.net> 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

Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup

[Jeff King]

On Tue, Feb 23, 2016 at 01:06:10PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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

Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 23, 2016 at 01:06:10PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> 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);
 }

Re: [PATCH 3/3] credential-cache--daemon: change to the socket dir on startup

[Jeff King]

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