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