Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()

From: Jeff King <peff@peff.net>
Date: Jul 21, 2014, 02:25 UTC
Message-ID: <20140721022501.GB22750@peff.net>
In-Reply-To: <53CB7729.40101@web.de>

On Sun, Jul 20, 2014 at 10:00:41AM +0200, René Scharfe wrote:

Show 9 quoted lines

> -- >8 --
> Subject: [PATCH] unix-socket: remove stale socket before calling chdir()
> 
> unix_stream_listen() is given a path.  It calls unix_sockaddr_init(),
> which in turn can call chdir().  After that a relative path doesn't
> mean the same as before.  Any use of the original path should thus
> happen before that call.  For that reason, unlink the given path
> (to get rid of a possibly existing stale socket) right at the
> beginning of the function.

Thanks, I think this ordering problem was just missed in 1eb10f4 (unix-socket: handle long socket pathnames, 2012-01-09).

Your solution looks OK, though I think also just using:

  unlink(sa.sun_path);

would work, too (that is the path we are feeding to bind(), whether we have chdir'd or not, so perhaps it is a little more obviously correct?). I'm OK with either.

Show 15 quoted lines

> diff --git a/unix-socket.c b/unix-socket.c
> index 01f119f..91bd6b8 100644
> --- a/unix-socket.c
> +++ b/unix-socket.c
> @@ -99,11 +99,12 @@ int unix_stream_listen(const char *path)
>  	struct sockaddr_un sa;
>  	struct unix_sockaddr_context ctx;
>  
> +	unlink(path);
> +
>  	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
>  		return -1;
>  	fd = unix_stream_socket();
>  
> -	unlink(path);

I briefly wondered if this should be unlinking only when we get EEXIST, but I don't think it is worth caring about. The only caller is credential-cache, and it always wants to unconditionally replace whatever is there (it will already have tried to contact any existing socket).

-Peff