{"thread":{"id":"45685","subject":"[PATCH] xgethostname: handle long hostnames","startedAt":"2017-04-13T19:23:49Z","lastAt":"2017-04-13T22:32:22Z","messageCount":5,"participants":["David Turner","Jeff King","Jonathan Nieder","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316773","messageId":"20170413192335.20679-1-dturner@twosigma.com","threadId":"45685","inReplyTo":null,"subject":"[PATCH] xgethostname: handle long hostnames","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-13T19:23:35Z","receivedAt":"2017-04-13T19:23:49Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"If the full hostname doesn't fit in the buffer supplied to\ngethostname, POSIX does not specify whether the buffer will be\nnull-terminated, so to be safe, we should do it ourselves. Introduce\nnew function, xgethostname, which ensures that there is always a \\0\nat the end of the buffer.\n\nSigned-off-by: David Turner \n---\n builtin/gc.c | 2 +-\n builtin/receive-pack.c | 2 +-\n fetch-pack.c | 2 +-\n git-compat-util.h | 2 ++\n ident.c | 2 +-\n wrapper.c | 13 +++++++++++++\n 6 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c2c61a57bb..5633483f56 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -250,7 +250,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t/* already locked */\n \t\treturn NULL;\n \n-\tif (gethostname(my_host, sizeof(my_host)))\n+\tif (xgethostname(my_host, sizeof(my_host)))\n \t\txsnprintf(my_host, sizeof(my_host), \"unknown\");\n \n \tpidfile_path = git_pathdup(\"gc.pid\");\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex aca9c33d8d..fb62a94bc3 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1700,7 +1700,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\targv_array_pushl(&child.args, \"index-pack\",\n \t\t\t\t \"--stdin\", hdr_arg, NULL);\n \n-\t\tif (gethostname(hostname, sizeof(hostname)))\n+\t\tif (xgethostname(hostname, sizeof(hostname)))\n \t\t\txsnprintf(hostname, sizeof(hostname), \"localhost\");\n \t\targv_array_pushf(&child.args,\n \t\t\t\t \"--keep=receive-pack %\"PRIuMAX\" on %s\",\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex d07d85ce30..a899441c34 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -803,7 +803,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\targv_array_push(&cmd.args, \"--fix-thin\");\n \t\tif (args->lock_pack || unpack_limit) {\n \t\t\tchar hostname[256];\n-\t\t\tif (gethostname(hostname, sizeof(hostname)))\n+\t\t\tif (xgethostname(hostname, sizeof(hostname)))\n \t\t\t\txsnprintf(hostname, sizeof(hostname), \"localhost\");\n \t\t\targv_array_pushf(&cmd.args,\n \t\t\t\t\t\"--keep=fetch-pack %\"PRIuMAX \" on %s\",\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8a4a3f85e7..e49b65c235 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -884,6 +884,8 @@ static inline size_t xsize_t(off_t len)\n __attribute__((format (printf, 3, 4)))\n extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);\n \n+extern int xgethostname(char *buf, size_t len);\n+\n /* in ctype.c, for kwset users */\n extern const unsigned char tolower_trans_tbl[256];\n \ndiff --git a/ident.c b/ident.c\nindex c0364fe3a1..7de9f47c41 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -122,7 +122,7 @@ static void add_domainname(struct strbuf *out, int *is_bogus)\n {\n \tchar buf[1024];\n \n-\tif (gethostname(buf, sizeof(buf))) {\n+\tif (xgethostname(buf, sizeof(buf))) {\n \t\twarning_errno(\"cannot get host name\");\n \t\tstrbuf_addstr(out, \"(none)\");\n \t\t*is_bogus = 1;\ndiff --git a/wrapper.c b/wrapper.c\nindex 0542fc7582..d837417709 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -655,3 +655,16 @@ void sleep_millisec(int millisec)\n {\n \tpoll(NULL, 0, millisec);\n }\n+\n+int xgethostname(char *buf, size_t len)\n+{\n+\t/*\n+\t * If the full hostname doesn't fit in buf, POSIX does not\n+\t * specify whether the buffer will be null-terminated, so to\n+\t * be safe, do it ourselves.\n+\t */\n+\tint ret = gethostname(buf, len);\n+\tif (!ret)\n+\t\tbuf[len - 1] = 0;\n+\treturn ret;\n+}\n-- \n2.11.GIT\n\n"},{"id":"316787","messageId":"20170413212341.cxsd3jzemuu6ec7h@sigill.intra.peff.net","threadId":"45685","inReplyTo":"20170413192335.20679-1-dturner@twosigma.com","subject":"Re: [PATCH] xgethostname: handle long hostnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-13T21:23:41Z","receivedAt":"2017-04-13T21:23:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 03:23:35PM -0400, David Turner wrote:\n\n> If the full hostname doesn't fit in the buffer supplied to\n> gethostname, POSIX does not specify whether the buffer will be\n> null-terminated\n\nWow, TIL. What an utterly terrible and error-prone interface (I always\njust assumed we'd get ENAMETOOLONG, which is what glibc does).\n\n> so to be safe, we should do it ourselves. Introduce\n> new function, xgethostname, which ensures that there is always a \\0\n> at the end of the buffer.\n\nYour patch looks good to me.\n\n-Peff\n"},{"id":"316792","messageId":"20170413220524.GE10084@aiede.mtv.corp.google.com","threadId":"45685","inReplyTo":"20170413192335.20679-1-dturner@twosigma.com","subject":"Re: [PATCH] xgethostname: handle long hostnames","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-13T22:05:24Z","receivedAt":"2017-04-13T22:05:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDavid Turner wrote:\n\n> If the full hostname doesn't fit in the buffer supplied to\n> gethostname, POSIX does not specify whether the buffer will be\n> null-terminated, so to be safe, we should do it ourselves.\n[...]\n> +++ b/wrapper.c\n> @@ -655,3 +655,16 @@ void sleep_millisec(int millisec)\n> {\n> \tpoll(NULL, 0, millisec);\n> }\n> +\n> +int xgethostname(char *buf, size_t len)\n> +{\n> +\t/*\n> +\t * If the full hostname doesn't fit in buf, POSIX does not\n> +\t * specify whether the buffer will be null-terminated, so to\n> +\t * be safe, do it ourselves.\n> +\t */\n> +\tint ret = gethostname(buf, len);\n> +\tif (!ret)\n> +\t\tbuf[len - 1] = 0;\n> +\treturn ret;\n\nI wonder if after null-terminating we would want to report this as\nan error, instead of silently using a truncated result. I.e. something\nlike\n\n> +\tif (!ret)\n> +\t\tbuf[len - 1] = 0;\n> +\tif (strlen(buf) >= len - 1) {\n> +\t\terrno = ENAMETOOLONG;\n> +\t\treturn -1;\n> +\t}\n\n(or EINVAL --- either is equally descriptive).\n\nAlso POSIX requires that hostnames are <= 255 bytes. Maybe we can\nforce the buffer to be large enough.\n\nThoughts?\nJonathan\n"},{"id":"316794","messageId":"b53099edc57947329f826f05de5303af@exmbdft7.ad.twosigma.com","threadId":"45685","inReplyTo":"20170413220524.GE10084@aiede.mtv.corp.google.com","subject":"RE: [PATCH] xgethostname: handle long hostnames","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-13T22:29:50Z","receivedAt":"2017-04-13T22:29:57Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jonathan Nieder [mailto:jrnieder@gmail.com]\n> Sent: Thursday, April 13, 2017 6:05 PM\n> To: David Turner \n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH] xgethostname: handle long hostnames\n> \n> Hi,\n> \n> David Turner wrote:\n> \n> > If the full hostname doesn't fit in the buffer supplied to\n> > gethostname, POSIX does not specify whether the buffer will be\n> > null-terminated, so to be safe, we should do it ourselves.\n> [...]\n> > +++ b/wrapper.c\n> > @@ -655,3 +655,16 @@ void sleep_millisec(int millisec) {\n> > \tpoll(NULL, 0, millisec);\n> > }\n> > +\n> > +int xgethostname(char *buf, size_t len) {\n> > +\t/*\n> > +\t * If the full hostname doesn't fit in buf, POSIX does not\n> > +\t * specify whether the buffer will be null-terminated, so to\n> > +\t * be safe, do it ourselves.\n> > +\t */\n> > +\tint ret = gethostname(buf, len);\n> > +\tif (!ret)\n> > +\t\tbuf[len - 1] = 0;\n> > +\treturn ret;\n> \n> I wonder if after null-terminating we would want to report this as an error,\n> instead of silently using a truncated result. I.e. something like\n> \n> > +\tif (!ret)\n> > +\t\tbuf[len - 1] = 0;\n> > +\tif (strlen(buf) >= len - 1) {\n> > +\t\terrno = ENAMETOOLONG;\n> > +\t\treturn -1;\n> > +\t}\n>\n> (or EINVAL --- either is equally descriptive).\n\nLooking at the users of this function, I think most would be happier with a truncated buffer than an error:\ngc.c: used to see if we are the same machine as the machine that locked the repo. Unlikely that two machines have hostnames that differ only in the 256th-or-above character.\nfetch-pack.c, receive-pack.c: similar to gc.c; the hostname is a note in the .keep file\nIdent.c: used to make up a fake email address. On my laptop, gethostname returns \"corey\" (no domain part), so the email address is not likely to be valid anyway.\n\n> Also POSIX requires that hostnames are <= 255 bytes. Maybe we can force the\n> buffer to be large enough.\n\nThat is now how I read it. I read the limit as HOST_NAME_MAX, which has a *minimum* value of 255, but which might be larger.\n\nThe existing hostname buffers are 128, 256, and 1024 bytes, so they're pretty arbitrary. \n\n"},{"id":"316795","messageId":"aa10d2cb-c21a-fbfd-9fae-0d0153e4901b@web.de","threadId":"45685","inReplyTo":"20170413192335.20679-1-dturner@twosigma.com","subject":"Re: [PATCH] xgethostname: handle long hostnames","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-13T22:32:03Z","receivedAt":"2017-04-13T22:32:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.04.2017 um 21:23 schrieb David Turner:\n> If the full hostname doesn't fit in the buffer supplied to\n> gethostname, POSIX does not specify whether the buffer will be\n> null-terminated, so to be safe, we should do it ourselves. Introduce\n> new function, xgethostname, which ensures that there is always a \\0\n> at the end of the buffer.\n> \n> Signed-off-by: David Turner \n> ---\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 0542fc7582..d837417709 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -655,3 +655,16 @@ void sleep_millisec(int millisec)\n> {\n> \tpoll(NULL, 0, millisec);\n> }\n> +\n> +int xgethostname(char *buf, size_t len)\n> +{\n> +\t/*\n> +\t * If the full hostname doesn't fit in buf, POSIX does not\n> +\t * specify whether the buffer will be null-terminated, so to\n> +\t * be safe, do it ourselves.\n> +\t */\n> +\tint ret = gethostname(buf, len);\n> +\tif (!ret)\n> +\t\tbuf[len - 1] = 0;\n> +\treturn ret;\n> +}\n\nSilent truncation is not ideal, no matter if it's done by the wrapper or\nthe original function. It would be better to use a properly sized\nbuffer.\n\nPOSIX requires hostnames to have a maximum length of HOST_NAME_MAX. So\nhow about just adding an assert to make sure len is big enough? Or\nevaluate the condition at compile time with BUILD_ASSERT_OR_ZERO?\n\nDownside: Not all platforms define HOST_NAME_MAX. daemon.c uses 256 as\na fallback. On Windows a buffer size of 256 is documented to suffice\nin all cases [1]. The Linux manpage [2] mentions a hostname length\nlimit of 255 (plus NUL) as well, even though HOST_NAME_MAX is 64 there.\n\nAnother possibility: Die (or at least warn) if the buffer doesn't\ncontain a NUL byte after calling gethostname(). That only works for\nplatforms that don't NUL-terminate on truncation, though, so silent\ntruncation would still go unnoticed.\n\nAnyway, the buffer in builtin/gc.c with its 128 bytes seems to be too\nshort; the others are at least 256 bytes long. Replacing the magic\nbuffer size number with HOST_NAME_MAX + 1 might be a good idea (after\nmoving the fallback definition to git-compat-util.h).\n\nRené\n\n\n[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms738527(v=vs.85).aspx\n[2] http://man7.org/linux/man-pages/man2/gethostname.2.html\n"}]}