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

[Chris Hegarty]

On 30/04/18 10:44, Chris Hegarty wrote:


On 30/04/18 09:00, Langer, Christoph wrote:

...
Taking this input I have updated my webrev to round things up a 
little bit:

http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/

Thank you Christophe, this latest version looks good.
I'll run some tests on it and report the results by tomorrow.


Results of my testing are positive. Reviewed.

-Chris.

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

[Chris Hegarty]

On 30/04/18 11:07, Langer, Christoph wrote:

Hi Chris,

thanks for looking at this. I just received positive confirmation from 
jdk-submit. Our internal tests did not show regressions as well. So I'll wait 
for word on your tests before submitting.


Do you have a link to the jdk-submit job, or anything
suitable for forwarding? I'm curious what test sets it ran.

-Chris.



Thanks
Christoph


-Original Message-
From: Chris Hegarty [mailto:chris.hega...@oracle.com]
Sent: Montag, 30. April 2018 11:45
To: Langer, Christoph ; Thomas Stüfe

Cc: net-dev@openjdk.java.net
Subject: Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in
Unix Inet*AddressImpl_getLocalHostName implementations


On 30/04/18 09:00, Langer, Christoph wrote:

...

Taking this input I have updated my webrev to round things up a little

bit:

http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/

Thank you Christophe, this latest version looks good.
I'll run some tests on it and report the results by tomorrow.

-Chris.

P.S. I do remember adding the unconditional null termination
to this code years ago, but I suspect that it's been touched
a few times since.

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

[Langer, Christoph]

Hi Chris,

thanks for looking at this. I just received positive confirmation from 
jdk-submit. Our internal tests did not show regressions as well. So I'll wait 
for word on your tests before submitting.

Thanks
Christoph

> -Original Message-
> From: Chris Hegarty [mailto:chris.hega...@oracle.com]
> Sent: Montag, 30. April 2018 11:45
> To: Langer, Christoph ; Thomas Stüfe
> 
> Cc: net-dev@openjdk.java.net
> Subject: Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in
> Unix Inet*AddressImpl_getLocalHostName implementations
> 
> 
> On 30/04/18 09:00, Langer, Christoph wrote:
> > ...
> >>> Taking this input I have updated my webrev to round things up a little
> bit:
> >> http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/
> Thank you Christophe, this latest version looks good.
> I'll run some tests on it and report the results by tomorrow.
> 
> -Chris.
> 
> P.S. I do remember adding the unconditional null termination
> to this code years ago, but I suspect that it's been touched
> a few times since.

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

[Chris Hegarty]

On 30/04/18 09:00, Langer, Christoph wrote:

...

Taking this input I have updated my webrev to round things up a little bit:

http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/

Thank you Christophe, this latest version looks good.
I'll run some tests on it and report the results by tomorrow.

-Chris.

P.S. I do remember adding the unconditional null termination
to this code years ago, but I suspect that it's been touched
a few times since.

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

[Langer, Christoph]

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.stu...@gmail.com]
> Sent: Freitag, 27. April 2018 18:15
> To: Langer, Christoph 
> Cc: vyom tewari ; net-dev@openjdk.java.net;
> Brian Burkhalter 
> 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
>  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 
> >> Cc: Langer, Christoph ; 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
> 
> >> 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 i

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

[Thomas Stüfe]

Hi Christoph,

On Fri, Apr 27, 2018 at 9:35 AM, 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...
>

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 
>> Cc: Langer, Christoph ; 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 
>> 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.
>> >>
>> >> #

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

[vyom tewari]

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 
Cc: Langer, Christoph ; 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 

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



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.



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 

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

[Langer, Christoph]

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 
> Cc: Langer, Christoph ; 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 
> 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
> >
> > 
> >
> > 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 arg

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



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.



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

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

[Thomas Stüfe]

On Fri, Apr 27, 2018 at 5:57 AM, vyom tewari  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



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.



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.

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

Thanks, Thomas

> Thanks,
> Vyom
>
> Best regards
> Christoph
>
>

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

[Thomas Stüfe]

On Fri, Apr 27, 2018 at 7:02 AM, Thomas Stüfe  wrote:
> Hi Christoph,
>
> code change makes sense. +1 on the "sizeof()" arg change.
>
> I would prefer it though if you would move the zero-termination out of
> the if/else altogether.

Disregard this last comment. What I meant was I would probably
unconditionally zero-terminate the result string before calling
NewStringUTF() but I guess this is not needed.

