From 1eb10f4091931d6b89ff10edad63ce9c01ed17fd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Jan 2012 23:44:30 -0500 Subject: 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 Signed-off-by: Junio C Hamano --- unix-socket.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 5 deletions(-) diff --git a/unix-socket.c b/unix-socket.c index 84b15099f..7d8bec615 100644 --- a/unix-socket.c +++ b/unix-socket.c @@ -9,27 +9,83 @@ 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; + + if (!slash) { + errno = ENAMETOOLONG; + return -1; + } + + dir = 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, slash - dir) < 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 +93,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; } -- cgit v1.2.1 From 8ec6c8d79567a71ca3c6f1ec73eb453d371b1ade Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Jan 2012 23:57:33 -0500 Subject: credential-cache: report more daemon connection errors Originally, this code remained relatively silent when we failed to connect to the cache. The idea was that it was simply a cache, and we didn't want to bother the user with temporary failures (the worst case is that we would simply ask their password again). However, if you have a configuration failure or other problem, it is helpful for the daemon to report those problems. Git will happily ignore the failed error code, but the extra information to stderr can help the user diagnose the problem. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- credential-cache.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/credential-cache.c b/credential-cache.c index b15a9a744..193301877 100644 --- a/credential-cache.c +++ b/credential-cache.c @@ -71,10 +71,14 @@ static void do_cache(const char *socket, const char *action, int timeout, die_errno("unable to relay credential"); } - if (send_request(socket, &buf) < 0 && (flags & FLAG_SPAWN)) { - spawn_daemon(socket); - if (send_request(socket, &buf) < 0) + if (send_request(socket, &buf) < 0) { + if (errno != ENOENT) die_errno("unable to connect to cache daemon"); + if (flags & FLAG_SPAWN) { + spawn_daemon(socket); + if (send_request(socket, &buf) < 0) + die_errno("unable to connect to cache daemon"); + } } strbuf_release(&buf); } -- cgit v1.2.1 From 06121a0a8328c8aaa7a023cf6ebb142e9dc2b45c Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 11 Jan 2012 17:50:10 -0600 Subject: unix-socket: do not let close() or chdir() clobber errno during cleanup unix_stream_connect and unix_stream_listen return -1 on error, with errno set by the failing underlying call to allow the caller to write a useful diagnosis. Unfortunately the error path involves a few system calls itself, such as close(), that can themselves touch errno. This is not as worrisome as it might sound. If close() fails, this just means substituting one meaningful error message for another, which is perfectly fine. However, when the call _succeeds_, it is allowed to (and sometimes might) clobber errno along the way with some undefined value, so it is good higiene to save errno and restore it immediately before returning to the caller. Do so. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- unix-socket.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/unix-socket.c b/unix-socket.c index 7d8bec615..01f119f97 100644 --- a/unix-socket.c +++ b/unix-socket.c @@ -73,25 +73,29 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path, int unix_stream_connect(const char *path) { - int fd; + int fd, saved_errno; struct sockaddr_un sa; struct unix_sockaddr_context ctx; 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; - } + if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) + goto fail; unix_sockaddr_cleanup(&ctx); return fd; + +fail: + saved_errno = errno; + unix_sockaddr_cleanup(&ctx); + close(fd); + errno = saved_errno; + return -1; } int unix_stream_listen(const char *path) { - int fd; + int fd, saved_errno; struct sockaddr_un sa; struct unix_sockaddr_context ctx; @@ -100,18 +104,19 @@ int unix_stream_listen(const char *path) fd = unix_stream_socket(); unlink(path); - if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) { - unix_sockaddr_cleanup(&ctx); - close(fd); - return -1; - } + if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) + goto fail; - if (listen(fd, 5) < 0) { - unix_sockaddr_cleanup(&ctx); - close(fd); - return -1; - } + if (listen(fd, 5) < 0) + goto fail; unix_sockaddr_cleanup(&ctx); return fd; + +fail: + saved_errno = errno; + unix_sockaddr_cleanup(&ctx); + close(fd); + errno = saved_errno; + return -1; } -- cgit v1.2.1 From 35a71f1402b40b580d985a9d7e5fb1c9ec4d0232 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 17 Jan 2012 01:02:32 -0500 Subject: credential-cache: ignore "connection refused" errors The credential-cache helper will try to connect to its daemon over a unix socket. Originally, a failure to do so was silently ignored, and we would either give up (if performing a "get" or "erase" operation), or spawn a new daemon (for a "store" operation). But since 8ec6c8d, we try to report more errors. We detect a missing daemon by checking for ENOENT on our connection attempt. If the daemon is missing, we continue as before (giving up or spawning a new daemon). For any other error, we die and report the problem. However, checking for ENOENT is not sufficient for a missing daemon. We might also get ECONNREFUSED if a dead daemon process left a stale socket. This generally shouldn't happen, as the daemon cleans up after itself, but the daemon may not always be given a chance to do so (e.g., power loss, "kill -9"). The resulting state is annoying not just because the helper outputs an extra useless message, but because it actually blocks the helper from spawning a new daemon to replace the stale socket. Fix it by checking for ECONNREFUSED. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- credential-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/credential-cache.c b/credential-cache.c index 193301877..9a03792c7 100644 --- a/credential-cache.c +++ b/credential-cache.c @@ -72,7 +72,7 @@ static void do_cache(const char *socket, const char *action, int timeout, } if (send_request(socket, &buf) < 0) { - if (errno != ENOENT) + if (errno != ENOENT && errno != ECONNREFUSED) die_errno("unable to connect to cache daemon"); if (flags & FLAG_SPAWN) { spawn_daemon(socket); -- cgit v1.2.1