git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv3 10/13] credentials: add "cache" helper

From
Jeff King <peff@peff.net>
Date
Jan 10, 2012, 04:44 UTC
Message-ID
<20120110044430.GA23619@sigill.intra.peff.net>
In-Reply-To
<20120110015038.GA17754@burratino>
On Mon, Jan 09, 2012 at 07:50:38PM -0600, Jonathan Nieder wrote:
Show 13 quoted lines
> 	--- expect-stderr       2012-01-10 00:07:02.398365248 +0000
> 	+++ stderr      2012-01-10 00:07:02.418364996 +0000
> 	@@ -1,2 +1,3 @@
> 	+fatal: socket path is too long to fit in sockaddr
> 	 askpass: Username for 'https://example.com':
> 	 askpass: Password for 'https://askpass-username@example.com':
> 	not ok - 1 helper (cache) has no existing data
> 
> I didn't notice until recently because typically the cwd is short.
> The sun_path[] array on this machine is 108 bytes; POSIX informs me
> that some platforms make it as small as 96 bytes and there's no
> guaranteed lower limit.  The machines[*] triggering it were running
> tests in a chroot, hence the long path.

Ugh. Some days I really love working on Unix systems. And then some days you run across junk like this.

Presumably Linux has kept to 108 to avoid breaking older programs. So it's not as if we can assume it will be changed soon, or just write this off as a limitation for old crappy systems.

Googling around, I've seen some indication that you can simply over-allocate the sockaddr_un and pass a longer length to connect. However that just seems to yield EINVAL on Linux (and even if it did work on Linux, I'd be surprised if it worked everywhere).

Which really leaves us with only one option: chdir and bind to a relative path, as you did below.

Show 16 quoted lines
> -	if (!socket_path)
> -		socket_path = expand_user_path("~/.git-credential-cache/socket");
> -	if (!socket_path)
> -		die("unable to find a suitable socket path; use --socket");
> +	if (!socket_path) {
> +		char *home = expand_user_path("~");
> +		if (!home)
> +			die("unable to find a suitable socket path; use --socket");
> +
> +		if (!chdir(home))
> +			socket_path = ".git-credential-cache/socket";
> +		else if (errno == ENOENT && !strcmp(op, "exit"))
> +			return 0;
> +		else
> +			die("cannot enter home directory");
> +	}

I think this is the right approach, but the wrong place to do it. Your patch only helps people using the default path (so passing --socket=$longpath would still be broken). And we'd need a similar fix for the binding side in credential-cache--daemon.

Here's a patch which passes your $longpath test.
-- >8 --
Subject: [PATCH] unix-socket: handle long socket pathnames

On many systems, the sockaddr_un.sun_path field is quite small. Even on Linux, it is only 108 characters. A user of the credential-cache daemon can easily surpass this, especially if their home directory is in a deep directory tree (since the default location expands ~/.git-credentials).

We can hack around this in the unix-socket.[ch] code by doing a chdir() to the enclosing directory, feeding the relative basename to the socket functions, and then restoring the working directory.

This introduces several new possible error cases for creating a socket, including an irrecoverable one in the case that we can't restore the working directory. In the case of the credential-cache code, we could perhaps get away with simply chdir()-ing to the socket directory and never coming back. However, I'd rather do it at the lower level for a few reasons:

  1. It keeps the hackery behind an opaque interface instead
     of polluting the main program logic.
  2. A hack in credential-cache won't help any unix-socket
     users who come along later.
  3. The chdir trickery isn't that likely to fail (basically
     it's only a problem if your cwd is missing or goes away
     while you're running).  And because we only enable the
     hack when we get a too-long name, it can only fail in
     cases that would have failed under the previous code
     anyway.
Signed-off-by: Jeff King <peff@peff.net>
---
 unix-socket.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/unix-socket.c b/unix-socket.c
index 84b1509..664782a 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -9,27 +9,86 @@ static int unix_stream_socket(void)
 	return fd;
 }
 
-static void unix_sockaddr_init(struct sockaddr_un *sa, const char *path)
+static int chdir_len(const char *orig, int len)
+{
+	char *path = xmemdupz(orig, len);
+	int r = chdir(path);
+	free(path);
+	return r;
+}
+
+struct unix_sockaddr_context {
+	char orig_dir[PATH_MAX];
+};
+
+static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
+{
+	if (!ctx->orig_dir[0])
+		return;
+	/*
+	 * If we fail, we can't just return an error, since we have
+	 * moved the cwd of the whole process, which could confuse calling
+	 * code.  We are better off to just die.
+	 */
+	if (chdir(ctx->orig_dir) < 0)
+		die("unable to restore original working directory");
+}
+
+static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
+			      struct unix_sockaddr_context *ctx)
 {
 	int size = strlen(path) + 1;
-	if (size > sizeof(sa->sun_path))
-		die("socket path is too long to fit in sockaddr");
+
+	ctx->orig_dir[0] = '\0';
+	if (size > sizeof(sa->sun_path)) {
+		const char *slash = find_last_dir_sep(path);
+		const char *dir;
+		int dir_len;
+
+		if (!slash) {
+			errno = ENAMETOOLONG;
+			return -1;
+		}
+
+		dir = path;
+		dir_len = slash - path;
+
+		path = slash + 1;
+		size = strlen(path) + 1;
+		if (size > sizeof(sa->sun_path)) {
+			errno = ENAMETOOLONG;
+			return -1;
+		}
+
+		if (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) {
+			errno = ENAMETOOLONG;
+			return -1;
+		}
+		if (chdir_len(dir, dir_len) < 0)
+			return -1;
+	}
+
 	memset(sa, 0, sizeof(*sa));
 	sa->sun_family = AF_UNIX;
 	memcpy(sa->sun_path, path, size);
+	return 0;
 }
 
 int unix_stream_connect(const char *path)
 {
 	int fd;
 	struct sockaddr_un sa;
+	struct unix_sockaddr_context ctx;
 
-	unix_sockaddr_init(&sa, path);
+	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+		return -1;
 	fd = unix_stream_socket();
 	if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+		unix_sockaddr_cleanup(&ctx);
 		close(fd);
 		return -1;
 	}