..Thomas

>
> Best Regards, Thomas
>
>
>
> On Tue, Apr 24, 2018 at 11:38 AM, Langer, Christoph
>  wrote:
>> Hi,
>>
>>
>>
>> please help reviewing a small change that I stumbled over when looking into
>> the getLocalHostName implementation. I found that the length of the hostname
>> buffer is not correctly passed to sub functions. The buffer size is
>> specified as “NI_MAXHOST + 1”, so this size should be handed down to
>> gethostname() and getnameinfo() calls, not just NI_MAXHOST. I also moved the
>> solaris #ifdefs into the else clause to spare a few lines of code.
>>
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202181
>>
>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8202181.0/
>>
>>
>>
>> Thanks
>>
>> Christoph
>>
>>

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

[Thomas Stüfe]

Hi Christoph,

code change makes sense. +1 on the "sizeof()" arg change.

I would prefer it though if you would move the zero-termination out of
the if/else altogether.

Best Regards, Thomas



On Tue, Apr 24, 2018 at 11:38 AM, Langer, Christoph
 wrote:
> Hi,
>
>
>
> please help reviewing a small change that I stumbled over when looking into
> the getLocalHostName implementation. I found that the length of the hostname
> buffer is not correctly passed to sub functions. The buffer size is
> specified as “NI_MAXHOST + 1”, so this size should be handed down to
> gethostname() and getnameinfo() calls, not just NI_MAXHOST. I also moved the
> solaris #ifdefs into the else clause to spare a few lines of code.
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8202181
>
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8202181.0/
>
>
>
> Thanks
>
> Christoph
>
>

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

[vyom tewari]

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


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.

Thanks,
Vyom



Best regards
Christoph

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

[Brian Burkhalter]

Hi Christoph,

This looks OK to me but probably a net-dev engineer should also comment. The 
bug needs a noreg label, e.g., noreg-cleanup.

Brian

On Apr 26, 2018, at 11:56 AM, Langer, Christoph  
wrote:

> Ping, can some reviewer please have a look at this small fix?
>  […]
>  
> Bug: https://bugs.openjdk.java.net/browse/JDK-8202181
> Webrev:http://cr.openjdk.java.net/~clanger/webrevs/8202181.0/

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

[Langer, Christoph]

Ping, can some reviewer please have a look at this small fix?

Thanks
Christoph

From: Langer, Christoph
Sent: Dienstag, 24. April 2018 11:39
To: net-dev@openjdk.java.net
Subject: RFR(XS): 8202181: Correctly specify size of hostname buffer in Unix 
Inet*AddressImpl_getLocalHostName implementations

Hi,

please help reviewing a small change that I stumbled over when looking into the 
getLocalHostName implementation. I found that the length of the hostname buffer 
is not correctly passed to sub functions. The buffer size is specified as 
"NI_MAXHOST + 1", so this size should be handed down to gethostname() and 
getnameinfo() calls, not just NI_MAXHOST. I also moved the solaris #ifdefs into 
the else clause to spare a few lines of code.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202181
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8202181.0/

Thanks
Christoph

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

[Langer, Christoph]

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.

Best regards
Christoph

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

[vyom tewari]

On Tuesday 24 April 2018 03:08 PM, Langer, Christoph wrote:


Hi,

please help reviewing a small change that I stumbled over when looking 
into the getLocalHostName implementation. I found that the length of 
the hostname buffer is not correctly passed to sub functions. The 
buffer size is specified as “NI_MAXHOST + 1”, so this size should be 
handed down to gethostname() and getnameinfo() calls, not just 
NI_MAXHOST. I also moved the solaris #ifdefs into the else clause to 
spare a few lines of code.


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 ?

Thanks,
Vyom


Bug: https://bugs.openjdk.java.net/browse/JDK-8202181 



Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8202181.0/ 



Thanks

Christoph

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

[Langer, Christoph]

Hi,

please help reviewing a small change that I stumbled over when looking into the 
getLocalHostName implementation. I found that the length of the hostname buffer 
is not correctly passed to sub functions. The buffer size is specified as 
"NI_MAXHOST + 1", so this size should be handed down to gethostname() and 
getnameinfo() calls, not just NI_MAXHOST. I also moved the solaris #ifdefs into 
the else clause to spare a few lines of code.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202181
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8202181.0/

Thanks
Christoph