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

Fri Apr 27 08:40:33 UTC 2018


On Friday 27 April 2018 01:05 PM, Langer, Christoph 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...
>
> 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.
>
> Taking this input I have updated my webrev to round things up a little bit: http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/
it looks good to me, although i am not official reviewer.
> 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.
good cleanup, comment will definitely help.

Thanks,
Vyom
>
> 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
>>>>
>>>>