{"thread":{"id":"45737","subject":"[PATCH v3 0/2] gethostbyname fixes","startedAt":"2017-04-18T21:58:02Z","lastAt":"2017-04-21T04:18:41Z","messageCount":18,"participants":["David Turner","Jonathan Nieder","Junio C Hamano","René Scharfe","Torsten Bögershausen"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"317143","messageId":"20170418215743.18406-1-dturner@twosigma.com","threadId":"45737","inReplyTo":null,"subject":"[PATCH v3 0/2] gethostbyname fixes","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-18T21:57:41Z","receivedAt":"2017-04-18T21:58:02Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"This version includes Junio's fixup to René's patch, and then my patch\nrebased on top of René's. I thought it was easier to just send both\nin one series, than to have Junio do a bunch of conflict resolution.\nI think this still needs Junio's signoff on the first patch, since\nI've added his code.\n\nDavid Turner (1):\n xgethostname: handle long hostnames\n\nRené Scharfe (1):\n use HOST_NAME_MAX to size buffers for gethostname(2)\n\n builtin/gc.c | 12 ++++++++----\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, 33 insertions(+), 14 deletions(-)\n\n-- \n2.11.GIT\n\n"},{"id":"317144","messageId":"20170418215743.18406-3-dturner@twosigma.com","threadId":"45737","inReplyTo":"20170418215743.18406-1-dturner@twosigma.com","subject":"[PATCH v3 2/2] xgethostname: handle long hostnames","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-18T21:57:43Z","receivedAt":"2017-04-18T21:58:04Z","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 4c4a36e2b5..33a1edabbc 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 2612efad3d..0ca423a711 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 055f568775..15d59a0440 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[HOST_NAME_MAX + 1];\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 46f3abe401..bd04564a69 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -888,6 +888,8 @@ extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);\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 556851cf94..bea871c8e0 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[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":"317145","messageId":"20170418215743.18406-2-dturner@twosigma.com","threadId":"45737","inReplyTo":"20170418215743.18406-1-dturner@twosigma.com","subject":"[PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-18T21:57:42Z","receivedAt":"2017-04-18T21:58:06Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: René Scharfe \n\nPOSIX limits the length of host names to HOST_NAME_MAX. Export the\nfallback definition from daemon.c and use this constant to make all\nbuffers used with gethostname(2) big enough for any possible result\nand a terminating NUL.\n\nInspired-by: David Turner \nSigned-off-by: Rene Scharfe \nSigned-off-by: David Turner \n---\n builtin/gc.c | 10 +++++++---\n builtin/receive-pack.c | 2 +-\n daemon.c | 4 ----\n fetch-pack.c | 2 +-\n git-compat-util.h | 4 ++++\n ident.c | 2 +-\n 6 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c2c61a57bb..4c4a36e2b5 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@@ -257,8 +257,12 @@ 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 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@@ -274,7 +278,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)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex aca9c33d8d..2612efad3d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1695,7 +1695,7 @@ 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);\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..055f568775 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -802,7 +802,7 @@ 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\tchar hostname[HOST_NAME_MAX + 1];\n \t\t\tif (gethostname(hostname, sizeof(hostname)))\n \t\t\t\txsnprintf(hostname, sizeof(hostname), \"localhost\");\n \t\t\targv_array_pushf(&cmd.args,\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8a4a3f85e7..46f3abe401 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -884,6 +884,10 @@ 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 /* 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..556851cf94 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -120,7 +120,7 @@ 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 \t\twarning_errno(\"cannot get host name\");\n-- \n2.11.GIT\n\n"},{"id":"317166","messageId":"20170419012824.GA28740@aiede.svl.corp.google.com","threadId":"45737","inReplyTo":"20170418215743.18406-2-dturner@twosigma.com","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-19T01:28:24Z","receivedAt":"2017-04-19T01:28:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDavid Turner wrote:\n\n> From: René Scharfe \n>\n> POSIX limits the length of host names to HOST_NAME_MAX. Export the\n> fallback definition from daemon.c and use this constant to make all\n> buffers used with gethostname(2) big enough for any possible result\n> and a terminating NUL.\n\nSince some platforms do not define HOST_NAME_MAX and we provide a\nfallback, this is not actually big enough for any possible result.\nFor example, the Hurd allows arbitrarily long hostnames.\n\nNevertheless this patch seems like the right thing to do.\n\n> Inspired-by: David Turner \n> Signed-off-by: Rene Scharfe \n> Signed-off-by: David Turner \n> ---\n> builtin/gc.c | 10 +++++++---\n> builtin/receive-pack.c | 2 +-\n> daemon.c | 4 ----\n> fetch-pack.c | 2 +-\n> git-compat-util.h | 4 ++++\n> ident.c | 2 +-\n> 6 files changed, 14 insertions(+), 10 deletions(-)\n\nThanks for picking this up.\n\n[...]\n> +++ b/builtin/gc.c\n[...]\n> @@ -257,8 +257,12 @@ 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 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> @@ -274,7 +278,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\nI hoped this could be simplified since HOST_NAME_MAX is a numeric literal,\nusing the double-expansion trick:\n\n#define STR_(s) # s\n#define STR(s) STR_(s)\n\n\t\t\tfscanf(fp, \"%\" SCNuMAX \" %\" STR(HOST_NAME_MAX) \"c\",\n\t\t\t &pid, locking_host);\n\nUnfortunately, I don't think there's anything stopping a platform from\ndefining\n\n\t#define HOST_NAME_MAX 0x100\n\nwhich would break that.\n\nSo this run-time calculation appears to be necessary.\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"317168","messageId":"20170419013552.GB28740@aiede.svl.corp.google.com","threadId":"45737","inReplyTo":"20170418215743.18406-3-dturner@twosigma.com","subject":"Re: [PATCH v3 2/2] xgethostname: handle long hostnames","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-19T01:35:52Z","receivedAt":"2017-04-19T01:36:00Z","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. Introduce\n> new function, xgethostname, which ensures that there is always a \\0\n> at the end of the buffer.\n\nI think we should detect the error instead of truncating the hostname.\nThat (on top of your patch) would look like the following.\n\nThoughts?\nJonathan\n\ndiff --git i/wrapper.c w/wrapper.c\nindex d837417709..e218bd3bef 100644\n--- i/wrapper.c\n+++ w/wrapper.c\n@@ -660,11 +660,13 @@ 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 * guarantee that an error will be returned. Check for ourselves\n+\t * to be safe.\n \t */\n \tint ret = gethostname(buf, len);\n-\tif (!ret)\n-\t\tbuf[len - 1] = 0;\n+\tif (!ret && !memchr(buf, 0, len)) {\n+\t\terrno = ENAMETOOLONG;\n+\t\treturn -1;\n+\t}\n \treturn ret;\n }\n"},{"id":"317177","messageId":"xmqq8tmxcdt3.fsf@gitster.mtv.corp.google.com","threadId":"45737","inReplyTo":"20170418215743.18406-3-dturner@twosigma.com","subject":"Re: [PATCH v3 2/2] xgethostname: handle long hostnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T02:48:56Z","receivedAt":"2017-04-19T02:49:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\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. Introduce\n\nThe name of the character whose ASCII value is '\\0' is NUL, not\nnull (similarly for in-code comment).\n"},{"id":"317179","messageId":"xmqq4lxlcdpf.fsf@gitster.mtv.corp.google.com","threadId":"45737","inReplyTo":"20170419013552.GB28740@aiede.svl.corp.google.com","subject":"Re: [PATCH v3 2/2] xgethostname: handle long hostnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T02:51:08Z","receivedAt":"2017-04-19T02:51:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\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. Introduce\n>> new function, xgethostname, which ensures that there is always a \\0\n>> at the end of the buffer.\n>\n> I think we should detect the error instead of truncating the hostname.\n> That (on top of your patch) would look like the following.\n>\n> Thoughts?\n> Jonathan\n>\n> diff --git i/wrapper.c w/wrapper.c\n> index d837417709..e218bd3bef 100644\n> --- i/wrapper.c\n> +++ w/wrapper.c\n> @@ -660,11 +660,13 @@ 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 * guarantee that an error will be returned. Check for ourselves\n> +\t * to be safe.\n> \t */\n> \tint ret = gethostname(buf, len);\n> -\tif (!ret)\n> -\t\tbuf[len - 1] = 0;\n> +\tif (!ret && !memchr(buf, 0, len)) {\n> +\t\terrno = ENAMETOOLONG;\n> +\t\treturn -1;\n> +\t}\n\nHmmmm. \"Does not specify if the buffer will be NUL-terminated\"\nwould mean that it is OK for the platform gethostname() to stuff\nsizeof(buf)-1 first bytes of the hostname in the buffer and then\ntruncate by placing '\\0' at the end of the buf, and we would not\nnotice truncation with the above change on such a platform, no?\n"},{"id":"317181","messageId":"xmqqzifdayuc.fsf@gitster.mtv.corp.google.com","threadId":"45737","inReplyTo":"20170419012824.GA28740@aiede.svl.corp.google.com","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T02:57:31Z","receivedAt":"2017-04-19T02:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> @@ -274,7 +278,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>\n> I hoped this could be simplified since HOST_NAME_MAX is a numeric literal,\n> using the double-expansion trick:\n>\n> #define STR_(s) # s\n> #define STR(s) STR_(s)\n>\n> \t\t\tfscanf(fp, \"%\" SCNuMAX \" %\" STR(HOST_NAME_MAX) \"c\",\n> \t\t\t &pid, locking_host);\n>\n> Unfortunately, I don't think there's anything stopping a platform from\n> defining\n>\n> \t#define HOST_NAME_MAX 0x100\n>\n> which would break that.\n\nYes, that was exactly why I went to the xstrfmt() route when I sent\nmine yesterday ;-).\n\n> So this run-time calculation appears to be necessary.\n>\n> Reviewed-by: Jonathan Nieder \n\nThanks. \n"},{"id":"317238","messageId":"269cbe1c-d10a-7022-1977-2128d59aebb3@web.de","threadId":"45737","inReplyTo":"20170419012824.GA28740@aiede.svl.corp.google.com","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-19T14:03:19Z","receivedAt":"2017-04-19T14:03:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.04.2017 um 03:28 schrieb Jonathan Nieder:\n>> From: René Scharfe \n>>\n>> POSIX limits the length of host names to HOST_NAME_MAX. Export the\n>> fallback definition from daemon.c and use this constant to make all\n>> buffers used with gethostname(2) big enough for any possible result\n>> and a terminating NUL.\n> \n> Since some platforms do not define HOST_NAME_MAX and we provide a\n> fallback, this is not actually big enough for any possible result.\n> For example, the Hurd allows arbitrarily long hostnames.\n\nInteresting. No limits, eh? They suggest to allocate memory\ndynamically [1]. Perhaps we should import their xgethostname() (which\ngrows a buffer as needed), or implement a strbuf_add_hostname()?\n\nRené\n\n\nhttps://www.gnu.org/software/hurd/hurd/porting/guidelines.html#MAXHOSTNAMELEN_tt_\n"},{"id":"317242","messageId":"0701e70b52fe4bdd8e04e4c6918aab7a@exmbdft7.ad.twosigma.com","threadId":"45737","inReplyTo":"xmqq4lxlcdpf.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH v3 2/2] xgethostname: handle long hostnames","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-19T15:50:34Z","receivedAt":"2017-04-19T15:50:43Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Tuesday, April 18, 2017 10:51 PM\n> To: Jonathan Nieder \n> Cc: David Turner ; git@vger.kernel.org;\n> l.s.r@web.de\n> Subject: Re: [PATCH v3 2/2] xgethostname: handle long hostnames\n> \n> Jonathan Nieder writes:\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. Introduce\n> >> new function, xgethostname, which ensures that there is always a \\0\n> >> at the end of the buffer.\n> >\n> > I think we should detect the error instead of truncating the hostname.\n> > That (on top of your patch) would look like the following.\n> >\n> > Thoughts?\n> > Jonathan\n> >\n> > diff --git i/wrapper.c w/wrapper.c\n> > index d837417709..e218bd3bef 100644\n> > --- i/wrapper.c\n> > +++ w/wrapper.c\n> > @@ -660,11 +660,13 @@ 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 * guarantee that an error will be returned. Check for ourselves\n> > +\t * to be safe.\n> > \t */\n> > \tint ret = gethostname(buf, len);\n> > -\tif (!ret)\n> > -\t\tbuf[len - 1] = 0;\n> > +\tif (!ret && !memchr(buf, 0, len)) {\n> > +\t\terrno = ENAMETOOLONG;\n> > +\t\treturn -1;\n> > +\t}\n> \n> Hmmmm. \"Does not specify if the buffer will be NUL-terminated\"\n> would mean that it is OK for the platform gethostname() to stuff\n> sizeof(buf)-1 first bytes of the hostname in the buffer and then truncate by\n> placing '\\0' at the end of the buf, and we would not notice truncation with the\n> above change on such a platform, no?\n\nMy read of the docs is that not only is that OK, but it is also permitted\nfor the platform to put sizeof(buf) bytes into the buffer and *not* \nput \\0 at the end.\n\nSo in order to do a dynamic approach, we would have to allocate some\nbuffer, then run gethostname, then check if the penultimate element \nof the buffer was written to, and if so, allocate a larger buffer. Yucky,\nbut possible.\n\n"},{"id":"317246","messageId":"ae1d9cb2-dd4b-4bc7-469f-1b7729811f5a@web.de","threadId":"45737","inReplyTo":"0701e70b52fe4bdd8e04e4c6918aab7a@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH v3 2/2] xgethostname: handle long hostnames","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-19T16:43:59Z","receivedAt":"2017-04-19T16:44:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.04.2017 um 17:50 schrieb David Turner:\n>> -----Original Message-----\n>> From: Junio C Hamano [mailto:gitster@pobox.com]\n>> Sent: Tuesday, April 18, 2017 10:51 PM\n>> To: Jonathan Nieder \n>> Cc: David Turner ; git@vger.kernel.org;\n>> l.s.r@web.de\n>> Subject: Re: [PATCH v3 2/2] xgethostname: handle long hostnames\n>>\n>> Jonathan Nieder writes:\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. Introduce\n>>>> new function, xgethostname, which ensures that there is always a \\0\n>>>> at the end of the buffer.\n>>>\n>>> I think we should detect the error instead of truncating the hostname.\n>>> That (on top of your patch) would look like the following.\n>>>\n>>> Thoughts?\n>>> Jonathan\n>>>\n>>> diff --git i/wrapper.c w/wrapper.c\n>>> index d837417709..e218bd3bef 100644\n>>> --- i/wrapper.c\n>>> +++ w/wrapper.c\n>>> @@ -660,11 +660,13 @@ 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 * guarantee that an error will be returned. Check for ourselves\n>>> +\t * to be safe.\n>>> \t */\n>>> \tint ret = gethostname(buf, len);\n>>> -\tif (!ret)\n>>> -\t\tbuf[len - 1] = 0;\n>>> +\tif (!ret && !memchr(buf, 0, len)) {\n>>> +\t\terrno = ENAMETOOLONG;\n>>> +\t\treturn -1;\n>>> +\t}\n>>\n>> Hmmmm. \"Does not specify if the buffer will be NUL-terminated\"\n>> would mean that it is OK for the platform gethostname() to stuff\n>> sizeof(buf)-1 first bytes of the hostname in the buffer and then truncate by\n>> placing '\\0' at the end of the buf, and we would not notice truncation with the\n>> above change on such a platform, no?\n> \n> My read of the docs is that not only is that OK, but it is also permitted\n> for the platform to put sizeof(buf) bytes into the buffer and *not*\n> put \\0 at the end.\n\nThat sounds crazy, but that's how I read the spec [1] as well. And\nPOSIX also doesn't specify any errors for gethostname. But that\nmakes kinda sense because it *does* specify HOST_NAME_MAX as maximum\nsize. Things get more interesting when this spec meets systems that\ndon't have HOST_NAME_MAX, or error returns, or bugs.\n\n> So in order to do a dynamic approach, we would have to allocate some\n> buffer, then run gethostname, then check if the penultimate element\n> of the buffer was written to, and if so, allocate a larger buffer. Yucky,\n> but possible.\n\nThat's what the gnulib version of xgethostname does [2], among\nother things.\n\nThe more I read about gethostname and its weirdness, the more I\nthink we should import an existing, proven version of xgethostname\nthat returns an allocated buffer. That way we wouldn't have to\nworry about truncation or missing NULs or buffer sizes anymore.\nWhat do you think?\n\nI found the one from gnulib and from Neal Walfield [3] mentioned\nin the Hurd docs; are there more?\n\nRené\n\n\n[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/gethostname.html\n[2] http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/xgethostname.c;hb=0632e115747ff96e93330c88f536d7354a7ce507\n[3] http://walfield.org/pub/people/neal/xgethostname/\n[4] https://www.gnu.org/software/hurd/hurd/porting/guidelines.html#MAXHOSTNAMELEN_tt_\n"},{"id":"317257","messageId":"c0333c81-d3b2-ca2d-a553-75642d8fb949@web.de","threadId":"45737","inReplyTo":"20170419012824.GA28740@aiede.svl.corp.google.com","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-19T17:28:29Z","receivedAt":"2017-04-19T17:28:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.04.2017 um 03:28 schrieb Jonathan Nieder:\n> David Turner wrote:\n>> @@ -274,7 +278,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> \n> I hoped this could be simplified since HOST_NAME_MAX is a numeric literal,\n> using the double-expansion trick:\n> \n> #define STR_(s) # s\n> #define STR(s) STR_(s)\n> \n> \t\t\tfscanf(fp, \"%\" SCNuMAX \" %\" STR(HOST_NAME_MAX) \"c\",\n> \t\t\t &pid, locking_host);\n> \n> Unfortunately, I don't think there's anything stopping a platform from\n> defining\n> \n> \t#define HOST_NAME_MAX 0x100\n> \n> which would break that.\n> \n> So this run-time calculation appears to be necessary.\n\nI had another look at this last night and cooked up the following\npatch. Might have gone overboard with it..\n\n-- >8 --\nSubject: [PATCH] gc: support arbitrary hostnames and pids in lock_repo_for_gc()\n\ngit gc writes its pid and hostname into a pidfile to prevent concurrent\ngarbage collection. Repositories may be shared between systems with\ndifferent limits for host name length and different pid ranges. Use a\nstrbuf to store the file contents to allow for arbitrarily long\nhostnames and pids to be shown to the user on early abort.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/gc.c | 151 +++++++++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 111 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 2daede7820..4c1c01e87d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -228,21 +228,99 @@ static int need_to_gc(void)\n \treturn 1;\n }\n \n+struct pidfile {\n+\tstruct strbuf buf;\n+\tchar *hostname;\n+};\n+\n+#define PIDFILE_INIT { STRBUF_INIT }\n+\n+static void pidfile_release(struct pidfile *pf)\n+{\n+\tpf->hostname = NULL;\n+\tstrbuf_release(&pf->buf);\n+}\n+\n+static int pidfile_read(struct pidfile *pf, const char *path,\n+\t\t\tunsigned int max_age_seconds)\n+{\n+\tint fd;\n+\tstruct stat st;\n+\tssize_t len;\n+\tchar *space;\n+\tint rc = -1;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn rc;\n+\n+\tif (fstat(fd, &st))\n+\t\tgoto out;\n+\tif (time(NULL) - st.st_mtime > max_age_seconds)\n+\t\tgoto out;\n+\tif (st.st_size > (size_t)st.st_size)\n+\t\tgoto out;\n+\n+\tlen = strbuf_read(&pf->buf, fd, st.st_size);\n+\tif (len < 0)\n+\t\tgoto out;\n+\n+\tspace = strchr(pf->buf.buf, ' ');\n+\tif (!space) {\n+\t\tpidfile_release(pf);\n+\t\tgoto out;\n+\t}\n+\tpf->hostname = space + 1;\n+\t*space = '\\0';\n+\n+\trc = 0;\n+out:\n+\tclose(fd);\n+\treturn rc;\n+}\n+\n+static int parse_pid(const char *value, pid_t *ret)\n+{\n+\tif (value && *value) {\n+\t\tchar *end;\n+\t\tintmax_t val;\n+\n+\t\terrno = 0;\n+\t\tval = strtoimax(value, &end, 0);\n+\t\tif (errno == ERANGE)\n+\t\t\treturn 0;\n+\t\tif (*end)\n+\t\t\treturn 0;\n+\t\tif (labs(val) > maximum_signed_value_of_type(pid_t)) {\n+\t\t\terrno = ERANGE;\n+\t\t\treturn 0;\n+\t\t}\n+\t\t*ret = val;\n+\t\treturn 1;\n+\t}\n+\terrno = EINVAL;\n+\treturn 0;\n+}\n+\n+static int pidfile_process_exists(const struct pidfile *pf)\n+{\n+\tpid_t pid;\n+\treturn parse_pid(pf->buf.buf, &pid) &&\n+\t\t(!kill(pid, 0) || errno == EPERM);\n+}\n+\n /* return NULL on success, else hostname running the gc */\n-static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n+static int lock_repo_for_gc(int force, struct pidfile *pf)\n {\n \tstatic struct lock_file lock;\n \tchar my_host[128];\n \tstruct strbuf sb = STRBUF_INIT;\n-\tstruct stat st;\n-\tuintmax_t pid;\n-\tFILE *fp;\n \tint fd;\n \tchar *pidfile_path;\n \n \tif (is_tempfile_active(&pidfile))\n \t\t/* already locked */\n-\t\treturn NULL;\n+\t\treturn 0;\n \n \tif (gethostname(my_host, sizeof(my_host)))\n \t\txsnprintf(my_host, sizeof(my_host), \"unknown\");\n@@ -251,34 +329,27 @@ 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\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-\t\t\t/*\n-\t\t\t * 12 hour limit is very generous as gc should\n-\t\t\t * never take that long. On the other hand we\n-\t\t\t * don't really need a strict limit here,\n-\t\t\t * running gc --auto one day late is not a big\n-\t\t\t * problem. --force can be used in manual gc\n-\t\t\t * after the user verifies that no gc is\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/*\n+\t\t * 12 hour limit is very generous as gc should\n+\t\t * never take that long. On the other hand we\n+\t\t * don't really need a strict limit here,\n+\t\t * running gc --auto one day late is not a big\n+\t\t * problem. --force can be used in manual gc\n+\t\t * after the user verifies that no gc is\n+\t\t * running.\n+\t\t */\n+\t\tconst unsigned max_age_seconds = 12 * 3600;\n+\n+\t\tif (!pidfile_read(pf, pidfile_path, max_age_seconds)) {\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-\t\t\tfclose(fp);\n-\t\tif (should_exit) {\n-\t\t\tif (fd >= 0)\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\tif (strcmp(pf->hostname, my_host) ||\n+\t\t\t pidfile_process_exists(pf)) {\n+\t\t\t\tif (fd >= 0)\n+\t\t\t\t\trollback_lock_file(&lock);\n+\t\t\t\tfree(pidfile_path);\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t\tpidfile_release(pf);\n \t\t}\n \t}\n \n@@ -289,7 +360,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tcommit_lock_file(&lock);\n \tregister_tempfile(&pidfile, pidfile_path);\n \tfree(pidfile_path);\n-\treturn NULL;\n+\treturn 0;\n }\n \n static int report_last_gc_error(void)\n@@ -344,8 +415,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tint auto_gc = 0;\n \tint quiet = 0;\n \tint force = 0;\n-\tconst char *name;\n-\tpid_t pid;\n+\tstruct pidfile pf = PIDFILE_INIT;\n \tint daemonized = 0;\n \n \tstruct option builtin_gc_options[] = {\n@@ -420,12 +490,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tadd_repack_all_option();\n \n-\tname = lock_repo_for_gc(force, &pid);\n-\tif (name) {\n-\t\tif (auto_gc)\n+\tif (lock_repo_for_gc(force, &pf)) {\n+\t\tif (auto_gc) {\n+\t\t\tpidfile_release(&pf);\n \t\t\treturn 0; /* be quiet on --auto */\n-\t\tdie(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use --force if not)\"),\n-\t\t name, (uintmax_t)pid);\n+\t\t}\n+\t\tdie(_(\"gc is already running on machine '%s' pid %s (use --force if not)\"),\n+\t\t pf.hostname, pf.buf.buf);\n \t}\n \n \tif (daemonized) {\n-- \n2.12.2\n\n"},{"id":"317264","messageId":"b7a3844946934ecda1ba1ac5b972ff9d@exmbdft7.ad.twosigma.com","threadId":"45737","inReplyTo":"c0333c81-d3b2-ca2d-a553-75642d8fb949@web.de","subject":"RE: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-19T19:08:36Z","receivedAt":"2017-04-19T19:08:42Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> I had another look at this last night and cooked up the following patch. Might\n> have gone overboard with it..\n> \n> -- >8 --\n> Subject: [PATCH] gc: support arbitrary hostnames and pids in lock_repo_for_gc()\n> \n> git gc writes its pid and hostname into a pidfile to prevent concurrent garbage\n> collection. Repositories may be shared between systems with different limits\n> for host name length and different pid ranges. Use a strbuf to store the file\n> contents to allow for arbitrarily long hostnames and pids to be shown to the\n> user on early abort.\n\nThis is pretty paranoid, but maybe the remote host has a longer pid_t than we \ndo, so we should be using intmax_t when reading the pid, and only check its \nsize before passing it to kill?\n\n(Personally, I think this whole patch is kind of overkill, but some folks probably\nthink the same about my original patches, so I'm happy to live and let live).\n"},{"id":"317265","messageId":"1b3e983f-4a70-652b-53f3-0c571c6efa1e@web.de","threadId":"45737","inReplyTo":"c0333c81-d3b2-ca2d-a553-75642d8fb949@web.de","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-04-19T19:09:06Z","receivedAt":"2017-04-19T19:09:27Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-04-19 19:28, René Scharfe wrote:\n[]\nOne or two minor comments inline\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 2daede7820..4c1c01e87d 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -228,21 +228,99 @@ static int need_to_gc(void)\n> \treturn 1;\n> }\n> \n> +struct pidfile {\n> +\tstruct strbuf buf;\n> +\tchar *hostname;\n> +};\n> +\n> +#define PIDFILE_INIT { STRBUF_INIT }\n> +\n> +static void pidfile_release(struct pidfile *pf)\n> +{\n> +\tpf->hostname = NULL;\n> +\tstrbuf_release(&pf->buf);\n> +}\n> +\n> +static int pidfile_read(struct pidfile *pf, const char *path,\n> +\t\t\tunsigned int max_age_seconds)\n> +{\n> +\tint fd;\n> +\tstruct stat st;\n> +\tssize_t len;\n> +\tchar *space;\n> +\tint rc = -1;\n> +\n> +\tfd = open(path, O_RDONLY);\n> +\tif (fd < 0)\n> +\t\treturn rc;\n> +\n> +\tif (fstat(fd, &st))\n> +\t\tgoto out;\n> +\tif (time(NULL) - st.st_mtime > max_age_seconds)\n> +\t\tgoto out;\n> +\tif (st.st_size > (size_t)st.st_size)\n\nMinor: we need xsize_t here ?\nif (st.st_size > xsize_t(st.st_size))\n\n> +\t\tgoto out;\n> +\n> +\tlen = strbuf_read(&pf->buf, fd, st.st_size);\n> +\tif (len < 0)\n> +\t\tgoto out;\n> +\n> +\tspace = strchr(pf->buf.buf, ' ');\n> +\tif (!space) {\n> +\t\tpidfile_release(pf);\n> +\t\tgoto out;\n> +\t}\n> +\tpf->hostname = space + 1;\n> +\t*space = '\\0';\n> +\n> +\trc = 0;\n> +out:\n> +\tclose(fd);\n> +\treturn rc;\n> +}\n> +\n> +static int parse_pid(const char *value, pid_t *ret)\n> +{\n> +\tif (value && *value) {\n> +\t\tchar *end;\n> +\t\tintmax_t val;\n> +\n> +\t\terrno = 0;\n> +\t\tval = strtoimax(value, &end, 0);\n> +\t\tif (errno == ERANGE)\n> +\t\t\treturn 0;\n> +\t\tif (*end)\n> +\t\t\treturn 0;\n> +\t\tif (labs(val) > maximum_signed_value_of_type(pid_t)) {\n> +\t\t\terrno = ERANGE;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\t*ret = val;\n> +\t\treturn 1;\n> +\t}\n> +\terrno = EINVAL;\n> +\treturn 0;\n> +}\n> +\n> +static int pidfile_process_exists(const struct pidfile *pf)\n> +{\n> +\tpid_t pid;\n> +\treturn parse_pid(pf->buf.buf, &pid) &&\n> +\t\t(!kill(pid, 0) || errno == EPERM);\n> +}\n> +\n> /* return NULL on success, else hostname running the gc */\n> -static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n> +static int lock_repo_for_gc(int force, struct pidfile *pf)\n> {\n> \tstatic struct lock_file lock;\n> \tchar my_host[128];\n\nHuh ?\nshould this be increased, may be in another path ?\n\n\n"},{"id":"317267","messageId":"7d075a07-edc9-83eb-25cf-7f8b13700584@web.de","threadId":"45737","inReplyTo":"1b3e983f-4a70-652b-53f3-0c571c6efa1e@web.de","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-19T20:02:59Z","receivedAt":"2017-04-19T20:03:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.04.2017 um 21:09 schrieb Torsten Bögershausen:\n> On 2017-04-19 19:28, René Scharfe wrote:\n> []\n> One or two minor comments inline\n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index 2daede7820..4c1c01e87d 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -228,21 +228,99 @@ static int need_to_gc(void)\n>> \treturn 1;\n>> }\n>> \n>> +struct pidfile {\n>> +\tstruct strbuf buf;\n>> +\tchar *hostname;\n>> +};\n>> +\n>> +#define PIDFILE_INIT { STRBUF_INIT }\n>> +\n>> +static void pidfile_release(struct pidfile *pf)\n>> +{\n>> +\tpf->hostname = NULL;\n>> +\tstrbuf_release(&pf->buf);\n>> +}\n>> +\n>> +static int pidfile_read(struct pidfile *pf, const char *path,\n>> +\t\t\tunsigned int max_age_seconds)\n>> +{\n>> +\tint fd;\n>> +\tstruct stat st;\n>> +\tssize_t len;\n>> +\tchar *space;\n>> +\tint rc = -1;\n>> +\n>> +\tfd = open(path, O_RDONLY);\n>> +\tif (fd < 0)\n>> +\t\treturn rc;\n>> +\n>> +\tif (fstat(fd, &st))\n>> +\t\tgoto out;\n>> +\tif (time(NULL) - st.st_mtime > max_age_seconds)\n>> +\t\tgoto out;\n>> +\tif (st.st_size > (size_t)st.st_size)\n> \n> Minor: we need xsize_t here ?\n> if (st.st_size > xsize_t(st.st_size))\n\nNo, xsize_t() would do the same check and die on overflow, and \npidfile_read() is supposed to handle big pids gracefully.\n\n> \n>> +\t\tgoto out;\n>> +\n>> +\tlen = strbuf_read(&pf->buf, fd, st.st_size);\n>> +\tif (len < 0)\n>> +\t\tgoto out;\n>> +\n>> +\tspace = strchr(pf->buf.buf, ' ');\n>> +\tif (!space) {\n>> +\t\tpidfile_release(pf);\n>> +\t\tgoto out;\n>> +\t}\n>> +\tpf->hostname = space + 1;\n>> +\t*space = '\\0';\n>> +\n>> +\trc = 0;\n>> +out:\n>> +\tclose(fd);\n>> +\treturn rc;\n>> +}\n>> +\n>> +static int parse_pid(const char *value, pid_t *ret)\n>> +{\n>> +\tif (value && *value) {\n>> +\t\tchar *end;\n>> +\t\tintmax_t val;\n>> +\n>> +\t\terrno = 0;\n>> +\t\tval = strtoimax(value, &end, 0);\n>> +\t\tif (errno == ERANGE)\n>> +\t\t\treturn 0;\n>> +\t\tif (*end)\n>> +\t\t\treturn 0;\n>> +\t\tif (labs(val) > maximum_signed_value_of_type(pid_t)) {\n>> +\t\t\terrno = ERANGE;\n>> +\t\t\treturn 0;\n>> +\t\t}\n>> +\t\t*ret = val;\n>> +\t\treturn 1;\n>> +\t}\n>> +\terrno = EINVAL;\n>> +\treturn 0;\n>> +}\n>> +\n>> +static int pidfile_process_exists(const struct pidfile *pf)\n>> +{\n>> +\tpid_t pid;\n>> +\treturn parse_pid(pf->buf.buf, &pid) &&\n>> +\t\t(!kill(pid, 0) || errno == EPERM);\n>> +}\n>> +\n>> /* return NULL on success, else hostname running the gc */\n>> -static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>> +static int lock_repo_for_gc(int force, struct pidfile *pf)\n>> {\n>> \tstatic struct lock_file lock;\n>> \tchar my_host[128];\n> \n> Huh ?\n> should this be increased, may be in another path ?\n\nIt should, but not in this patch.\n\nRené\n"},{"id":"317364","messageId":"a718ca38-4c07-9f3d-e7f5-9efd7ef59007@web.de","threadId":"45737","inReplyTo":"7d075a07-edc9-83eb-25cf-7f8b13700584@web.de","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-04-20T18:37:29Z","receivedAt":"2017-04-20T18:37:58Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-04-19 22:02, René Scharfe wrote:\n> Am 19.04.2017 um 21:09 schrieb Torsten Bögershausen:\n>> On 2017-04-19 19:28, René Scharfe wrote:\n>> []\n>> One or two minor comments inline\n>>> diff --git a/builtin/gc.c b/builtin/gc.c\n>>> index 2daede7820..4c1c01e87d 100644\n>>> --- a/builtin/gc.c\n>>> +++ b/builtin/gc.c\n>>> @@ -228,21 +228,99 @@ static int need_to_gc(void)\n>>> return 1;\n>>> }\n>>> +struct pidfile {\n>>> + struct strbuf buf;\n>>> + char *hostname;\n>>> +};\n>>> +\n>>> +#define PIDFILE_INIT { STRBUF_INIT }\n>>> +\n>>> +static void pidfile_release(struct pidfile *pf)\n>>> +{\n>>> + pf->hostname = NULL;\n>>> + strbuf_release(&pf->buf);\n>>> +}\n>>> +\n>>> +static int pidfile_read(struct pidfile *pf, const char *path,\n>>> + unsigned int max_age_seconds)\n>>> +{\n>>> + int fd;\n>>> + struct stat st;\n>>> + ssize_t len;\n>>> + char *space;\n>>> + int rc = -1;\n>>> +\n>>> + fd = open(path, O_RDONLY);\n>>> + if (fd < 0)\n>>> + return rc;\n>>> +\n>>> + if (fstat(fd, &st))\n>>> + goto out;\n>>> + if (time(NULL) - st.st_mtime > max_age_seconds)\n>>> + goto out;\n>>> + if (st.st_size > (size_t)st.st_size)\n>>\n>> Minor: we need xsize_t here ?\n>> if (st.st_size > xsize_t(st.st_size))\n> \n> No, xsize_t() would do the same check and die on overflow, and pidfile_read() is\n> supposed to handle big pids gracefully.\nThis about the file size, isn't it ?\nAnd here xsize_t should be save to use and good practise.\n\n\n\n\n"},{"id":"317366","messageId":"0882e8f6-f9e2-20c2-4e94-3fa8a50097c9@web.de","threadId":"45737","inReplyTo":"a718ca38-4c07-9f3d-e7f5-9efd7ef59007@web.de","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-20T19:28:29Z","receivedAt":"2017-04-20T19:28:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.04.2017 um 20:37 schrieb Torsten Bögershausen:\n> On 2017-04-19 22:02, René Scharfe wrote:\n>> Am 19.04.2017 um 21:09 schrieb Torsten Bögershausen:\n>>> On 2017-04-19 19:28, René Scharfe wrote:\n>>> []\n>>> One or two minor comments inline\n>>>> diff --git a/builtin/gc.c b/builtin/gc.c\n>>>> index 2daede7820..4c1c01e87d 100644\n>>>> --- a/builtin/gc.c\n>>>> +++ b/builtin/gc.c\n>>>> @@ -228,21 +228,99 @@ static int need_to_gc(void)\n>>>> return 1;\n>>>> }\n>>>> +struct pidfile {\n>>>> + struct strbuf buf;\n>>>> + char *hostname;\n>>>> +};\n>>>> +\n>>>> +#define PIDFILE_INIT { STRBUF_INIT }\n>>>> +\n>>>> +static void pidfile_release(struct pidfile *pf)\n>>>> +{\n>>>> + pf->hostname = NULL;\n>>>> + strbuf_release(&pf->buf);\n>>>> +}\n>>>> +\n>>>> +static int pidfile_read(struct pidfile *pf, const char *path,\n>>>> + unsigned int max_age_seconds)\n>>>> +{\n>>>> + int fd;\n>>>> + struct stat st;\n>>>> + ssize_t len;\n>>>> + char *space;\n>>>> + int rc = -1;\n>>>> +\n>>>> + fd = open(path, O_RDONLY);\n>>>> + if (fd < 0)\n>>>> + return rc;\n>>>> +\n>>>> + if (fstat(fd, &st))\n>>>> + goto out;\n>>>> + if (time(NULL) - st.st_mtime > max_age_seconds)\n>>>> + goto out;\n>>>> + if (st.st_size > (size_t)st.st_size)\n>>>\n>>> Minor: we need xsize_t here ?\n>>> if (st.st_size > xsize_t(st.st_size))\n>>\n>> No, xsize_t() would do the same check and die on overflow, and pidfile_read() is\n>> supposed to handle big pids gracefully.\n> This about the file size, isn't it ?\n> And here xsize_t should be save to use and good practise.\n\nI think I meant to write \"big pidfiles\" there.\n\nWith xsize_t() gc would die when seeing a pidfile whose size doesn't fit \ninto size_t. The version I sent just ignores such files. However, it \nwould choke on slightly smaller files that happen to not fit into \nmemory. And no reasonable pidfile can be big enough to trigger any of \nthat, so dying on conversion error wouldn't really be a problem. Is \nthat what you meant? It makes sense, in any case.\n\nThanks,\nRené\n"},{"id":"317455","messageId":"62e3185b-3cbb-f0b4-47be-f4093bfb9a6a@web.de","threadId":"45737","inReplyTo":"0882e8f6-f9e2-20c2-4e94-3fa8a50097c9@web.de","subject":"Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-04-21T04:18:42Z","receivedAt":"2017-04-21T04:18:41Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n> I think I meant to write \"big pidfiles\" there.\n>\n> With xsize_t() gc would die when seeing a pidfile whose size doesn't fit into\n> size_t. The version I sent just ignores such files. However, it would choke\n> on slightly smaller files that happen to not fit into memory. And no\n> reasonable pidfile can be big enough to trigger any of that, so dying on\n> conversion error wouldn't really be a problem. Is that what you meant? It\n> makes sense, in any case.\n\nIn short: Yes.\n\n>\n> Thanks,\n> René\n"}]}