On Tue, Jan 10, 2012 at 11:44:21AM -0600, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > 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@xxxxxxxx>
>
> A part of me worries that this assumption (3), and the additional
> assumption that nothing running concurrently cares about the cwd,
> might come back to bite us when the future (2) arrives. But I don't
> see a better approach.
The problem is that when future (2) arrives, a credential-cache specific
hack won't be helping it at all. :)
To be honest, I don't really expect a lot of future unix-socket users.
It's not portable, and most of our IPC happens over pipes. The design of
the cache daemon is very specific in requiring a true
many-clients-to-one-server model, but also caring deeply about access
controls (making TCP sockets less good[1]).
[1] One could in theory do a loopback TCP socket and provide a random
token read from an access-controlled file. But that ends up a lot
more complicated and introduces new attack surfaces. Which is a bad
thing for security-sensitive code like this.
> Follow-on ideas: on platforms that support it, it would be nice to use
>
> ctx->orig_dirfd = open(".", O_RDONLY);
> if (ctx->orig_dirfd < 0)
> ... error handling ...
> ...
> if (ctx->orig_dirfd >= 0) {
> if (fchdir(ctx->orig_dirfd))
> die_errno("unable to restore original working directory");
> close(ctx->orig_dirfd);
> }
Yeah, I started by using fchdir, but noticed that we don't use it
anywhere else and didn't want to create a portability problem. It does
fix the "somebody deleted your cwd while you were gone from it" problem.
But if you have no cwd at all, the open call will still fail.
Still, it would be slightly more robust. I wonder how portable fchdir
is in practice (I guess we could always fall back to the getcwd code
path). Do you want to prepare a patch on top?
> [...]
> > + dir = path;
> > + dir_len = slash - path;
> [...]
> > + if (chdir_len(dir, dir_len) < 0)
> > + return -1;
>
> Could avoid some complication by eliminating the dir_len var.
>
> if (chdir_len(dir, slash - dir))
> return -1;
Ah, yeah. Originally I had written it so that "slash" didn't survive
untouched to there, but in the current code, that would work.
Junio, if you haven't merged it to next yet, it might be worth squashing
in the patch below. Otherwise I wouldn't worry about it.
> Neither seems worth holding up the patch, so
> Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Thanks for the review (and for, as usual, a well-written bug report).
---
diff --git a/unix-socket.c b/unix-socket.c
index 91ed9dc..e8f19c6 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -43,7 +43,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
if (size > sizeof(sa->sun_path)) {
const char *slash = find_last_dir_sep(path);
const char *dir;
- int dir_len;
if (!slash) {
errno = ENAMETOOLONG;
@@ -51,8 +50,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
}
dir = path;
- dir_len = slash - path;
-
path = slash + 1;
size = strlen(path) + 1;
if (size > sizeof(sa->sun_path)) {
@@ -64,7 +61,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
errno = ENAMETOOLONG;
return -1;
}
- if (chdir_len(dir, dir_len) < 0)
+ if (chdir_len(dir, slash - dir) < 0)
return -1;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html