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/ 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. 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.tew...@oracle.com] > Sent: Freitag, 27. April 2018 08:38 > To: Thomas Stüfe <thomas.stu...@gmail.com> > Cc: Langer, Christoph <christoph.lan...@sap.com>; net- > d...@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.tew...@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 > >> > >>
