Re: [PATCH] xgethostname: handle long hostnames
Am 13.04.2017 um 21:23 schrieb 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
> ---
> 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
RE: [PATCH] xgethostname: handle long hostnames
> -Original Message-
> From: Jonathan Nieder [mailto:jrnie...@gmail.com]
> Sent: Thursday, April 13, 2017 6:05 PM
> To: David Turner
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] xgethostname: handle long hostnames
>
> 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.
> [...]
> > +++ 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;
>
> I wonder if after null-terminating we would want to report this as an error,
> instead of silently using a truncated result. I.e. something like
>
> > + if (!ret)
> > + buf[len - 1] = 0;
> > + if (strlen(buf) >= len - 1) {
> > + errno = ENAMETOOLONG;
> > + return -1;
> > + }
>
> (or EINVAL --- either is equally descriptive).
Looking at the users of this function, I think most would be happier with a
truncated buffer than an error:
gc.c: used to see if we are the same machine as the machine that locked the
repo. Unlikely that two machines have hostnames that differ only in the
256th-or-above character.
fetch-pack.c, receive-pack.c: similar to gc.c; the hostname is a note in the
.keep file
Ident.c: used to make up a fake email address. On my laptop, gethostname
returns "corey" (no domain part), so the email address is not likely to be
valid anyway.
> Also POSIX requires that hostnames are <= 255 bytes. Maybe we can force the
> buffer to be large enough.
That is now how I read it. I read the limit as HOST_NAME_MAX, which has a
*minimum* value of 255, but which might be larger.
The existing hostname buffers are 128, 256, and 1024 bytes, so they're pretty
arbitrary.
Re: [PATCH] xgethostname: handle long hostnames
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.
[...]
> +++ 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;
I wonder if after null-terminating we would want to report this as
an error, instead of silently using a truncated result. I.e. something
like
> + if (!ret)
> + buf[len - 1] = 0;
> + if (strlen(buf) >= len - 1) {
> + errno = ENAMETOOLONG;
> + return -1;
> + }
(or EINVAL --- either is equally descriptive).
Also POSIX requires that hostnames are <= 255 bytes. Maybe we can
force the buffer to be large enough.
Thoughts?
Jonathan
Re: [PATCH] xgethostname: handle long hostnames
On Thu, Apr 13, 2017 at 03:23:35PM -0400, 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 Wow, TIL. What an utterly terrible and error-prone interface (I always just assumed we'd get ENAMETOOLONG, which is what glibc does). > 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. Your patch looks good to me. -Peff
