[PATCH v3 0/2] gethostbyname fixes

[PATCH v3 0/2] gethostbyname fixes

[David Turner]

This version includes Junio's fixup to René's patch, and then my patch
rebased on top of René's.  I thought it was easier to just send both
in one series, than to have Junio do a bunch of conflict resolution.
I think this still needs Junio's signoff on the first patch, since
I've added his code.

David Turner (1):
  xgethostname: handle long hostnames

René Scharfe (1):
  use HOST_NAME_MAX to size buffers for gethostname(2)

 builtin/gc.c           | 12 ++++++++----
 builtin/receive-pack.c |  4 ++--
 daemon.c               |  4 ----
 fetch-pack.c           |  4 ++--
 git-compat-util.h      |  6 ++++++
 ident.c                |  4 ++--
 wrapper.c              | 13 +++++++++++++
 7 files changed, 33 insertions(+), 14 deletions(-)

-- 
2.11.GIT

[PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[David Turner]

From: René Scharfe <l.s.r@web.de>

POSIX limits the length of host names to HOST_NAME_MAX.  Export the
fallback definition from daemon.c and use this constant to make all
buffers used with gethostname(2) big enough for any possible result
and a terminating NUL.

Inspired-by: David Turner <dturner@twosigma.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: David Turner <dturner@twosigma.com>
---
 builtin/gc.c           | 10 +++++++---
 builtin/receive-pack.c |  2 +-
 daemon.c               |  4 ----
 fetch-pack.c           |  2 +-
 git-compat-util.h      |  4 ++++
 ident.c                |  2 +-
 6 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c2c61a57bb..4c4a36e2b5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -238,7 +238,7 @@ static int need_to_gc(void)
 static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 {
 	static struct lock_file lock;
-	char my_host[128];
+	char my_host[HOST_NAME_MAX + 1];
 	struct strbuf sb = STRBUF_INIT;
 	struct stat st;
 	uintmax_t pid;
@@ -257,8 +257,12 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	fd = hold_lock_file_for_update(&lock, pidfile_path,
 				       LOCK_DIE_ON_ERROR);
 	if (!force) {
-		static char locking_host[128];
+		static char locking_host[HOST_NAME_MAX + 1];
+		static char *scan_fmt;
 		int should_exit;
+
+		if (!scan_fmt)
+			scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX, HOST_NAME_MAX);
 		fp = fopen(pidfile_path, "r");
 		memset(locking_host, 0, sizeof(locking_host));
 		should_exit =
@@ -274,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 			 * running.
 			 */
 			time(NULL) - st.st_mtime <= 12 * 3600 &&
-			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
+			fscanf(fp, scan_fmt, &pid, locking_host) == 2 &&
 			/* be gentle to concurrent "gc" on remote hosts */
 			(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
 		if (fp != NULL)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index aca9c33d8d..2612efad3d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1695,7 +1695,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		if (status)
 			return "unpack-objects abnormal exit";
 	} else {
-		char hostname[256];
+		char hostname[HOST_NAME_MAX + 1];
 
 		argv_array_pushl(&child.args, "index-pack",
 				 "--stdin", hdr_arg, NULL);
diff --git a/daemon.c b/daemon.c
index 473e6b6b63..1503e1ed6f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -4,10 +4,6 @@
 #include "strbuf.h"
 #include "string-list.h"
 
-#ifndef HOST_NAME_MAX
-#define HOST_NAME_MAX 256
-#endif
-
 #ifdef NO_INITGROUPS
 #define initgroups(x, y) (0) /* nothing */
 #endif
diff --git a/fetch-pack.c b/fetch-pack.c
index d07d85ce30..055f568775 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -802,7 +802,7 @@ static int get_pack(struct fetch_pack_args *args,
 		if (args->use_thin_pack)
 			argv_array_push(&cmd.args, "--fix-thin");
 		if (args->lock_pack || unpack_limit) {
-			char hostname[256];
+			char hostname[HOST_NAME_MAX + 1];
 			if (gethostname(hostname, sizeof(hostname)))
 				xsnprintf(hostname, sizeof(hostname), "localhost");
 			argv_array_pushf(&cmd.args,
diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..46f3abe401 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -884,6 +884,10 @@ static inline size_t xsize_t(off_t len)
 __attribute__((format (printf, 3, 4)))
 extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);
 
+#ifndef HOST_NAME_MAX
+#define HOST_NAME_MAX 256
+#endif
+
 /* in ctype.c, for kwset users */
 extern const unsigned char tolower_trans_tbl[256];
 
diff --git a/ident.c b/ident.c
index c0364fe3a1..556851cf94 100644
--- a/ident.c
+++ b/ident.c
@@ -120,7 +120,7 @@ static int canonical_name(const char *host, struct strbuf *out)
 
 static void add_domainname(struct strbuf *out, int *is_bogus)
 {
-	char buf[1024];
+	char buf[HOST_NAME_MAX + 1];
 
 	if (gethostname(buf, sizeof(buf))) {
 		warning_errno("cannot get host name");
-- 
2.11.GIT

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[Jonathan Nieder]

Hi,

David Turner wrote:

> From: René Scharfe <l.s.r@web.de>
>
> POSIX limits the length of host names to HOST_NAME_MAX.  Export the
> fallback definition from daemon.c and use this constant to make all
> buffers used with gethostname(2) big enough for any possible result
> and a terminating NUL.

Since some platforms do not define HOST_NAME_MAX and we provide a
fallback, this is not actually big enough for any possible result.
For example, the Hurd allows arbitrarily long hostnames.

Nevertheless this patch seems like the right thing to do.

> Inspired-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
>  builtin/gc.c           | 10 +++++++---
>  builtin/receive-pack.c |  2 +-
>  daemon.c               |  4 ----
>  fetch-pack.c           |  2 +-
>  git-compat-util.h      |  4 ++++
>  ident.c                |  2 +-
>  6 files changed, 14 insertions(+), 10 deletions(-)

Thanks for picking this up.

[...]
> +++ b/builtin/gc.c
[...]
> @@ -257,8 +257,12 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>  	fd = hold_lock_file_for_update(&lock, pidfile_path,
>  				       LOCK_DIE_ON_ERROR);
>  	if (!force) {
> -		static char locking_host[128];
> +		static char locking_host[HOST_NAME_MAX + 1];
> +		static char *scan_fmt;
>  		int should_exit;
> +
> +		if (!scan_fmt)
> +			scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX, HOST_NAME_MAX);
>  		fp = fopen(pidfile_path, "r");
>  		memset(locking_host, 0, sizeof(locking_host));
>  		should_exit =
> @@ -274,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>  			 * running.
>  			 */
>  			time(NULL) - st.st_mtime <= 12 * 3600 &&
> -			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
> +			fscanf(fp, scan_fmt, &pid, locking_host) == 2 &&

I hoped this could be simplified since HOST_NAME_MAX is a numeric literal,
using the double-expansion trick:

#define STR_(s) # s
#define STR(s) STR_(s)

			fscanf(fp, "%" SCNuMAX " %" STR(HOST_NAME_MAX) "c",
			       &pid, locking_host);

Unfortunately, I don't think there's anything stopping a platform from
defining

	#define HOST_NAME_MAX 0x100

which would break that.

So this run-time calculation appears to be necessary.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> @@ -274,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>>  			 * running.
>>  			 */
>>  			time(NULL) - st.st_mtime <= 12 * 3600 &&
>> -			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
>> +			fscanf(fp, scan_fmt, &pid, locking_host) == 2 &&
>
> I hoped this could be simplified since HOST_NAME_MAX is a numeric literal,
> using the double-expansion trick:
>
> #define STR_(s) # s
> #define STR(s) STR_(s)
>
> 			fscanf(fp, "%" SCNuMAX " %" STR(HOST_NAME_MAX) "c",
> 			       &pid, locking_host);
>
> Unfortunately, I don't think there's anything stopping a platform from
> defining
>
> 	#define HOST_NAME_MAX 0x100
>
> which would break that.

Yes, that was exactly why I went to the xstrfmt() route when I sent
mine yesterday ;-).

> So this run-time calculation appears to be necessary.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.  

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[René Scharfe]

Am 19.04.2017 um 03:28 schrieb Jonathan Nieder:
>> From: René Scharfe <l.s.r@web.de>
>>
>> POSIX limits the length of host names to HOST_NAME_MAX.  Export the
>> fallback definition from daemon.c and use this constant to make all
>> buffers used with gethostname(2) big enough for any possible result
>> and a terminating NUL.
> 
> Since some platforms do not define HOST_NAME_MAX and we provide a
> fallback, this is not actually big enough for any possible result.
> For example, the Hurd allows arbitrarily long hostnames.

Interesting.  No limits, eh?  They suggest to allocate memory
dynamically [1].  Perhaps we should import their xgethostname() (which
grows a buffer as needed), or implement a strbuf_add_hostname()?

René


https://www.gnu.org/software/hurd/hurd/porting/guidelines.html#MAXHOSTNAMELEN_tt_

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[René Scharfe]

Am 19.04.2017 um 03:28 schrieb Jonathan Nieder:
> David Turner wrote:
>> @@ -274,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>>   			 * running.
>>   			 */
>>   			time(NULL) - st.st_mtime <= 12 * 3600 &&
>> -			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
>> +			fscanf(fp, scan_fmt, &pid, locking_host) == 2 &&
> 
> I hoped this could be simplified since HOST_NAME_MAX is a numeric literal,
> using the double-expansion trick:
> 
> #define STR_(s) # s
> #define STR(s) STR_(s)
> 
> 			fscanf(fp, "%" SCNuMAX " %" STR(HOST_NAME_MAX) "c",
> 			       &pid, locking_host);
> 
> Unfortunately, I don't think there's anything stopping a platform from
> defining
> 
> 	#define HOST_NAME_MAX 0x100
> 
> which would break that.
> 
> So this run-time calculation appears to be necessary.

I had another look at this last night and cooked up the following
patch.  Might have gone overboard with it..

-- >8 --
Subject: [PATCH] gc: support arbitrary hostnames and pids in lock_repo_for_gc()

git gc writes its pid and hostname into a pidfile to prevent concurrent
garbage collection.  Repositories may be shared between systems with
different limits for host name length and different pid ranges.  Use a
strbuf to store the file contents to allow for arbitrarily long
hostnames and pids to be shown to the user on early abort.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/gc.c | 151 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 111 insertions(+), 40 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 2daede7820..4c1c01e87d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -228,21 +228,99 @@ static int need_to_gc(void)
 	return 1;
 }
 
+struct pidfile {
+	struct strbuf buf;
+	char *hostname;
+};
+
+#define PIDFILE_INIT { STRBUF_INIT }
+
+static void pidfile_release(struct pidfile *pf)
+{
+	pf->hostname = NULL;
+	strbuf_release(&pf->buf);
+}
+
+static int pidfile_read(struct pidfile *pf, const char *path,
+			unsigned int max_age_seconds)
+{
+	int fd;
+	struct stat st;
+	ssize_t len;
+	char *space;
+	int rc = -1;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return rc;
+
+	if (fstat(fd, &st))
+		goto out;
+	if (time(NULL) - st.st_mtime > max_age_seconds)
+		goto out;
+	if (st.st_size > (size_t)st.st_size)
+		goto out;
+
+	len = strbuf_read(&pf->buf, fd, st.st_size);
+	if (len < 0)
+		goto out;
+
+	space = strchr(pf->buf.buf, ' ');
+	if (!space) {
+		pidfile_release(pf);
+		goto out;
+	}
+	pf->hostname = space + 1;
+	*space = '\0';
+
+	rc = 0;
+out:
+	close(fd);
+	return rc;
+}
+
+static int parse_pid(const char *value, pid_t *ret)
+{
+	if (value && *value) {
+		char *end;
+		intmax_t val;
+
+		errno = 0;
+		val = strtoimax(value, &end, 0);
+		if (errno == ERANGE)
+			return 0;
+		if (*end)
+			return 0;
+		if (labs(val) > maximum_signed_value_of_type(pid_t)) {
+			errno = ERANGE;
+			return 0;
+		}
+		*ret = val;
+		return 1;
+	}
+	errno = EINVAL;
+	return 0;
+}
+
+static int pidfile_process_exists(const struct pidfile *pf)
+{
+	pid_t pid;
+	return parse_pid(pf->buf.buf, &pid) &&
+		(!kill(pid, 0) || errno == EPERM);
+}
+
 /* return NULL on success, else hostname running the gc */
-static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
+static int lock_repo_for_gc(int force, struct pidfile *pf)
 {
 	static struct lock_file lock;
 	char my_host[128];
 	struct strbuf sb = STRBUF_INIT;
-	struct stat st;
-	uintmax_t pid;
-	FILE *fp;
 	int fd;
 	char *pidfile_path;
 
 	if (is_tempfile_active(&pidfile))
 		/* already locked */
-		return NULL;
+		return 0;
 
 	if (gethostname(my_host, sizeof(my_host)))
 		xsnprintf(my_host, sizeof(my_host), "unknown");
@@ -251,34 +329,27 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	fd = hold_lock_file_for_update(&lock, pidfile_path,
 				       LOCK_DIE_ON_ERROR);
 	if (!force) {
-		static char locking_host[128];
-		int should_exit;
-		fp = fopen(pidfile_path, "r");
-		memset(locking_host, 0, sizeof(locking_host));
-		should_exit =
-			fp != NULL &&
-			!fstat(fileno(fp), &st) &&
-			/*
-			 * 12 hour limit is very generous as gc should
-			 * never take that long. On the other hand we
-			 * don't really need a strict limit here,
-			 * running gc --auto one day late is not a big
-			 * problem. --force can be used in manual gc
-			 * after the user verifies that no gc is
-			 * running.
-			 */
-			time(NULL) - st.st_mtime <= 12 * 3600 &&
-			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
+		/*
+		 * 12 hour limit is very generous as gc should
+		 * never take that long. On the other hand we
+		 * don't really need a strict limit here,
+		 * running gc --auto one day late is not a big
+		 * problem. --force can be used in manual gc
+		 * after the user verifies that no gc is
+		 * running.
+		 */
+		const unsigned max_age_seconds = 12 * 3600;
+
+		if (!pidfile_read(pf, pidfile_path, max_age_seconds)) {
 			/* be gentle to concurrent "gc" on remote hosts */
-			(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
-		if (fp != NULL)
-			fclose(fp);
-		if (should_exit) {
-			if (fd >= 0)
-				rollback_lock_file(&lock);
-			*ret_pid = pid;
-			free(pidfile_path);
-			return locking_host;
+			if (strcmp(pf->hostname, my_host) ||
+			    pidfile_process_exists(pf)) {
+				if (fd >= 0)
+					rollback_lock_file(&lock);
+				free(pidfile_path);
+				return -1;
+			}
+			pidfile_release(pf);
 		}
 	}
 
@@ -289,7 +360,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	commit_lock_file(&lock);
 	register_tempfile(&pidfile, pidfile_path);
 	free(pidfile_path);
-	return NULL;
+	return 0;
 }
 
 static int report_last_gc_error(void)
@@ -344,8 +415,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int auto_gc = 0;
 	int quiet = 0;
 	int force = 0;
-	const char *name;
-	pid_t pid;
+	struct pidfile pf = PIDFILE_INIT;
 	int daemonized = 0;
 
 	struct option builtin_gc_options[] = {
@@ -420,12 +490,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	} else
 		add_repack_all_option();
 
-	name = lock_repo_for_gc(force, &pid);
-	if (name) {
-		if (auto_gc)
+	if (lock_repo_for_gc(force, &pf)) {
+		if (auto_gc) {
+			pidfile_release(&pf);
 			return 0; /* be quiet on --auto */
-		die(_("gc is already running on machine '%s' pid %"PRIuMAX" (use --force if not)"),
-		    name, (uintmax_t)pid);
+		}
+		die(_("gc is already running on machine '%s' pid %s (use --force if not)"),
+		    pf.hostname, pf.buf.buf);
 	}
 
 	if (daemonized) {
-- 
2.12.2

RE: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[David Turner]

> I had another look at this last night and cooked up the following patch.  Might
> have gone overboard with it..
> 
> -- >8 --
> Subject: [PATCH] gc: support arbitrary hostnames and pids in lock_repo_for_gc()
> 
> git gc writes its pid and hostname into a pidfile to prevent concurrent garbage
> collection.  Repositories may be shared between systems with different limits
> for host name length and different pid ranges.  Use a strbuf to store the file
> contents to allow for arbitrarily long hostnames and pids to be shown to the
> user on early abort.

This is pretty paranoid, but maybe the remote host has a longer pid_t than we 
do, so we should be using intmax_t when reading the pid, and only check its 
size  before passing it to kill?

(Personally, I think this whole patch is kind of overkill, but some folks probably
think the same about my original patches, so I'm happy to live and let live).

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[Torsten Bögershausen]

On 2017-04-19 19:28, René Scharfe wrote:
[]
One or two minor comments inline
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 2daede7820..4c1c01e87d 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -228,21 +228,99 @@ static int need_to_gc(void)
>  	return 1;
>  }
>  
> +struct pidfile {
> +	struct strbuf buf;
> +	char *hostname;
> +};
> +
> +#define PIDFILE_INIT { STRBUF_INIT }
> +
> +static void pidfile_release(struct pidfile *pf)
> +{
> +	pf->hostname = NULL;
> +	strbuf_release(&pf->buf);
> +}
> +
> +static int pidfile_read(struct pidfile *pf, const char *path,
> +			unsigned int max_age_seconds)
> +{
> +	int fd;
> +	struct stat st;
> +	ssize_t len;
> +	char *space;
> +	int rc = -1;
> +
> +	fd = open(path, O_RDONLY);
> +	if (fd < 0)
> +		return rc;
> +
> +	if (fstat(fd, &st))
> +		goto out;
> +	if (time(NULL) - st.st_mtime > max_age_seconds)
> +		goto out;
> +	if (st.st_size > (size_t)st.st_size)

Minor: we need xsize_t here ?
if (st.st_size > xsize_t(st.st_size))

> +		goto out;
> +
> +	len = strbuf_read(&pf->buf, fd, st.st_size);
> +	if (len < 0)
> +		goto out;
> +
> +	space = strchr(pf->buf.buf, ' ');
> +	if (!space) {
> +		pidfile_release(pf);
> +		goto out;
> +	}
> +	pf->hostname = space + 1;
> +	*space = '\0';
> +
> +	rc = 0;
> +out:
> +	close(fd);
> +	return rc;
> +}
> +
> +static int parse_pid(const char *value, pid_t *ret)
> +{
> +	if (value && *value) {
> +		char *end;
> +		intmax_t val;
> +
> +		errno = 0;
> +		val = strtoimax(value, &end, 0);
> +		if (errno == ERANGE)
> +			return 0;
> +		if (*end)
> +			return 0;
> +		if (labs(val) > maximum_signed_value_of_type(pid_t)) {
> +			errno = ERANGE;
> +			return 0;
> +		}
> +		*ret = val;
> +		return 1;
> +	}
> +	errno = EINVAL;
> +	return 0;
> +}
> +
> +static int pidfile_process_exists(const struct pidfile *pf)
> +{
> +	pid_t pid;
> +	return parse_pid(pf->buf.buf, &pid) &&
> +		(!kill(pid, 0) || errno == EPERM);
> +}
> +
>  /* return NULL on success, else hostname running the gc */
> -static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> +static int lock_repo_for_gc(int force, struct pidfile *pf)
>  {
>  	static struct lock_file lock;
>  	char my_host[128];

Huh ?
should this be increased, may be in another path ?

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[René Scharfe]

Am 19.04.2017 um 21:09 schrieb Torsten Bögershausen:
> On 2017-04-19 19:28, René Scharfe wrote:
> []
> One or two minor comments inline
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index 2daede7820..4c1c01e87d 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -228,21 +228,99 @@ static int need_to_gc(void)
>>   	return 1;
>>   }
>>   
>> +struct pidfile {
>> +	struct strbuf buf;
>> +	char *hostname;
>> +};
>> +
>> +#define PIDFILE_INIT { STRBUF_INIT }
>> +
>> +static void pidfile_release(struct pidfile *pf)
>> +{
>> +	pf->hostname = NULL;
>> +	strbuf_release(&pf->buf);
>> +}
>> +
>> +static int pidfile_read(struct pidfile *pf, const char *path,
>> +			unsigned int max_age_seconds)
>> +{
>> +	int fd;
>> +	struct stat st;
>> +	ssize_t len;
>> +	char *space;
>> +	int rc = -1;
>> +
>> +	fd = open(path, O_RDONLY);
>> +	if (fd < 0)
>> +		return rc;
>> +
>> +	if (fstat(fd, &st))
>> +		goto out;
>> +	if (time(NULL) - st.st_mtime > max_age_seconds)
>> +		goto out;
>> +	if (st.st_size > (size_t)st.st_size)
> 
> Minor: we need xsize_t here ?
> if (st.st_size > xsize_t(st.st_size))

No, xsize_t() would do the same check and die on overflow, and 
pidfile_read() is supposed to handle big pids gracefully.

> 
>> +		goto out;
>> +
>> +	len = strbuf_read(&pf->buf, fd, st.st_size);
>> +	if (len < 0)
>> +		goto out;
>> +
>> +	space = strchr(pf->buf.buf, ' ');
>> +	if (!space) {
>> +		pidfile_release(pf);
>> +		goto out;
>> +	}
>> +	pf->hostname = space + 1;
>> +	*space = '\0';
>> +
>> +	rc = 0;
>> +out:
>> +	close(fd);
>> +	return rc;
>> +}
>> +
>> +static int parse_pid(const char *value, pid_t *ret)
>> +{
>> +	if (value && *value) {
>> +		char *end;
>> +		intmax_t val;
>> +
>> +		errno = 0;
>> +		val = strtoimax(value, &end, 0);
>> +		if (errno == ERANGE)
>> +			return 0;
>> +		if (*end)
>> +			return 0;
>> +		if (labs(val) > maximum_signed_value_of_type(pid_t)) {
>> +			errno = ERANGE;
>> +			return 0;
>> +		}
>> +		*ret = val;
>> +		return 1;
>> +	}
>> +	errno = EINVAL;
>> +	return 0;
>> +}
>> +
>> +static int pidfile_process_exists(const struct pidfile *pf)
>> +{
>> +	pid_t pid;
>> +	return parse_pid(pf->buf.buf, &pid) &&
>> +		(!kill(pid, 0) || errno == EPERM);
>> +}
>> +
>>   /* return NULL on success, else hostname running the gc */
>> -static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>> +static int lock_repo_for_gc(int force, struct pidfile *pf)
>>   {
>>   	static struct lock_file lock;
>>   	char my_host[128];
> 
> Huh ?
> should this be increased, may be in another path ?

It should, but not in this patch.

René

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[Torsten Bögershausen]

On 2017-04-19 22:02, René Scharfe wrote:
> Am 19.04.2017 um 21:09 schrieb Torsten Bögershausen:
>> On 2017-04-19 19:28, René Scharfe wrote:
>> []
>> One or two minor comments inline
>>> diff --git a/builtin/gc.c b/builtin/gc.c
>>> index 2daede7820..4c1c01e87d 100644
>>> --- a/builtin/gc.c
>>> +++ b/builtin/gc.c
>>> @@ -228,21 +228,99 @@ static int need_to_gc(void)
>>>       return 1;
>>>   }
>>>   +struct pidfile {
>>> +    struct strbuf buf;
>>> +    char *hostname;
>>> +};
>>> +
>>> +#define PIDFILE_INIT { STRBUF_INIT }
>>> +
>>> +static void pidfile_release(struct pidfile *pf)
>>> +{
>>> +    pf->hostname = NULL;
>>> +    strbuf_release(&pf->buf);
>>> +}
>>> +
>>> +static int pidfile_read(struct pidfile *pf, const char *path,
>>> +            unsigned int max_age_seconds)
>>> +{
>>> +    int fd;
>>> +    struct stat st;
>>> +    ssize_t len;
>>> +    char *space;
>>> +    int rc = -1;
>>> +
>>> +    fd = open(path, O_RDONLY);
>>> +    if (fd < 0)
>>> +        return rc;
>>> +
>>> +    if (fstat(fd, &st))
>>> +        goto out;
>>> +    if (time(NULL) - st.st_mtime > max_age_seconds)
>>> +        goto out;
>>> +    if (st.st_size > (size_t)st.st_size)
>>
>> Minor: we need xsize_t here ?
>> if (st.st_size > xsize_t(st.st_size))
> 
> No, xsize_t() would do the same check and die on overflow, and pidfile_read() is
> supposed to handle big pids gracefully.
This about the file size, isn't it ?
And here xsize_t should be save to use and good practise.

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[René Scharfe]

Am 20.04.2017 um 20:37 schrieb Torsten Bögershausen:
> On 2017-04-19 22:02, René Scharfe wrote:
>> Am 19.04.2017 um 21:09 schrieb Torsten Bögershausen:
>>> On 2017-04-19 19:28, René Scharfe wrote:
>>> []
>>> One or two minor comments inline
>>>> diff --git a/builtin/gc.c b/builtin/gc.c
>>>> index 2daede7820..4c1c01e87d 100644
>>>> --- a/builtin/gc.c
>>>> +++ b/builtin/gc.c
>>>> @@ -228,21 +228,99 @@ static int need_to_gc(void)
>>>>        return 1;
>>>>    }
>>>>    +struct pidfile {
>>>> +    struct strbuf buf;
>>>> +    char *hostname;
>>>> +};
>>>> +
>>>> +#define PIDFILE_INIT { STRBUF_INIT }
>>>> +
>>>> +static void pidfile_release(struct pidfile *pf)
>>>> +{
>>>> +    pf->hostname = NULL;
>>>> +    strbuf_release(&pf->buf);
>>>> +}
>>>> +
>>>> +static int pidfile_read(struct pidfile *pf, const char *path,
>>>> +            unsigned int max_age_seconds)
>>>> +{
>>>> +    int fd;
>>>> +    struct stat st;
>>>> +    ssize_t len;
>>>> +    char *space;
>>>> +    int rc = -1;
>>>> +
>>>> +    fd = open(path, O_RDONLY);
>>>> +    if (fd < 0)
>>>> +        return rc;
>>>> +
>>>> +    if (fstat(fd, &st))
>>>> +        goto out;
>>>> +    if (time(NULL) - st.st_mtime > max_age_seconds)
>>>> +        goto out;
>>>> +    if (st.st_size > (size_t)st.st_size)
>>>
>>> Minor: we need xsize_t here ?
>>> if (st.st_size > xsize_t(st.st_size))
>>
>> No, xsize_t() would do the same check and die on overflow, and pidfile_read() is
>> supposed to handle big pids gracefully.
> This about the file size, isn't it ?
> And here xsize_t should be save to use and good practise.

I think I meant to write "big pidfiles" there.

With xsize_t() gc would die when seeing a pidfile whose size doesn't fit 
into size_t.  The version I sent just ignores such files.  However, it 
would choke on slightly smaller files that happen to not fit into 
memory.  And no reasonable pidfile can be big enough to trigger any of 
that, so dying on conversion error wouldn't really be a problem.  Is 
that what you meant?  It makes sense, in any case.

Thanks,
René

Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[Torsten Bögershausen]

> I think I meant to write "big pidfiles" there.
>
> With xsize_t() gc would die when seeing a pidfile whose size doesn't fit into
> size_t.  The version I sent just ignores such files.  However, it would choke
> on slightly smaller files that happen to not fit into memory.  And no
> reasonable pidfile can be big enough to trigger any of that, so dying on
> conversion error wouldn't really be a problem.  Is that what you meant?  It
> makes sense, in any case.

In short: Yes.

>
> Thanks,
> René

[PATCH v3 2/2] xgethostname: handle long hostnames

[David Turner]

If the full hostname doesn't fit in the buffer supplied to
gethostname, POSIX does not specify whether the buffer will be
null-terminated, so to be safe, we should do it ourselves.  Introduce
new function, xgethostname, which ensures that there is always a \0
at the end of the buffer.

Signed-off-by: David Turner <dturner@twosigma.com>
---
 builtin/gc.c           |  2 +-
 builtin/receive-pack.c |  2 +-
 fetch-pack.c           |  2 +-
 git-compat-util.h      |  2 ++
 ident.c                |  2 +-
 wrapper.c              | 13 +++++++++++++
 6 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4c4a36e2b5..33a1edabbc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -250,7 +250,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 		/* already locked */
 		return NULL;
 
-	if (gethostname(my_host, sizeof(my_host)))
+	if (xgethostname(my_host, sizeof(my_host)))
 		xsnprintf(my_host, sizeof(my_host), "unknown");
 
 	pidfile_path = git_pathdup("gc.pid");
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2612efad3d..0ca423a711 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1700,7 +1700,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		argv_array_pushl(&child.args, "index-pack",
 				 "--stdin", hdr_arg, NULL);
 
-		if (gethostname(hostname, sizeof(hostname)))
+		if (xgethostname(hostname, sizeof(hostname)))
 			xsnprintf(hostname, sizeof(hostname), "localhost");
 		argv_array_pushf(&child.args,
 				 "--keep=receive-pack %"PRIuMAX" on %s",
diff --git a/fetch-pack.c b/fetch-pack.c
index 055f568775..15d59a0440 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -803,7 +803,7 @@ static int get_pack(struct fetch_pack_args *args,
 			argv_array_push(&cmd.args, "--fix-thin");
 		if (args->lock_pack || unpack_limit) {
 			char hostname[HOST_NAME_MAX + 1];
-			if (gethostname(hostname, sizeof(hostname)))
+			if (xgethostname(hostname, sizeof(hostname)))
 				xsnprintf(hostname, sizeof(hostname), "localhost");
 			argv_array_pushf(&cmd.args,
 					"--keep=fetch-pack %"PRIuMAX " on %s",
diff --git a/git-compat-util.h b/git-compat-util.h
index 46f3abe401..bd04564a69 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -888,6 +888,8 @@ extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);
 #define HOST_NAME_MAX 256
 #endif
 
+extern int xgethostname(char *buf, size_t len);
+
 /* in ctype.c, for kwset users */
 extern const unsigned char tolower_trans_tbl[256];
 
diff --git a/ident.c b/ident.c
index 556851cf94..bea871c8e0 100644
--- a/ident.c
+++ b/ident.c
@@ -122,7 +122,7 @@ static void add_domainname(struct strbuf *out, int *is_bogus)
 {
 	char buf[HOST_NAME_MAX + 1];
 
-	if (gethostname(buf, sizeof(buf))) {
+	if (xgethostname(buf, sizeof(buf))) {
 		warning_errno("cannot get host name");
 		strbuf_addstr(out, "(none)");
 		*is_bogus = 1;
diff --git a/wrapper.c b/wrapper.c
index 0542fc7582..d837417709 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -655,3 +655,16 @@ void sleep_millisec(int millisec)
 {
 	poll(NULL, 0, millisec);
 }
+
+int xgethostname(char *buf, size_t len)
+{
+	/*
+	 * If the full hostname doesn't fit in buf, POSIX does not
+	 * specify whether the buffer will be null-terminated, so to
+	 * be safe, do it ourselves.
+	 */
+	int ret = gethostname(buf, len);
+	if (!ret)
+		buf[len - 1] = 0;
+	return ret;
+}
-- 
2.11.GIT

Re: [PATCH v3 2/2] xgethostname: handle long hostnames

[Jonathan Nieder]

Hi,

David Turner wrote:

> If the full hostname doesn't fit in the buffer supplied to
> gethostname, POSIX does not specify whether the buffer will be
> null-terminated, so to be safe, we should do it ourselves.  Introduce
> new function, xgethostname, which ensures that there is always a \0
> at the end of the buffer.

I think we should detect the error instead of truncating the hostname.
That (on top of your patch) would look like the following.

Thoughts?
Jonathan

diff --git i/wrapper.c w/wrapper.c
index d837417709..e218bd3bef 100644
--- i/wrapper.c
+++ w/wrapper.c
@@ -660,11 +660,13 @@ int xgethostname(char *buf, size_t len)
 {
 	/*
 	 * If the full hostname doesn't fit in buf, POSIX does not
-	 * specify whether the buffer will be null-terminated, so to
-	 * be safe, do it ourselves.
+	 * guarantee that an error will be returned. Check for ourselves
+	 * to be safe.
 	 */
 	int ret = gethostname(buf, len);
-	if (!ret)
-		buf[len - 1] = 0;
+	if (!ret && !memchr(buf, 0, len)) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
 	return ret;
 }

Re: [PATCH v3 2/2] xgethostname: handle long hostnames

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi,
>
> David Turner wrote:
>
>> If the full hostname doesn't fit in the buffer supplied to
>> gethostname, POSIX does not specify whether the buffer will be
>> null-terminated, so to be safe, we should do it ourselves.  Introduce
>> new function, xgethostname, which ensures that there is always a \0
>> at the end of the buffer.
>
> I think we should detect the error instead of truncating the hostname.
> That (on top of your patch) would look like the following.
>
> Thoughts?
> Jonathan
>
> diff --git i/wrapper.c w/wrapper.c
> index d837417709..e218bd3bef 100644
> --- i/wrapper.c
> +++ w/wrapper.c
> @@ -660,11 +660,13 @@ int xgethostname(char *buf, size_t len)
>  {
>  	/*
>  	 * If the full hostname doesn't fit in buf, POSIX does not
> -	 * specify whether the buffer will be null-terminated, so to
> -	 * be safe, do it ourselves.
> +	 * guarantee that an error will be returned. Check for ourselves
> +	 * to be safe.
>  	 */
>  	int ret = gethostname(buf, len);
> -	if (!ret)
> -		buf[len - 1] = 0;
> +	if (!ret && !memchr(buf, 0, len)) {
> +		errno = ENAMETOOLONG;
> +		return -1;
> +	}

Hmmmm.  "Does not specify if the buffer will be NUL-terminated"
would mean that it is OK for the platform gethostname() to stuff
sizeof(buf)-1 first bytes of the hostname in the buffer and then
truncate by placing '\0' at the end of the buf, and we would not
notice truncation with the above change on such a platform, no?

RE: [PATCH v3 2/2] xgethostname: handle long hostnames

[David Turner]

> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Tuesday, April 18, 2017 10:51 PM
> To: Jonathan Nieder <jrnieder@gmail.com>
> Cc: David Turner <David.Turner@twosigma.com>; git@vger.kernel.org;
> l.s.r@web.de
> Subject: Re: [PATCH v3 2/2] xgethostname: handle long hostnames
> 
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Hi,
> >
> > David Turner wrote:
> >
> >> If the full hostname doesn't fit in the buffer supplied to
> >> gethostname, POSIX does not specify whether the buffer will be
> >> null-terminated, so to be safe, we should do it ourselves.  Introduce
> >> new function, xgethostname, which ensures that there is always a \0
> >> at the end of the buffer.
> >
> > I think we should detect the error instead of truncating the hostname.
> > That (on top of your patch) would look like the following.
> >
> > Thoughts?
> > Jonathan
> >
> > diff --git i/wrapper.c w/wrapper.c
> > index d837417709..e218bd3bef 100644
> > --- i/wrapper.c
> > +++ w/wrapper.c
> > @@ -660,11 +660,13 @@ int xgethostname(char *buf, size_t len)  {
> >  	/*
> >  	 * If the full hostname doesn't fit in buf, POSIX does not
> > -	 * specify whether the buffer will be null-terminated, so to
> > -	 * be safe, do it ourselves.
> > +	 * guarantee that an error will be returned. Check for ourselves
> > +	 * to be safe.
> >  	 */
> >  	int ret = gethostname(buf, len);
> > -	if (!ret)
> > -		buf[len - 1] = 0;
> > +	if (!ret && !memchr(buf, 0, len)) {
> > +		errno = ENAMETOOLONG;
> > +		return -1;
> > +	}
> 
> Hmmmm.  "Does not specify if the buffer will be NUL-terminated"
> would mean that it is OK for the platform gethostname() to stuff
> sizeof(buf)-1 first bytes of the hostname in the buffer and then truncate by
> placing '\0' at the end of the buf, and we would not notice truncation with the
> above change on such a platform, no?

My read of the docs is that not only is that OK, but it is also permitted
for the platform to put sizeof(buf) bytes into the buffer and *not* 
put \0 at the end.

So in order to do a dynamic approach, we would have to allocate some
buffer, then run gethostname, then check if the penultimate element 
of the buffer was written to, and if so, allocate a larger buffer.  Yucky,
but possible.

Re: [PATCH v3 2/2] xgethostname: handle long hostnames

[René Scharfe]

Am 19.04.2017 um 17:50 schrieb David Turner:
>> -----Original Message-----
>> From: Junio C Hamano [mailto:gitster@pobox.com]
>> Sent: Tuesday, April 18, 2017 10:51 PM
>> To: Jonathan Nieder <jrnieder@gmail.com>
>> Cc: David Turner <David.Turner@twosigma.com>; git@vger.kernel.org;
>> l.s.r@web.de
>> Subject: Re: [PATCH v3 2/2] xgethostname: handle long hostnames
>>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> Hi,
>>>
>>> David Turner wrote:
>>>
>>>> If the full hostname doesn't fit in the buffer supplied to
>>>> gethostname, POSIX does not specify whether the buffer will be
>>>> null-terminated, so to be safe, we should do it ourselves.  Introduce
>>>> new function, xgethostname, which ensures that there is always a \0
>>>> at the end of the buffer.
>>>
>>> I think we should detect the error instead of truncating the hostname.
>>> That (on top of your patch) would look like the following.
>>>
>>> Thoughts?
>>> Jonathan
>>>
>>> diff --git i/wrapper.c w/wrapper.c
>>> index d837417709..e218bd3bef 100644
>>> --- i/wrapper.c
>>> +++ w/wrapper.c
>>> @@ -660,11 +660,13 @@ int xgethostname(char *buf, size_t len)  {
>>>   	/*
>>>   	 * If the full hostname doesn't fit in buf, POSIX does not
>>> -	 * specify whether the buffer will be null-terminated, so to
>>> -	 * be safe, do it ourselves.
>>> +	 * guarantee that an error will be returned. Check for ourselves
>>> +	 * to be safe.
>>>   	 */
>>>   	int ret = gethostname(buf, len);
>>> -	if (!ret)
>>> -		buf[len - 1] = 0;
>>> +	if (!ret && !memchr(buf, 0, len)) {
>>> +		errno = ENAMETOOLONG;
>>> +		return -1;
>>> +	}
>>
>> Hmmmm.  "Does not specify if the buffer will be NUL-terminated"
>> would mean that it is OK for the platform gethostname() to stuff
>> sizeof(buf)-1 first bytes of the hostname in the buffer and then truncate by
>> placing '\0' at the end of the buf, and we would not notice truncation with the
>> above change on such a platform, no?
> 
> My read of the docs is that not only is that OK, but it is also permitted
> for the platform to put sizeof(buf) bytes into the buffer and *not*
> put \0 at the end.

That sounds crazy, but that's how I read the spec [1] as well.  And
POSIX also doesn't specify any errors for gethostname.  But that
makes kinda sense because it *does* specify HOST_NAME_MAX as maximum
size.  Things get more interesting when this spec meets systems that
don't have HOST_NAME_MAX, or error returns, or bugs.

> So in order to do a dynamic approach, we would have to allocate some
> buffer, then run gethostname, then check if the penultimate element
> of the buffer was written to, and if so, allocate a larger buffer.  Yucky,
> but possible.

That's what the gnulib version of xgethostname does [2], among
other things.

The more I read about gethostname and its weirdness, the more I
think we should import an existing, proven version of xgethostname
that returns an allocated buffer.  That way we wouldn't have to
worry about truncation or missing NULs or buffer sizes anymore.
What do you think?

I found the one from gnulib and from Neal Walfield [3] mentioned
in the Hurd docs; are there more?

René


[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/gethostname.html
[2] http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/xgethostname.c;hb=0632e115747ff96e93330c88f536d7354a7ce507
[3] http://walfield.org/pub/people/neal/xgethostname/
[4] https://www.gnu.org/software/hurd/hurd/porting/guidelines.html#MAXHOSTNAMELEN_tt_

Re: [PATCH v3 2/2] xgethostname: handle long hostnames

[Junio C Hamano]

David Turner <dturner@twosigma.com> writes:

> If the full hostname doesn't fit in the buffer supplied to
> gethostname, POSIX does not specify whether the buffer will be
> null-terminated, so to be safe, we should do it ourselves.  Introduce

The name of the character whose ASCII value is '\0' is NUL, not
null (similarly for in-code comment).