From 1eb10f4091931d6b89ff10edad63ce9c01ed17fd Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Mon, 9 Jan 2012 23:44:30 -0500
Subject: [PATCH 1/4] 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
unix-socket.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 66 insertions(+), 5 deletions(-)
diff --git a/unix-socket.c b/unix-socket.c
index 84b15099f2f..7d8bec61587 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;
}
From 8ec6c8d79567a71ca3c6f1ec73eb453d371b1ade Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Mon, 9 Jan 2012 23:57:33 -0500
Subject: [PATCH 2/4] 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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
credential-cache.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/credential-cache.c b/credential-cache.c
index b15a9a74494..193301877f1 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);
}
From 06121a0a8328c8aaa7a023cf6ebb142e9dc2b45c Mon Sep 17 00:00:00 2001
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 11 Jan 2012 17:50:10 -0600
Subject: [PATCH 3/4] 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 <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
unix-socket.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/unix-socket.c b/unix-socket.c
index 7d8bec61587..01f119f9700 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;
}
From 35a71f1402b40b580d985a9d7e5fb1c9ec4d0232 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Tue, 17 Jan 2012 01:02:32 -0500
Subject: [PATCH 4/4] 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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
credential-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/credential-cache.c b/credential-cache.c
index 193301877f1..9a03792c7de 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);