{"thread":{"id":"45721","subject":"[PATCH v2] xgethostname: handle long hostnames","startedAt":"2017-04-17T16:18:06Z","lastAt":"2017-04-19T03:49:17Z","messageCount":9,"participants":["David Turner","Junio C Hamano","René Scharfe","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"317014","messageId":"20170417161748.31231-1-dturner@twosigma.com","threadId":"45721","inReplyTo":null,"subject":"[PATCH v2] xgethostname: handle long hostnames","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-17T16:17:48Z","receivedAt":"2017-04-17T16:18:06Z","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\nAlways use a consistent buffer size when calling xgethostname.  We use\nHOST_NAME_MAX + 1.  When this is unavailable, we fall back to 256, a\ndecision we previously made in daemon.c, but which we now move to\ngit-compat-util.h so that it can be used everywhere.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n\nThis version addresses some comments by René Scharfe.  René, we're\nstill silently truncating, but as I noted in my message to Jonathan:\nLooking at the users of this function, I think most would be happier\nwith a truncated buffer than an error:\n\n* gc.c: used to see if we are the same machine as the machine that\nlocked the repo. it is unlikely that two machines have hostnames that\ndiffer only in the 256th-or-above character, and it would be weird to\nfail to gc just because our hostname is long.\n* fetch-pack.c, receive-pack.c: similar to gc.c; the hostname is a note\nin the .keep file for human consumption only\n* ident.c: used to make up a fake email address. On my laptop,\ngethostname returns \"corey\" (no domain part), so the email address is\nnot likely to be valid anyway.\n\n builtin/gc.c           |  6 +++---\n builtin/receive-pack.c |  4 ++--\n daemon.c               |  4 ----\n fetch-pack.c           |  4 ++--\n git-compat-util.h      |  6 ++++++\n ident.c                |  4 ++--\n wrapper.c              | 13 +++++++++++++\n 7 files changed, 28 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c2c61a57bb..5de0209c59 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -238,7 +238,7 @@ static int need_to_gc(void)\n static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n {\n \tstatic struct lock_file lock;\n-\tchar my_host[128];\n+\tchar my_host[HOST_NAME_MAX + 1];\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct stat st;\n \tuintmax_t pid;\n@@ -250,14 +250,14 @@ 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\");\n \tfd = hold_lock_file_for_update(&lock, pidfile_path,\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tif (!force) {\n-\t\tstatic char locking_host[128];\n+\t\tstatic char locking_host[HOST_NAME_MAX + 1];\n \t\tint should_exit;\n \t\tfp = fopen(pidfile_path, \"r\");\n \t\tmemset(locking_host, 0, sizeof(locking_host));\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex aca9c33d8d..0ca423a711 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1695,12 +1695,12 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (status)\n \t\t\treturn \"unpack-objects abnormal exit\";\n \t} else {\n-\t\tchar hostname[256];\n+\t\tchar hostname[HOST_NAME_MAX + 1];\n \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/daemon.c b/daemon.c\nindex 473e6b6b63..1503e1ed6f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -4,10 +4,6 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n-#ifndef HOST_NAME_MAX\n-#define HOST_NAME_MAX 256\n-#endif\n-\n #ifdef NO_INITGROUPS\n #define initgroups(x, y) (0) /* nothing */\n #endif\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex d07d85ce30..15d59a0440 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -802,8 +802,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t\tif (args->use_thin_pack)\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\tchar hostname[HOST_NAME_MAX + 1];\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..bd04564a69 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -884,6 +884,12 @@ 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+#ifndef HOST_NAME_MAX\n+#define HOST_NAME_MAX 256\n+#endif\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..bea871c8e0 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -120,9 +120,9 @@ static int canonical_name(const char *host, struct strbuf *out)\n \n static void add_domainname(struct strbuf *out, int *is_bogus)\n {\n-\tchar buf[1024];\n+\tchar buf[HOST_NAME_MAX + 1];\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":"317058","messageId":"xmqq1ssqikc5.fsf@gitster.mtv.corp.google.com","threadId":"45721","inReplyTo":"20170417161748.31231-1-dturner@twosigma.com","subject":"Re: [PATCH v2] xgethostname: handle long hostnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T01:19:06Z","receivedAt":"2017-04-18T01:19:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n> ...\n>  \tif (!force) {\n> -\t\tstatic char locking_host[128];\n> +\t\tstatic char locking_host[HOST_NAME_MAX + 1];\n>  \t\tint should_exit;\n>  \t\tfp = fopen(pidfile_path, \"r\");\n>  \t\tmemset(locking_host, 0, sizeof(locking_host));\n\nI compared the result of applying this v2 directly on top of master\nand applying René's \"Use HOST_NAME_MAX\"and then applying your v1.  \nThis hunk is the only difference.\n\nAs this locking_host is used like so in the later part of the code:\n\n \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n \t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n \t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n\nI suspect that turning it to HOST_NAME_MAX + 1 without tweaking\nthe format \"%127c\" gives us an inconsistent resulting code.\n\nOf course, my_host is sized to HOST_NAME_MAX + 1 and we are\ncomparing it with locking_host, so perhaps we'd need to take this\nversion to size locking_host to also HOST_NAME_MAX + 1, and then\nscan with %255c (but then shouldn't we scan with %256c instead?  I\nam not sure where these +1 comes from).\n"},{"id":"317059","messageId":"xmqqwpaih4q2.fsf@gitster.mtv.corp.google.com","threadId":"45721","inReplyTo":"xmqq1ssqikc5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] xgethostname: handle long hostnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T01:41:41Z","receivedAt":"2017-04-18T01:41:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> David Turner <dturner@twosigma.com> writes:\n>\n>> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>> ...\n>>  \tif (!force) {\n>> -\t\tstatic char locking_host[128];\n>> +\t\tstatic char locking_host[HOST_NAME_MAX + 1];\n>>  \t\tint should_exit;\n>>  \t\tfp = fopen(pidfile_path, \"r\");\n>>  \t\tmemset(locking_host, 0, sizeof(locking_host));\n>\n> I compared the result of applying this v2 directly on top of master\n> and applying René's \"Use HOST_NAME_MAX\"and then applying your v1.  \n> This hunk is the only difference.\n>\n> As this locking_host is used like so in the later part of the code:\n>\n>  \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n>  \t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n>  \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n>  \t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n>\n> I suspect that turning it to HOST_NAME_MAX + 1 without tweaking\n> the format \"%127c\" gives us an inconsistent resulting code.\n>\n> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are\n> comparing it with locking_host, so perhaps we'd need to take this\n> version to size locking_host to also HOST_NAME_MAX + 1, and then\n> scan with %255c (but then shouldn't we scan with %256c instead?  I\n> am not sure where these +1 comes from).\n\nThat is, something along this line...\n\n builtin/gc.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex be75508292..4f85610d87 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tif (!force) {\n \t\tstatic char locking_host[HOST_NAME_MAX + 1];\n+\t\tstatic char *scan_fmt;\n \t\tint should_exit;\n+\n+\t\tif (!scan_fmt)\n+\t\t\tscan_fmt = xstrfmt(\"%s %%%dc\", \"%\"SCNuMAX, HOST_NAME_MAX);\n \t\tfp = fopen(pidfile_path, \"r\");\n \t\tmemset(locking_host, 0, sizeof(locking_host));\n \t\tshould_exit =\n@@ -256,7 +260,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t\t * running.\n \t\t\t */\n \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n-\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n+\t\t\tfscanf(fp, scan_fmt, &pid, locking_host) == 2 &&\n \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n \t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n \t\tif (fp != NULL)\n"},{"id":"317111","messageId":"281d0843-d48a-b7ab-737b-b9528689d44e@web.de","threadId":"45721","inReplyTo":"xmqqwpaih4q2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] xgethostname: handle long hostnames","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-18T16:07:43Z","receivedAt":"2017-04-18T16:08:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.04.2017 um 03:41 schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> David Turner <dturner@twosigma.com> writes:\n>>\n>>> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>>> ...\n>>>   \tif (!force) {\n>>> -\t\tstatic char locking_host[128];\n>>> +\t\tstatic char locking_host[HOST_NAME_MAX + 1];\n>>>   \t\tint should_exit;\n>>>   \t\tfp = fopen(pidfile_path, \"r\");\n>>>   \t\tmemset(locking_host, 0, sizeof(locking_host));\n>>\n>> I compared the result of applying this v2 directly on top of master\n>> and applying René's \"Use HOST_NAME_MAX\"and then applying your v1.\n>> This hunk is the only difference.\n>>\n>> As this locking_host is used like so in the later part of the code:\n>>\n>>   \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n>>   \t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n>>   \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n>>   \t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n>>\n>> I suspect that turning it to HOST_NAME_MAX + 1 without tweaking\n>> the format \"%127c\" gives us an inconsistent resulting code.\n\nOh, missed that.  Thanks for catching it!\n\n>> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are\n>> comparing it with locking_host, so perhaps we'd need to take this\n>> version to size locking_host to also HOST_NAME_MAX + 1, and then\n>> scan with %255c (but then shouldn't we scan with %256c instead?  I\n>> am not sure where these +1 comes from).\n> \n> That is, something along this line...\n> \n>   builtin/gc.c | 6 +++++-\n>   1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index be75508292..4f85610d87 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>   \t\t\t\t       LOCK_DIE_ON_ERROR);\n>   \tif (!force) {\n>   \t\tstatic char locking_host[HOST_NAME_MAX + 1];\n> +\t\tstatic char *scan_fmt;\n>   \t\tint should_exit;\n> +\n> +\t\tif (!scan_fmt)\n> +\t\t\tscan_fmt = xstrfmt(\"%s %%%dc\", \"%\"SCNuMAX, HOST_NAME_MAX);\n>   \t\tfp = fopen(pidfile_path, \"r\");\n>   \t\tmemset(locking_host, 0, sizeof(locking_host));\n>   \t\tshould_exit =\n> @@ -256,7 +260,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>   \t\t\t * running.\n>   \t\t\t */\n>   \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n> -\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n> +\t\t\tfscanf(fp, scan_fmt, &pid, locking_host) == 2 &&\n>   \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n>   \t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n>   \t\tif (fp != NULL)\n> \n\nHow important is it to scan the whole file in one call?  We could split\nit up like this and use a strbuf to handle host names of any length.  We\nneed to be permissive here to allow machines with different values for\nHOST_NAME_MAX to work with the same file on a network file system, so\nthis would have to be the first patch, right?\n\nNB: That && cascade has enough meat for a whole function.\n\nRené\n\n---\n builtin/gc.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 1fca84c19d..d5e880028e 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -251,10 +251,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tfd = hold_lock_file_for_update(&lock, pidfile_path,\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tif (!force) {\n-\t\tstatic char locking_host[128];\n+\t\tstatic struct strbuf locking_host = STRBUF_INIT;\n \t\tint should_exit;\n \t\tfp = fopen(pidfile_path, \"r\");\n-\t\tmemset(locking_host, 0, sizeof(locking_host));\n \t\tshould_exit =\n \t\t\tfp != NULL &&\n \t\t\t!fstat(fileno(fp), &st) &&\n@@ -268,9 +267,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t\t * running.\n \t\t\t */\n \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n-\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n+\t\t\tfscanf(fp, \"%\"SCNuMAX\" \", &pid) == 1 &&\n+\t\t\t!strbuf_getwholeline(&locking_host, fp, '\\0') &&\n \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n-\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n+\t\t\t(strcmp(locking_host.buf, my_host) || !kill(pid, 0) || errno == EPERM);\n \t\tif (fp != NULL)\n \t\t\tfclose(fp);\n \t\tif (should_exit) {\n@@ -278,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t\t\trollback_lock_file(&lock);\n \t\t\t*ret_pid = pid;\n \t\t\tfree(pidfile_path);\n-\t\t\treturn locking_host;\n+\t\t\treturn locking_host.buf;\n \t\t}\n \t}\n \n-- \n2.12.2\n"},{"id":"317113","messageId":"20170418161734.pa665rqwdtbnsj7f@sigill.intra.peff.net","threadId":"45721","inReplyTo":"281d0843-d48a-b7ab-737b-b9528689d44e@web.de","subject":"Re: [PATCH v2] xgethostname: handle long hostnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T16:17:34Z","receivedAt":"2017-04-18T16:17:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2017 at 06:07:43PM +0200, René Scharfe wrote:\n\n> > -\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n> > +\t\t\tfscanf(fp, scan_fmt, &pid, locking_host) == 2 &&\n> >   \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n> >   \t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n> >   \t\tif (fp != NULL)\n> > \n> \n> How important is it to scan the whole file in one call?  We could split\n> it up like this and use a strbuf to handle host names of any length.  We\n> need to be permissive here to allow machines with different values for\n> HOST_NAME_MAX to work with the same file on a network file system, so\n> this would have to be the first patch, right?\n\nI doubt that doing it in one call matters. It's not like stdio promises\nus any atomicity in the first place.\n\n> -\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n> +\t\t\tfscanf(fp, \"%\"SCNuMAX\" \", &pid) == 1 &&\n> +\t\t\t!strbuf_getwholeline(&locking_host, fp, '\\0') &&\n\nI don't think there is anything wrong with using fscanf here, but it has\nenough pitfalls in general that I don't really like its use as a parser\n(and the general lack of it in Git's code base seems to agree).\n\nI wonder if this should just read a line (or the whole file) into a\nstrbuf and parse it there. That would better match our usual style, I\nthink.\n\nI can live with it either way.\n\n> NB: That && cascade has enough meat for a whole function.\n\nYeah.\n\n-Peff\n"},{"id":"317126","messageId":"e1f3ca5df4484496a2e0ab601a940ecb@exmbdft7.ad.twosigma.com","threadId":"45721","inReplyTo":"281d0843-d48a-b7ab-737b-b9528689d44e@web.de","subject":"RE: [PATCH v2] xgethostname: handle long hostnames","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-18T17:52:40Z","receivedAt":"2017-04-18T17:52:46Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: René Scharfe [mailto:l.s.r@web.de]\n> Sent: Tuesday, April 18, 2017 12:08 PM\n> To: Junio C Hamano <gitster@pobox.com>; David Turner\n... \n> >> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are comparing\n> >> it with locking_host, so perhaps we'd need to take this version to\n> >> size locking_host to also HOST_NAME_MAX + 1, and then scan with %255c\n> >> (but then shouldn't we scan with %256c instead?  I am not sure where\n> >> these +1 comes from).\n> >\n> > That is, something along this line...\n> >\n> >   builtin/gc.c | 6 +++++-\n> >   1 file changed, 5 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/gc.c b/builtin/gc.c index be75508292..4f85610d87\n> > 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t*\n> ret_pid)\n> >   \t\t\t\t       LOCK_DIE_ON_ERROR);\n> >   \tif (!force) {\n> >   \t\tstatic char locking_host[HOST_NAME_MAX + 1];\n> > +\t\tstatic char *scan_fmt;\n> >   \t\tint should_exit;\n> > +\n> > +\t\tif (!scan_fmt)\n> > +\t\t\tscan_fmt = xstrfmt(\"%s %%%dc\", \"%\"SCNuMAX,\n> HOST_NAME_MAX);\n> >   \t\tfp = fopen(pidfile_path, \"r\");\n> >   \t\tmemset(locking_host, 0, sizeof(locking_host));\n> >   \t\tshould_exit =\n> > @@ -256,7 +260,7 @@ static const char *lock_repo_for_gc(int force, pid_t*\n> ret_pid)\n> >   \t\t\t * running.\n> >   \t\t\t */\n> >   \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n> > -\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host)\n> == 2 &&\n> > +\t\t\tfscanf(fp, scan_fmt, &pid, locking_host) == 2 &&\n> >   \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n> >   \t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno\n> == EPERM);\n> >   \t\tif (fp != NULL)\n> >\n> \n> How important is it to scan the whole file in one call?  We could split it up like\n> this and use a strbuf to handle host names of any length.  We need to be\n> permissive here to allow machines with different values for HOST_NAME_MAX\n> to work with the same file on a network file system, so this would have to be the\n> first patch, right?\n\nIf the writer has the smaller HOST_NAME_MAX, this will work fine.  If the reader\nhas the smaller HOST_NAME_MAX, and the writer's actual value is too long,\nthen there's no way the strcmp would succeed anyway.  So I don't think we need\nto worry about it.\n\n> NB: That && cascade has enough meat for a whole function.\n\n+1\n\n"},{"id":"317172","messageId":"xmqqtw5lcgnd.fsf@gitster.mtv.corp.google.com","threadId":"45721","inReplyTo":"20170418161734.pa665rqwdtbnsj7f@sigill.intra.peff.net","subject":"Re: [PATCH v2] xgethostname: handle long hostnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T01:47:34Z","receivedAt":"2017-04-19T01:47:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I doubt that doing it in one call matters. It's not like stdio promises\n> us any atomicity in the first place.\n>\n>> -\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n>> +\t\t\tfscanf(fp, \"%\"SCNuMAX\" \", &pid) == 1 &&\n>> +\t\t\t!strbuf_getwholeline(&locking_host, fp, '\\0') &&\n>\n> I don't think there is anything wrong with using fscanf here, but it has\n> enough pitfalls in general that I don't really like its use as a parser\n> (and the general lack of it in Git's code base seems to agree).\n>\n> I wonder if this should just read a line (or the whole file) into a\n> strbuf and parse it there. That would better match our usual style, I\n> think.\n\nYeah, I think it would be a good change.\n"},{"id":"317173","messageId":"xmqqpog9cgkq.fsf@gitster.mtv.corp.google.com","threadId":"45721","inReplyTo":"281d0843-d48a-b7ab-737b-b9528689d44e@web.de","subject":"Re: [PATCH v2] xgethostname: handle long hostnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T01:49:09Z","receivedAt":"2017-04-19T01:49:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> How important is it to scan the whole file in one call?  We could split\n> it up like this and use a strbuf to handle host names of any length.  We\n> need to be permissive here to allow machines with different values for\n> HOST_NAME_MAX to work with the same file on a network file system, so\n> this would have to be the first patch, right?\n\nAbsolutely.  FWIW, I agree with Peff that we do not need to use\nfscanf here; just reading a line into strbuf and picking pieces\nwould be sufficient.\n\n> NB: That && cascade has enough meat for a whole function.\n\nTrue, too.\n\n>\n> René\n>\n> ---\n>  builtin/gc.c | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 1fca84c19d..d5e880028e 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -251,10 +251,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>  \tfd = hold_lock_file_for_update(&lock, pidfile_path,\n>  \t\t\t\t       LOCK_DIE_ON_ERROR);\n>  \tif (!force) {\n> -\t\tstatic char locking_host[128];\n> +\t\tstatic struct strbuf locking_host = STRBUF_INIT;\n>  \t\tint should_exit;\n>  \t\tfp = fopen(pidfile_path, \"r\");\n> -\t\tmemset(locking_host, 0, sizeof(locking_host));\n>  \t\tshould_exit =\n>  \t\t\tfp != NULL &&\n>  \t\t\t!fstat(fileno(fp), &st) &&\n> @@ -268,9 +267,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>  \t\t\t * running.\n>  \t\t\t */\n>  \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n> -\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n> +\t\t\tfscanf(fp, \"%\"SCNuMAX\" \", &pid) == 1 &&\n> +\t\t\t!strbuf_getwholeline(&locking_host, fp, '\\0') &&\n>  \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n> -\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n> +\t\t\t(strcmp(locking_host.buf, my_host) || !kill(pid, 0) || errno == EPERM);\n>  \t\tif (fp != NULL)\n>  \t\t\tfclose(fp);\n>  \t\tif (should_exit) {\n> @@ -278,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>  \t\t\t\trollback_lock_file(&lock);\n>  \t\t\t*ret_pid = pid;\n>  \t\t\tfree(pidfile_path);\n> -\t\t\treturn locking_host;\n> +\t\t\treturn locking_host.buf;\n>  \t\t}\n>  \t}\n"},{"id":"317185","messageId":"xmqqk26hawg9.fsf@gitster.mtv.corp.google.com","threadId":"45721","inReplyTo":"e1f3ca5df4484496a2e0ab601a940ecb@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH v2] xgethostname: handle long hostnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T03:49:10Z","receivedAt":"2017-04-19T03:49:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <David.Turner@twosigma.com> writes:\n\n> If the writer has the smaller HOST_NAME_MAX, this will work fine.  If the reader\n> has the smaller HOST_NAME_MAX, and the writer's actual value is too long,\n> then there's no way the strcmp would succeed anyway.  So I don't think we need\n> to worry about it.\n\nHmph, I have to agree with that reasoning, only because the value we\nread into locking_host[] is not used for error reporting at all.  I\nwould have insisted to read what is on the filesystem anyway if that\nwere not the case.\n\nThanks.\n"}]}