| 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
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= >
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)