Patchwork [1,of,3] chg: let hgc_open support long path

login
register
mail settings
Submitter Jun Wu
Date Dec. 23, 2016, 4:46 p.m.
Message ID <68566eb02a680d9a67ff.1482511608@x1c>
Download mbox | patch
Permalink /patch/18019/
State Accepted
Headers show

Comments

Jun Wu - Dec. 23, 2016, 4:46 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1482511020 0
#      Fri Dec 23 16:37:00 2016 +0000
# Node ID 68566eb02a680d9a67fff8eb9ae3eab77872f886
# Parent  b0a0f7b9ed9007a3f1323f9417efad715489e918
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 68566eb02a68
chg: let hgc_open support long path

"sizeof(sun_path)" is too small. Use the chdir trick to support long socket
path, like "mercurial.util.bindunixsocket".

It's useful for cases where TMPDIR is long. Modern OS X rewrites TMPDIR to a
long value. And we probably want to use XDG_RUNTIME_DIR [2] for Linux.

The approach is a bit different from the previous plan, where we will have
hgc_openat and pass cmdserveropts.sockdirfd to it. That's because the
current change is easier: chg has to pass a full path to "hg" as the
"--address" parameter. There is no "--address-basename" or "--address-dirfd"
flags. The next patch will remove "sockdirfd".

Note: It'd be nice if we can use a native "connectat" implementation.
However, that's not available everywhere. Some platform (namely FreeBSD)
does support it, but the implementation has bugs so it cannot be used [2].

