Subject: Re: [PATCH v4 09/12] unix-socket: disallow chdir() when creating unix domain sockets

From: Jeff King <[email-masked]>
Date: Thu, 4 Mar 2021 09:56:14 -0500

On Wed, Mar 03, 2021 at 02:53:23PM -0800, Junio C Hamano wrote:

> "Jeff Hostetler via GitGitGadget" <[email-masked]> writes:
> 
> > From: Jeff Hostetler <[email-masked]>
> >
> > 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