Re: [PATCH] xgethostname: handle long hostnames

From: René Scharfe <l.s.r@web.de>
Date: Apr 13, 2017, 22:32 UTC
Message-ID: <aa10d2cb-c21a-fbfd-9fae-0d0153e4901b@web.de>
In-Reply-To: <20170413192335.20679-1-dturner@twosigma.com>

Am 13.04.2017 um 21:23 schrieb David Turner:

Show 8 quoted lines

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

Show 21 quoted lines

> 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;
> +}

Silent truncation is not ideal, no matter if it's done by the wrapper or the original function. It would be better to use a properly sized buffer.

POSIX requires hostnames to have a maximum length of HOST_NAME_MAX. So how about just adding an assert to make sure len is big enough? Or evaluate the condition at compile time with BUILD_ASSERT_OR_ZERO?

Downside: Not all platforms define HOST_NAME_MAX.  daemon.c uses 256 as
a fallback.  On Windows a buffer size of 256 is documented to suffice
in all cases [1].  The Linux manpage [2] mentions a hostname length
limit of 255 (plus NUL) as well, even though HOST_NAME_MAX is 64 there.

Another possibility: Die (or at least warn) if the buffer doesn't contain a NUL byte after calling gethostname(). That only works for platforms that don't NUL-terminate on truncation, though, so silent truncation would still go unnoticed.

Anyway, the buffer in builtin/gc.c with its 128 bytes seems to be too short; the others are at least 256 bytes long. Replacing the magic buffer size number with HOST_NAME_MAX + 1 might be a good idea (after moving the fallback definition to git-compat-util.h).

René

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms738527(v=vs.85).aspx [2] http://man7.org/linux/man-pages/man2/gethostname.2.html