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

Fri Apr 27 16:14:41 UTC 2018

Hi Christoph,

On Fri, Apr 27, 2018 at 9:35 AM, Langer, Christoph
<christoph.langer at sap.com> wrote:
> Hi all,
>
> thanks for looking into this. Here are a few comments
>
> First of all, there are no real life issues I have seen with this. It is just something that occurred to me when working with the code. But, why not fix it, even it is a corner case that might never happen.
>
> @Thomas: As for the zero termination of the hostname result after the call to gethostname: Yes, we should unconditionally terminate the result, which we do. Unfortunately this part of code cannot be moved outside the solaris #ifdef because the part in the #ifdef contains variable declarations. And you know - the C compatibility issue...
>

Ok.

> I looked again into the macro definitions for for HOST_NAME_MAX and NI_MAXHOST. HOST_NAME_MAX is mentioned in the gethostname docs ([1] and [2]). Glibc docs indicate it is 64 Byte or 255 Byte. So it looks like it is a quite small buffer, compared to NI_MAXHOST from netdb.h, which is the value that getnameinfo likes to work with, as per [3]. Posix genameinfo doc ([4]) does not mention NI_MAXHOST but Linux doc says it is 1025 which is what we'd define if it is not set.
>

Okay, thanks for the research! This is weird, why two different
defines for the same thing.

The only (probably highly theoretical) problem I see is that there may
be platforms which do not define NI_MAXHOST but where HOST_MAX_NAME is
defined and larger than 1025 char sized output buffers. Then, we would
artificially limit ourselves to 1025 characters. (Was Matthias not
working on a problem with truncated host names in our hpux port?).

One could in theory solve this by falling back to HOST_MAX_NAME if
NI_MAXHOST is undefined:

#ifdnef NI_MAXHOST
#ifdef HOST_MAX_NAME
#define NI_MAXHOST HOST_MAX_NAME
#else
#define NI_MAXHOST 1025
#endif
#endif

However, I am fine with your current patch. I think redefinition of
NI_MAXHOST - if even necessary - should be done in a follow up issue.

> Taking this input I have updated my webrev to round things up a little bit: http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/
>
> I moved the definition of NI_MAXHOST into net_util_md.h and updated the comment a little bit to make clearer why it is there. In Inet4AddressImpl.c and Inet6AddressImpl.c I also fixed the other places where getnameinfo is called to use sizeof(buffer) instead of NI_MAXHOST.
>

All looks well. Again, thanks for the research.

... Thomas

> Best regards
> Christoph
>
> [1] http://man7.org/linux/man-pages/man2/gethostname.2.html
> [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/gethostname.html
> [3] http://man7.org/linux/man-pages/man3/getnameinfo.3.html
> [4] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getnameinfo.html
>
>
>> -----Original Message-----
>> From: vyom tewari [mailto:vyom.tewari at oracle.com]
>> Sent: Freitag, 27. April 2018 08:38
>> To: Thomas Stüfe <thomas.stuefe at gmail.com>
>> Cc: Langer, Christoph <christoph.langer at sap.com>; net-
>> dev at openjdk.java.net
>> Subject: Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in
>> Unix Inet*AddressImpl_getLocalHostName implementations
>>
>>
>>
>> On Friday 27 April 2018 10:58 AM, Thomas Stüfe wrote:
>> > 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.
>> Even i noticed this, why we use our own NI_MAXHOST instead
>> 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?
>>   :) Agreed, as i said Christoph change is logically correct  but i
>> don't know the history behind current code, so just wanted to be sure
>> that  we are not missing any corner case.
>>
>> Thanks,
>> Vyom
>>
>> >
>> > Thanks, Thomas
>> >
>> >> Thanks,
>> >> Vyom
>> >>
>> >> Best regards
>> >> Christoph
>> >>
>> >>
>