[1]: https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
[2]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-April/082892.html
Jun Wu - Dec. 23, 2016, 4:51 p.m.
Excerpts from Jun Wu's message of 2016-12-23 16:46:48 +0000:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1482511020 0
> #      Fri Dec 23 16:37:00 2016 +0000
> # Node ID 68566eb02a680d9a67fff8eb9ae3eab77872f886
> # Parent  b0a0f7b9ed9007a3f1323f9417efad715489e918
> # Available At https://bitbucket.org/quark-zju/hg-draft 
> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 68566eb02a68
> chg: let hgc_open support long path
> 
> "sizeof(sun_path)" is too small. Use the chdir trick to support long socket
> path, like "mercurial.util.bindunixsocket".
> 
> It's useful for cases where TMPDIR is long. Modern OS X rewrites TMPDIR to a
> long value. And we probably want to use XDG_RUNTIME_DIR [2] for Linux.
> 
> The approach is a bit different from the previous plan, where we will have
> hgc_openat and pass cmdserveropts.sockdirfd to it. That's because the
> current change is easier: chg has to pass a full path to "hg" as the
> "--address" parameter. There is no "--address-basename" or "--address-dirfd"
> flags. The next patch will remove "sockdirfd".
> 
> Note: It'd be nice if we can use a native "connectat" implementation.
> However, that's not available everywhere. Some platform (namely FreeBSD)
> does support it, but the implementation has bugs so it cannot be used [2].
> 
> [1]: https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html 
> [2]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-April/082892.html 
> 
> diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
> --- a/contrib/chg/hgclient.c
> +++ b/contrib/chg/hgclient.c
> @@ -426,8 +426,41 @@ hgclient_t *hgc_open(const char *socknam
>      struct sockaddr_un addr;
>      addr.sun_family = AF_UNIX;
> -    strncpy(addr.sun_path, sockname, sizeof(addr.sun_path));
> +
> +    /* use chdir to workaround small sizeof(sun_path) */
> +    int bakfd = -1;
> +    const char *basename = sockname;
> +    {
> +        const char *split = strrchr(sockname, '/');
> +        if (split) {

I just realized that this line should be changed to:

           if (split && split != sockname)

to support socket paths like "/foo".

> +            if (split[1] == '\0')
> +                abortmsg("sockname cannot end with a slash");
> +            size_t len = split - sockname;
> +            char sockdir[len + 1];
> +            memcpy(sockdir, sockname, len);
> +            sockdir[len] = '\0';
> +
> +            bakfd = open(".", O_DIRECTORY);
> +            if (bakfd == -1)
> +                abortmsgerrno("cannot open cwd");
> +
> +            int r = chdir(sockdir);
> +            if (r != 0)
> +                abortmsgerrno("cannot chdir %s", sockdir);
> +
> +            basename = split + 1;
> +        }
> +    }
> +    if (strlen(basename) >= sizeof(addr.sun_path))
> +        abortmsg("sockname is too long: %s", basename);
> +    strncpy(addr.sun_path, basename, sizeof(addr.sun_path));
>      addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
>  
> +    /* real connect */
>      int r = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
> +    if (bakfd != -1) {
> +        fchdirx(bakfd);
> +        close(bakfd);
> +    }
> +
>      if (r < 0) {
>          close(fd);
Yuya Nishihara - Dec. 25, 2016, 10:29 a.m.
On Fri, 23 Dec 2016 16:51:15 +0000, Jun Wu wrote:
> Excerpts from Jun Wu's message of 2016-12-23 16:46:48 +0000:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1482511020 0
> > #      Fri Dec 23 16:37:00 2016 +0000
> > # Node ID 68566eb02a680d9a67fff8eb9ae3eab77872f886
> > # Parent  b0a0f7b9ed9007a3f1323f9417efad715489e918
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 68566eb02a68
> > chg: let hgc_open support long path
> > 
> > "sizeof(sun_path)" is too small. Use the chdir trick to support long socket
> > path, like "mercurial.util.bindunixsocket".
> > 
> > It's useful for cases where TMPDIR is long. Modern OS X rewrites TMPDIR to a
> > long value. And we probably want to use XDG_RUNTIME_DIR [2] for Linux.
> > 
> > The approach is a bit different from the previous plan, where we will have
> > hgc_openat and pass cmdserveropts.sockdirfd to it. That's because the
> > current change is easier: chg has to pass a full path to "hg" as the
> > "--address" parameter. There is no "--address-basename" or "--address-dirfd"
> > flags. The next patch will remove "sockdirfd".
> > 
> > Note: It'd be nice if we can use a native "connectat" implementation.
> > However, that's not available everywhere. Some platform (namely FreeBSD)
> > does support it, but the implementation has bugs so it cannot be used [2].
> > 
> > [1]: https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html 
> > [2]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-April/082892.html 
> > 
> > diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
> > --- a/contrib/chg/hgclient.c
> > +++ b/contrib/chg/hgclient.c
> > @@ -426,8 +426,41 @@ hgclient_t *hgc_open(const char *socknam
> >      struct sockaddr_un addr;
> >      addr.sun_family = AF_UNIX;
> > -    strncpy(addr.sun_path, sockname, sizeof(addr.sun_path));
> > +
> > +    /* use chdir to workaround small sizeof(sun_path) */
> > +    int bakfd = -1;
> > +    const char *basename = sockname;
> > +    {
> > +        const char *split = strrchr(sockname, '/');
> > +        if (split) {
> 
> I just realized that this line should be changed to:
> 
>            if (split && split != sockname)
> 
> to support socket paths like "/foo".

Fixed in flight and queued the series, thanks.

> > +    /* real connect */
> >      int r = connect(fd, (struct sockaddr *)&addr, sizeof(addr));

Perhaps we should back up (errno == ENOENT || errno == ECONNREFUSED) here
before doing another syscall, but that isn't the issue introduced by this patch.
(and the original code doesn't care a close() failure.)

> > +    if (bakfd != -1) {
> > +        fchdirx(bakfd);
> > +        close(bakfd);
> > +    }
> > +
> >      if (r < 0) {
> >          close(fd);
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
--- a/contrib/chg/hgclient.c
+++ b/contrib/chg/hgclient.c
@@ -426,8 +426,41 @@  hgclient_t *hgc_open(const char *socknam
 	struct sockaddr_un addr;
 	addr.sun_family = AF_UNIX;
-	strncpy(addr.sun_path, sockname, sizeof(addr.sun_path));
+
+	/* use chdir to workaround small sizeof(sun_path) */
+	int bakfd = -1;
+	const char *basename = sockname;
+	{
+		const char *split = strrchr(sockname, '/');
+		if (split) {
+			if (split[1] == '\0')
+				abortmsg("sockname cannot end with a slash");
+			size_t len = split - sockname;
+			char sockdir[len + 1];
+			memcpy(sockdir, sockname, len);
+			sockdir[len] = '\0';
+
+			bakfd = open(".", O_DIRECTORY);
+			if (bakfd == -1)
+				abortmsgerrno("cannot open cwd");
+
+			int r = chdir(sockdir);
+			if (r != 0)
+				abortmsgerrno("cannot chdir %s", sockdir);
+
+			basename = split + 1;
+		}
+	}
+	if (strlen(basename) >= sizeof(addr.sun_path))
+		abortmsg("sockname is too long: %s", basename);
+	strncpy(addr.sun_path, basename, sizeof(addr.sun_path));
 	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
 
+	/* real connect */
 	int r = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
+	if (bakfd != -1) {
+		fchdirx(bakfd);
+		close(bakfd);
+	}
+
 	if (r < 0) {
 		close(fd);