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

[vyom tewari]

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