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