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

Mon Apr 30 08:00:55 UTC 2018

Hi Thomas,

thanks for reviewing.

The test has also passed our internal testing. I'll run it through jdk-submit and then push it in the course of the day.

Best regards
Christoph

> -----Original Message-----
> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> Sent: Freitag, 27. April 2018 18:15
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: vyom tewari <vyom.tewari at oracle.com>; net-dev at openjdk.java.net;
> Brian Burkhalter <brian.burkhalter at oracle.com>
> Subject: Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in
> Unix Inet*AddressImpl_getLocalHostName implementations
> 
> 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
> >> >>
> >> >>
> >