| Message ID | 745b6d5fb74699b7fe7e32080b18779aa4a82547.1612208747.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Simple IPC Mechanism | expand |
On Mon, Feb 01, 2021 at 07:45:45PM +0000, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@microsoft.com> > > Calls to `chdir()` are dangerous in a multi-threaded context. If > `unix_stream_listen()` is given a socket pathname that is too big to > fit in a `sockaddr_un` structure, it will `chdir()` to the parent > directory of the requested socket pathname, create the socket using a > relative pathname, and then `chdir()` back. This is not thread-safe. > > Add `disallow_chdir` flag to `struct unix_sockaddr_context` and change > all callers to pass an initialized context structure. > > Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag > is set. Makes sense, and it fits nicely into the options pattern you set up in the earlier patch. > struct unix_sockaddr_context { > char *orig_dir; > + unsigned int disallow_chdir:1; > }; > > +#define UNIX_SOCKADDR_CONTEXT_INIT \ > +{ \ > + .orig_dir=NULL, \ > + .disallow_chdir=0, \ > +} It is really just zero-initializing, so "{ 0 }" would be OK (I think we are relaxed about allowing 0 as NULL in initializers). But I don't mind it being written out (but do mind whitespace around the "="). However, the point of unix_sockaddr_init() is that it's supposed to initialize the struct. And I don't think we need to carry disallow_chdir around; the cleanup function knows from orig_dir whether it's supposed to do any cleanup, so only the init function has to care. So would: diff --git a/unix-socket.c b/unix-socket.c index 19ed48be99..0eb14faf54 100644 --- a/unix-socket.c +++ b/unix-socket.c @@ -36,16 +36,23 @@ static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx) } static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path, - struct unix_sockaddr_context *ctx) + struct unix_sockaddr_context *ctx, + int disallow_chdir) { int size = strlen(path) + 1; ctx->orig_dir = NULL; if (size > sizeof(sa->sun_path)) { - const char *slash = find_last_dir_sep(path); + const char *slash; const char *dir; struct strbuf cwd = STRBUF_INIT; + if (disallow_chdir) { + errno = ENAMETOOLONG; + return -1; + } + + slash = find_last_dir_sep(path); if (!slash) { errno = ENAMETOOLONG; return -1; make it more obvious? There are only two callers, and this is all file-local, so I don't mind adding the extra parameter there. And you would not need an initializer at all. > #define UNIX_STREAM_LISTEN_OPTS_INIT \ > { \ > .listen_backlog_size = 5, \ > .force_unlink_before_bind = 1, \ > + .disallow_chdir = 0, \ > } I don't know if we care, but some options are positive "do this unlink" and some are negative "do not do this chdir". Those could be made consistent (and flip the initializer value to keep the same defaults). There is actually value in making struct defaults generally "0" unless we have reason not to, because callers sometimes zero-initialize without thinking about it. I doubt that would happen for this particular struct, and I'm deep into bike-shedding anyway, so I'm OK either way. But something like: struct unix_stream_listen_opts_init { int listen_backlog_size; int disallow_unlink; int disallow_chdir; }; would work with just a "{ 0 }" zero-initializer. :) -Peff
diff --git a/unix-socket.c b/unix-socket.c index 8bcef18ea55..9726992f276 100644 --- a/unix-socket.c +++ b/unix-socket.c @@ -11,8 +11,15 @@ static int chdir_len(const char *orig, int len) struct unix_sockaddr_context { char *orig_dir; + unsigned int disallow_chdir:1; }; +#define UNIX_SOCKADDR_CONTEXT_INIT \ +{ \ + .orig_dir=NULL, \ + .disallow_chdir=0, \ +} + static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx) { if (!ctx->orig_dir) @@ -32,7 +39,11 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path, { int size = strlen(path) + 1; - ctx->orig_dir = NULL; + if (ctx->disallow_chdir && size > sizeof(sa->sun_path)) { + errno = ENAMETOOLONG; + return -1; + } + if (size > sizeof(sa->sun_path)) { const char *slash = find_last_dir_sep(path); const char *dir; @@ -67,7 +78,7 @@ int unix_stream_connect(const char *path) { int fd, saved_errno; struct sockaddr_un sa; - struct unix_sockaddr_context ctx; + struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT; if (unix_sockaddr_init(&sa, path, &ctx) < 0) return -1; @@ -96,7 +107,9 @@ int unix_stream_listen(const char *path, int bind_successful = 0; int backlog; struct sockaddr_un sa; - struct unix_sockaddr_context ctx; + struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT; + + ctx.disallow_chdir = opts->disallow_chdir; if (unix_sockaddr_init(&sa, path, &ctx) < 0) return -1; diff --git a/unix-socket.h b/unix-socket.h index c28372ef48e..5b0e8ccef10 100644 --- a/unix-socket.h +++ b/unix-socket.h @@ -4,12 +4,14 @@ struct unix_stream_listen_opts { int listen_backlog_size; unsigned int force_unlink_before_bind:1; + unsigned int disallow_chdir:1; }; #define UNIX_STREAM_LISTEN_OPTS_INIT \ { \ .listen_backlog_size = 5, \ .force_unlink_before_bind = 1, \ + .disallow_chdir = 0, \ } int unix_stream_connect(const char *path);