threads / patch / 41480

patch, 3 partscredential-cache: give daemon a predictable cwd

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

## tl;dr

8 messages between Feb 23, 2016 and Feb 23, 2016. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Jeff King· Feb 23, 2016, 07:14 UTC · lore

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· Feb 23, 2016, 07:15 UTC · re: Jeff King · lore

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

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(-)
Show changes to credential-cache--daemon.c +13 −15
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· Feb 23, 2016, 07:15 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +4 −1

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

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· Feb 23, 2016, 07:16 UTC · re: Jeff King · lore

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

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(+)
Show changes to credential-cache--daemon.c +8 −0
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· Feb 23, 2016, 21:06 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> +	/*
> +	 * 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· Feb 23, 2016, 21:07 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 23, 2016 at 01:06:10PM -0800, Junio C Hamano wrote:
Show 18 quoted lines
> 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
Junio C Hamano· Feb 23, 2016, 21:09 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 33 quoted lines
> 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.
Show changes to credential-cache--daemon.c +7 −6
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· Feb 23, 2016, 21:10 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 23, 2016 at 01:09:44PM -0800, Junio C Hamano wrote:
Show 26 quoted lines
> 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

← back to recent threads