Patchwork [6,of,7] chg: use relative path at connect

login
register
mail settings
Submitter Jun Wu
Date April 4, 2016, 2:30 a.m.
Message ID <f7c6cfae4dba1c48a63b.1459737053@x1c>
Download mbox | patch
Permalink /patch/14323/
State Changes Requested
Headers show

Comments

Jun Wu - April 4, 2016, 2:30 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1459736917 -3600
#      Mon Apr 04 03:28:37 2016 +0100
# Node ID f7c6cfae4dba1c48a63bdea1b9aa816e7f634d02
# Parent  f5414d2b4f32431a251029ebf46f48efdd9f013c
chg: use relative path at connect

We have made chgserver use relative path at bind(), now do the same change to
the client at connect().
Simon Farnsworth - April 4, 2016, 11:02 a.m.
On 04/04/2016 03:30, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459736917 -3600
> #      Mon Apr 04 03:28:37 2016 +0100
> # Node ID f7c6cfae4dba1c48a63bdea1b9aa816e7f634d02
> # Parent  f5414d2b4f32431a251029ebf46f48efdd9f013c
> chg: use relative path at connect
>
> We have made chgserver use relative path at bind(), now do the same change to
> the client at connect().
>
> diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
> --- a/contrib/chg/hgclient.c
> +++ b/contrib/chg/hgclient.c
> @@ -424,12 +424,36 @@
>   	if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC) < 0)
>   		abortmsg("cannot set flags of socket (errno = %d)", errno);
>
> +	/* use relative (shorter) path at connect() if possible. it is helpful
> +	 * because sizeof(addr.sun_path) can be very small (usually 108). */
> +	int bakwdfd = -1;
> +	const char *lastsep = strrchr(sockname, '/');
> +	if (lastsep) {
> +		bakwdfd = open(".", O_DIRECTORY);
> +		size_t len = lastsep - sockname;
> +		char dir[len + 1];
> +		memcpy(dir, sockname, len);
> +		if (chdir(dir) != 0)
> +			abortmsg("cannot chdir (errno = %d)", errno);

This is problematic if the socket is in a directory that the user 
doesn't have search permission on; you cannot chdir(2) to a directory 
without the x bit set, but you can connect to a socket if you're given 
the full path, even if the containing directory does not have the x bit set.

I'd also suggest printing strerror_r(3) output, not just a raw errno; I 
think the problem Pierre-Yves is seeing is that dir is not guaranteed to 
be null-terminated (nothing sets the terminator), and on his platform, 
the garbage after the copied socket name is not 0.

> +		sockname = lastsep + 1;
> +	}
> +
>   	struct sockaddr_un addr;
>   	addr.sun_family = AF_UNIX;
> +	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
>   	strncpy(addr.sun_path, sockname, sizeof(addr.sun_path));
> -	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
> +	if (addr.sun_path[sizeof(addr.sun_path) - 1] != '\0')
> +		abortmsg("sockname %s is too long", sockname);
>
>   	int r = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
> +
> +	/* restore workdir right after connect() */
> +	if (bakwdfd != -1) {
> +		if (fchdir(bakwdfd) != 0)
> +			abortmsg("cannot chdir back (errno = %d)", errno);
> +		close(bakwdfd);
> +	}
> +
>   	if (r < 0) {
>   		close(fd);
>   		if (errno == ENOENT || errno == ECONNREFUSED)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=XYNfvuIDYcnYIeJjtdXN_gLKBWVh-PuW-S8sP1eYb_c&s=EEM63v8KB8qQlXlGLrTy4wXA-YevTxwyxFM69WPMrM8&e=
>
Jun Wu - April 4, 2016, 12:34 p.m.
On 04/04/2016 12:02 PM, Simon Farnsworth wrote:
> This is problematic if the socket is in a directory that the user doesn't
> have search permission on;

> I'd also suggest printing strerror_r(3) output, not just a raw errno;

This makes sense if errno = 2 is not ENOENT, or the system is mangling errno
using a Linux security module.

If it runs Linux (it is according to the wiki), then errno 2 should be ENOENT.
That said, making another abortmsg to use perror() is a good idea so it works
with other operating systems.

> I think the problem Pierre-Yves is seeing is that dir is not guaranteed to
> be null-terminated (nothing sets the terminator), and on his platform, the
> garbage after the copied socket name is not 0.

My mistake. I forgot to set dir[len] to '\0'. Shouldn't write code while sleepy
though...

Patch

diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
--- a/contrib/chg/hgclient.c
+++ b/contrib/chg/hgclient.c
@@ -424,12 +424,36 @@ 
 	if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC) < 0)
 		abortmsg("cannot set flags of socket (errno = %d)", errno);
 
+	/* use relative (shorter) path at connect() if possible. it is helpful
+	 * because sizeof(addr.sun_path) can be very small (usually 108). */
+	int bakwdfd = -1;
+	const char *lastsep = strrchr(sockname, '/');
+	if (lastsep) {
+		bakwdfd = open(".", O_DIRECTORY);
+		size_t len = lastsep - sockname;
+		char dir[len + 1];
+		memcpy(dir, sockname, len);
+		if (chdir(dir) != 0)
+			abortmsg("cannot chdir (errno = %d)", errno);
+		sockname = lastsep + 1;
+	}
+
 	struct sockaddr_un addr;
 	addr.sun_family = AF_UNIX;
+	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
 	strncpy(addr.sun_path, sockname, sizeof(addr.sun_path));
-	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
+	if (addr.sun_path[sizeof(addr.sun_path) - 1] != '\0')
+		abortmsg("sockname %s is too long", sockname);
 
 	int r = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
+
+	/* restore workdir right after connect() */
+	if (bakwdfd != -1) {
+		if (fchdir(bakwdfd) != 0)
+			abortmsg("cannot chdir back (errno = %d)", errno);
+		close(bakwdfd);
+	}
+
 	if (r < 0) {
 		close(fd);
 		if (errno == ENOENT || errno == ECONNREFUSED)