On Wed, Mar 03, 2021 at 02:53:23PM -0800, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx> > > > > Calls to `chdir()` are dangerous in a multi-threaded context. If > > `unix_stream_listen()` or `unix_stream_connect()` is given a socket > > pathname that is too long 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. > > > > Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when this > > flag is set. > > While it is clear that this will not affect any existing callers, I > am not sure if this is a good direction to go in the longer term. > > I have to wonder if somebody actually relies on this "feature", > though. As long as ENAMETOOLONG is passed back to the caller so > that it can react to it, any caller that knows it is safe to chdir() > at the point of calling "send_request()" should be able to chdir() > itself and come back (or fork a child that chdirs and opens a unix > domain socket there, and then send the file descriptor back to the > parent process). The feature is definitely useful; I think I did 1eb10f4091 (unix-socket: handle long socket pathnames, 2012-01-09) in response to a real problem. Certainly callers could handle the error themselves. The reason I pushed it down into the socket code was to avoid having to implement in multiple callers. There are only two, but we'd have needed it in both sides (credential-cache--daemon as the listener, and credential-cache as the client). Ironically, the listening side now does a permanent chdir() to the socket directory anyway, since 6e61449051 (credential-cache--daemon: change to the socket dir on startup, 2016-02-23). So we could just do that first, and then feed the basename to the socket code. The client side would still need to handle it, though. It could probably also chdir to the socket directory without any real downside (once started, I don't think the helper program needs to access the filesystem at all outside of the socket). So I dunno. I'd be OK to just rip the feature out in favor of doing those chdir()s. But that seems like a non-zero amount of work versus leaving, and the existing code has the benefit that if another caller shows up, it could benefit from the feature. -Peff
