RFR(XS): 8202181: Correctly specify size of hostname buffer in Unix Inet*AddressImpl_getLocalHostName implementations

[Thomas Stüfe]

On Fri, Apr 27, 2018 at 5:57 AM, vyom tewari <vyom.tewari at oracle.com> wrote:
> Hi Christoph,
>
>
> On Tuesday 24 April 2018 04:45 PM, Langer, Christoph wrote:
>
> Hi Vyom,
>
> I think, it is intentional to handle case where return "hostname" is to
> large to
> fit in  array.  if you see the man page(http://man7.org/linux/man-
> pages/man2/gethostname.2.html) it says that it is unspecified whether
> returned buffer includes a terminating null byte.
>
> current code will put null in case of large "hostname", What do you think ?
>
> yes, I had read the man page and saw this point of the spec. But exactly for
> this purpose there's this code:
>
> // make sure string is null-terminated
> hostname[NI_MAXHOST] = '\0';
>
> If we only hand 'NI_MAXHOST' as size value into gethostname, then the
> function might only write NI_MAXHOST - 1 characters of the hostname into the
> buffer.
>
> doc says it will copy len bytes into buffer and will not copy null character
> into the buffer.
>
> ################################
>
> C library/kernel differences
>        The GNU C library does not employ the gethostname() system call;
>        instead, it implements gethostname() as a library function that calls
>        uname(2) and copies up to len bytes from the returned nodename field
>        into name.  Having performed the copy, the function then checks if
>        the length of the nodename was greater than or equal to len, and if
>        it is, then the function returns -1 with errno set to ENAMETOOLONG;
>        in this case, a terminating null byte is not included in the returned
>        name.
> ############################################################
>

This is shared code, so we should refer to Posix, not linux specific man pages.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html

<quote>

DESCRIPTION

The gethostname() function shall return the standard host name for the
current machine. The namelen argument shall specify the size of the
array pointed to by the name argument. The returned name shall be
null-terminated, except that if namelen is an insufficient length to
hold the host name, then the returned name shall be truncated and it
is unspecified whether the returned name is null-terminated.

Host names are limited to {HOST_NAME_MAX} bytes.

RETURN VALUE

Upon successful completion, 0 shall be returned; otherwise, -1 shall
be returned.

</quote>

Note that there is no indication what happens if the buffer is too
small. It may zero-terminate, it may not. It may return an error, it
may not. Decision is left to the platform implementors.

So from that, I would pass in a large-enough buffer and always
zero-terminate on success. According to Posix, a large-enough buffer
means HOST_NAME_MAX bytes.

I do not understand why we use NI_MAXHOST instead (and we we define it
to an arbitrary 1025 byte if undefined). Were there platforms where
HOST_NAME_MAX was too short? If yes, one should at least check that
NI_MAXHOST >= HOST_NAME_MAX.

> Just for curiosity, are we facing any issues with the current code ?. Your
> code change looks logical but if nothing is broken then why to change code.
>

If it can be proven by looking at the API description that it is
broken for some corner case, why keep it broken?

Thanks, Thomas

> Thanks,
> Vyom
>
> Best regards
> Christoph
>
>