+	unix_sockaddr_cleanup(&ctx);
 	return fd;
 }
 
@@ -37,20 +96,25 @@ int unix_stream_listen(const char *path)
 {
 	int fd;
 	struct sockaddr_un sa;
+	struct unix_sockaddr_context ctx;
 
-	unix_sockaddr_init(&sa, path);
+	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+		return -1;
 	fd = unix_stream_socket();
 
 	unlink(path);
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+		unix_sockaddr_cleanup(&ctx);
 		close(fd);
 		return -1;
 	}
 
 	if (listen(fd, 5) < 0) {
+		unix_sockaddr_cleanup(&ctx);
 		close(fd);
 		return -1;
 	}
 
+	unix_sockaddr_cleanup(&ctx);
 	return fd;
 }

-- 
1.7.9.rc0.33.g15ced5
Previous: Jonathan NiederNext: Jeff King
Message 17 of 54 in “[PATCHv3 0/13] credential helpers”
  1. Jeff KingDec 10, 2011
  2. 01/13 test-lib: add test_config_global variantJeff King, Dec 10, 2011
  3. 02/13 t5550: fix typoJeff King, Dec 10, 2011
  4. 03/13 introduce credentials APIJeff King, Dec 10, 2011
  5. Jakub NarebskiDec 10, 2011
  6. Jeff KingDec 10, 2011
  7. 04/13 credential: add function for parsing url componentsJeff King, Dec 10, 2011
  8. 05/13 http: use credential API to get passwordsJeff King, Dec 10, 2011
  9. 06/13 credential: apply helper configJeff King, Dec 10, 2011
  10. 07/13 credential: add credential.*.usernameJeff King, Dec 10, 2011
  11. 08/13 credential: make relevance of http path configurableJeff King, Dec 10, 2011
  12. Jakub NarebskiDec 10, 2011
  13. Jeff KingDec 10, 2011
  14. 09/13 docs: end-user documentation for the credential subsystemJeff King, Dec 10, 2011
  15. 10/13 credentials: add "cache" helperJeff King, Dec 10, 2011
  16. Jonathan NiederJan 10, 2012
  17. Jeff KingJan 10, 2012
  18. Jeff KingJan 10, 2012
  19. Junio C HamanoJan 10, 2012
  20. Jeff KingJan 17, 2012
  21. Junio C HamanoJan 17, 2012
  22. Jonathan NiederJan 10, 2012
  23. Jeff KingJan 10, 2012
  24. Jonathan NiederJan 11, 2012
  25. Jeff KingJan 12, 2012
  26. 11/13 strbuf: add strbuf_add*_urlencodeJeff King, Dec 10, 2011
  27. Jakub NarebskiDec 10, 2011
  28. Jeff KingDec 10, 2011
  29. 12/13 credentials: add "store" helperJeff King, Dec 10, 2011
  30. 13/13 t: add test harness for external credential helpersJeff King, Dec 10, 2011
  31. 0/9 echo characters in username promptJeff King, Dec 10, 2011
  32. 1/9 imap-send: avoid buffer overflowJeff King, Dec 10, 2011
  33. 2/9 imap-send: don't check return value of git_getpassJeff King, Dec 10, 2011
  34. 3/9 move git_getpass to its own source fileJeff King, Dec 10, 2011
  35. 4/9 refactor git_getpass into generic prompt functionJeff King, Dec 10, 2011
  36. 5/9 add generic terminal prompt functionJeff King, Dec 10, 2011
  37. Pete WyckoffDec 15, 2011
  38. Jeff KingDec 15, 2011
  39. Pete WyckoffDec 15, 2011
  40. 6/9 prompt: use git_terminal_promptJeff King, Dec 10, 2011
  41. 7/9 credential: use git_prompt instead of git_getpassJeff King, Dec 10, 2011
  42. 8/9 Makefile: linux has /dev/ttyJeff King, Dec 10, 2011
  43. 9/9 Makefile: OS X has /dev/ttyJeff King, Dec 10, 2011
  44. 1/2 Makefile: Windows lacks /dev/ttyJohannes Sixt, Dec 12, 2011
  45. 2/2 Makefile: optionally exclude code that needs Unix socketsJohannes Sixt, Dec 12, 2011
  46. Jeff KingDec 12, 2011
  47. Junio C HamanoDec 12, 2011
  48. Junio C HamanoDec 13, 2011
  49. Jeff KingDec 13, 2011
  50. Johannes SixtDec 13, 2011
  51. Junio C HamanoDec 14, 2011
  52. Jeff KingDec 12, 2011
  53. Johannes SixtDec 12, 2011
  54. 1/1 contrib: add credential helper for OS X KeychainJeff King, Dec 10, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.