Hi Christoph, On Fri, Apr 27, 2018 at 9:35 AM, Langer, Christoph <christoph.lan...@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.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 >> >> >> >> >
