Re: java.net.Socket should report the attempted address and port

[Péter Gergely Horváth]

Hi Tobias,

Thank you for pointing me to that thread: it's good to have that context
(it was sent before I joined the mailing list, so please bear with me).

I understand the JDK developers want to be safe than sorry around reporting
target addresses and I absolutely agree with that point.

However considering how useful it would be to have this _optionally_ for
debugging, I am wondering if it would be possible to have a dedicated Java
system property defined for this (e.g.
'java.net.socket.reportAddressInException' or something like that), which
would enable this behaviour (retaining the current behaviour of *not
reporting anything by default.*).

What do you think about this, guys? With this in place both the
secure-by-default requirement would be met, and there would be a powerful
tool available to fight the horrors of debugging a running complex
distributed application from its logs.

Thanks,
Peter





On Sun, Apr 22, 2018 at 10:21 PM, James Roper  wrote:

> This would be especially useful in asynchronous applications - since in
> those cases the exception rarely maps back to a place in user code that
> would indicate what is being connected to. As someone who has spent a lot
> of time supporting developers who use asynchronous libraries and post
> exceptions of this nature (supporting both in open source, eg on stack
> overflow, as well as providing commercial support), where I don't have
> access to their code base so I can't do the necessary investigations
> directly myself, having the attempted address and port in the error message
> would save a lot of time, and probably even prevent a lot of people from
> requiring support in the first place.
>
> On 22 April 2018 at 20:59, Péter Gergely Horváth <
> peter.gergely.horv...@gmail.com> wrote:
>
>> Hi All,
>>
>> I am wondering if it would be possible to make a minor improvement to the
>> way *java.net.Socket* reports connectivity errors and incorporate the
>> attempted address, port and the timeout used into the exception message.
>>
>> The current implementation emits a generic error message, which is not
>> that helpful when one is operating a _large_ application. (Consider e.g.
>> Big Data or complex legacy systems written by someone else).
>>
>> java.net.ConnectException: Connection refused (Connection refused)
>> at java.net.PlainSocketImpl.socketConnect(Native Method)
>> at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSock
>> etImpl.java:350)
>> at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPl
>> ainSocketImpl.java:206)
>> at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocket
>> Impl.java:188)
>> at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
>> at java.net.Socket.connect(Socket.java:589)
>> at java.net.Socket.connect(Socket.java:538)
>> at java.net.Socket.(Socket.java:434)
>> at java.net.Socket.(Socket.java:211)
>> at Sample.main(Sample.java:9)
>>
>>
>> I have looked into the JDK code base and implemented a patch that reports
>> the address, port and timeout used in the error message without touching
>> any native parts (see attached patch file). I have tested this (created my
>> own build of the JDK and run a sample application against it) and it seems
>> to work properly.
>>
>> Would it be possible to incorporate this change into the official JDK
>> code base? I do believe it would help a lot of people out there.
>>
>> Based on my understanding, once I have signed the OCA, I should simply
>> write an email to the group and request a sponsor to pick up this issue.
>> Could someone help me with this?
>>
>> Thank you,
>> Peter
>>
>>
>>
>>
>>
>>
>>
>>
>
>
> --
> *James Roper*
> *Senior Octonaut*
>
> Lightbend  – Build reactive apps!
> Twitter: @jroper 
>

Re: RFR 8202091 : Rename DualStackPlainSocketImpl to PlainSocketImpl [win]

[Chris Hegarty]

On 20/04/18 21:40, Ivan Gerasimov wrote:

The correct links to the Bug and webrev are:

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202091
WEBREV: http://cr.openjdk.java.net/~igerasim/8202091/00/webrev/


This looks good. Thanks Ivan.

-Chris.

Re: [PATCH] JDK-8201545 Clarify the return value of InetAddress.getByName/getAllName for empty host value

[Chris Hegarty]

Thanks Jaikiran,

I think your patch looks good. I filed the following CSR to track
the Java SE API ( javadoc ) change.

https://bugs.openjdk.java.net/browse/JDK-8202139

Once approved, I can sponsor this for you.

-Chris.

On 20/04/18 11:08, Jaikiran Pai wrote:

Hi,

The attached patch addresses the issue noted in [1], by updating the 
javadoc of InetAddress.getByName and InetAddress.getAllByName to clarify 
that these methods return a loopback address, if the host parameter is 
an empty string (same behaviour as host == null). The patch also updates 
an existing test case to test these methods for both null and empty 
parameter values.


After looking at existing tests for InetAddress, I felt the existing 
GetLoopbackAddress.java test case is closest to what we are testing 
here. One thing I need input on, for the GetLoopbackAddress.java test 
class, is the value of @summary. Should I update it to include a summary 
of this new test too, or should I remove it altogether? I have anyway 
updated the @bug to include the JIRA id of this issue. I am open to 
creating a fresh new test case class just for this issue, if that's better.


[1] https://bugs.openjdk.java.net/browse/JDK-8201545

-Jaikiran

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.


Thanks,

Vyom


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : 
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html


Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
   and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
   ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
   system dependent
6) Added typical values. I think this is useful, as it
   gives an idea to the developer, but maybe it could be
   misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

---

    /**
 * Keep-Alive idle time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. The default value for this idle 
period is
 * system dependent, but is typically 2 hours. The {@code 
TCP_KEEPIDLE}

 * option can be used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds of idle time before keep-alive initiates a 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 *  @since 11
 */
    public static final SocketOption TCP_KEEPIDLE
    = new ExtSocketOption("TCP_KEEPIDLE", 
Integer.class);


    /**
 * Keep-Alive retransmission interval time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe after some amount 
of time.
 * The default value for this retransmition interval is system 
dependent,
 * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL} 
option can be

 * used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds to wait before retransmitting a keep-alive 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPINTERVAL
    = new ExtSocketOption("TCP_KEEPINTERVAL", 
Integer.class);


    /**
 * Keep-Alive retransmission maximum limit.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe a certain number of 
times
 * before a connection is considered to be broken. The default 
value for

 * this keep-alive probe retransmit limit is system dependent, but is
 * typically 8. The {@code TCP_KEEPCOUNT} option can be used to 
affect

 * this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * maximum number of keep-alive probes to be sent. The socket 
option is
 * specific to stream-oriented sockets using the TCP/IP protocol. 
The exact
 * semantics of this socket option are socket type and system 
dependent.

 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPCOUNT
    = new ExtSocketOption("TCP_KEEPCOUNT", 
Integer.class);


-Chris.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[Alan Bateman]

On 23/04/2018 13:05, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.
This looks much better but I think the second paragraph of the spec of 
each option needs to be inverted so that it states clearly what the 
options does before it gets into the background of SO_KEEPALIVE. For 
example, TCP_KEEPALIVE should say that it sets the idle period for keep 
alive before it explains SO_KEEPALIVE. The currently wording also begs 
questions as to whether the socket option means anything when 
SO_KEEPALIVE is not enabled. Also it probably should say something about 
whether it can be changed at any time or not.


The implementation looks much better but the filtering by socket option 
name is still a bit fragile. It might be better for 
ExtendedSocketOptions to mainain 3 sets of options, one for SOCK_UNSPEC, 
one for SOCK_STREAM, the other for SOCK_DGRAM. A map would work too. The 
register method can specify socket type and it's very obvious which 
sockets the option is intended to be used for.


All usages are now ExtendedSocketOptions.getInstance().options(...). 
Have you considered a static options(...) method to reduce the typing at 
each of the usage sites?


-Alan

[8u] RFR(XS): 8201369: Inet4AddressImpl_getLocalHostName reverse lookup on Solaris only

[Langer, Christoph]

Hi,

I created a JDK 8 backport proposal for JDK-8201369: 
http://cr.openjdk.java.net/~clanger/webrevs/8201369-jdk8.0/
This is the Bug: https://bugs.openjdk.java.net/browse/JDK-8201369
The issue was recently discussed and reviewed here: 
http://mail.openjdk.java.net/pipermail/net-dev/2018-April/011322.html

Apart from limiting the getaddrinfo/getnameinfo lookup in 
Java_java_net_Inet4AddressImpl_getLocalHostName to Solaris only, I took over 
the formatting cleanups to make both Ipv4 and IPv6 versions look similar and 
resemble the layout in the current jdk depot.

Please help reviewing this  for JDK 8.

Thanks & Best regards
Christoph

Re: java.net.Socket should report the attempted address and port

[Michael McMahon]

I agree we should do something about this. I will make some enquiries with
the security folks here as to what might be permitted. I suspect either 
some kind
of debugging property/switch to enable it, or the limited information 
only being

provided when a security manager is enabled, might work.

I will get back with a firm proposal.

Thanks,

Michael.

On 23/04/2018, 10:05, Péter Gergely Horváth wrote:

Hi Tobias,

Thank you for pointing me to that thread: it's good to have that 
context (it was sent before I joined the mailing list, so please bear 
with me).


I understand the JDK developers want to be safe than sorry around 
reporting target addresses and I absolutely agree with that point.


However considering how useful it would be to have this _optionally_ 
for debugging, I am wondering if it would be possible to have a 
dedicated Java system property defined for this (e.g. 
'java.net.socket.reportAddressInException' or something like that), 
which would enable this behaviour (retaining the current behaviour of 
*not reporting anything by default.*).


What do you think about this, guys? With this in place both the 
secure-by-default requirement would be met, and there would be a 
powerful tool available to fight the horrors of debugging a running 
complex distributed application from its logs.


Thanks,
Peter





On Sun, Apr 22, 2018 at 10:21 PM, James Roper > wrote:


This would be especially useful in asynchronous applications -
since in those cases the exception rarely maps back to a place in
user code that would indicate what is being connected to. As
someone who has spent a lot of time supporting developers who use
asynchronous libraries and post exceptions of this nature
(supporting both in open source, eg on stack overflow, as well as
providing commercial support), where I don't have access to their
code base so I can't do the necessary investigations directly
myself, having the attempted address and port in the error message
would save a lot of time, and probably even prevent a lot of
people from requiring support in the first place.

On 22 April 2018 at 20:59, Péter Gergely Horváth
mailto:peter.gergely.horv...@gmail.com>> wrote:

Hi All,

I am wondering if it would be possible to make a minor
improvement to the way *java.net.Socket* reports connectivity
errors and incorporate the attempted address, port and the
timeout used into the exception message.

The current implementation emits a generic error message,
which is not that helpful when one is operating a _large_
application. (Consider e.g. Big Data or complex legacy systems
written by someone else).

java.net.ConnectException: Connection refused (Connection refused)
at java.net.PlainSocketImpl.socketConnect(Native Method)
at java.net

.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:350)
at java.net

.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:206)
at java.net

.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:188)
at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
at java.net.Socket.connect(Socket.java:589)
at java.net.Socket.connect(Socket.java:538)
at java.net.Socket.(Socket.java:434)
at java.net.Socket.(Socket.java:211)
at Sample.main(Sample.java:9)


I have looked into the JDK code base and implemented a patch
that reports the address, port and timeout used in the error
message without touching any native parts (see attached patch
file). I have tested this (created my own build of the JDK and
run a sample application against it) and it seems to work
properly.

Would it be possible to incorporate this change into the
official JDK code base? I do believe it would help a lot of
people out there.

Based on my understanding, once I have signed the OCA, I
should simply write an email to the group and request
a sponsor to pick up this issue. Could someone help me with this?

Thank you,
Peter










-- 
*James Roper*

/Senior Octonaut/

Lightbend  – Build reactive apps!
Twitter: @jroper 

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[Chris Hegarty]

On 23/04/18 13:34, Alan Bateman wrote:

On 23/04/2018 13:05, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.
This looks much better but I think the second paragraph of the spec of 
each option needs to be inverted so that it states clearly what the 
options does before it gets into the background of SO_KEEPALIVE. For 
example, TCP_KEEPALIVE should say that it sets the idle period for keep 
alive before it explains SO_KEEPALIVE. 


Mea culpa, I ordered the paragraphs as they are to be consistent
with TCP_QUICKACK. I don't have any objection if they are reversed.

The currently wording also begs 
questions as to whether the socket option means anything when 
SO_KEEPALIVE is not enabled.


It's implicit. The option can still be set and its value retrieved.


Also it probably should say something about 
whether it can be changed at any time or not.


You can set it any time. Of course, it may not be effective
immediately, depending on the exact state of the socket.

We may also want to add a note that the accepted values may
have an upper-bound. For example, the following is the largest
set of values that I can set on my Ubuntu Linux, without an
exception being thrown [*].

 TCP_KEEPIDLE = 32767
 TCP_KEEPINTERVAL = 32767
 TCP_KEEPCOUNT = 127

-Chris.

[*] "java.net.SocketException: Invalid argument" when a given
value is "too" large.

Re: [8u] RFR(XS): 8201369: Inet4AddressImpl_getLocalHostName reverse lookup on Solaris only

[Chris Hegarty]

On 23/04/18 13:49, Langer, Christoph wrote:

Hi,

I created a JDK 8 backport proposal for JDK-8201369: 
http://cr.openjdk.java.net/~clanger/webrevs/8201369-jdk8.0/


This is the Bug: https://bugs.openjdk.java.net/browse/JDK-8201369

The issue was recently discussed and reviewed here: 
http://mail.openjdk.java.net/pipermail/net-dev/2018-April/011322.html


Apart from limiting the getaddrinfo/getnameinfo lookup in 
Java_java_net_Inet4AddressImpl_getLocalHostName to Solaris only, I took 
over the formatting cleanups to make both Ipv4 and IPv6 versions look 
similar and resemble the layout in the current jdk depot.


Please help reviewing this  for JDK 8.


The code changes look ok to me.

-Chris.

Re: [PATCH] JDK-8201545 Clarify the return value of InetAddress.getByName/getAllName for empty host value

[Jaikiran Pai]

Thank you Chris.

-Jaikiran


On 23/04/18 5:03 PM, Chris Hegarty wrote:

Thanks Jaikiran,

I think your patch looks good. I filed the following CSR to track
the Java SE API ( javadoc ) change.

https://bugs.openjdk.java.net/browse/JDK-8202139

Once approved, I can sponsor this for you.

-Chris.

On 20/04/18 11:08, Jaikiran Pai wrote:

Hi,

The attached patch addresses the issue noted in [1], by updating the 
javadoc of InetAddress.getByName and InetAddress.getAllByName to 
clarify that these methods return a loopback address, if the host 
parameter is an empty string (same behaviour as host == null). The 
patch also updates an existing test case to test these methods for 
both null and empty parameter values.


After looking at existing tests for InetAddress, I felt the existing 
GetLoopbackAddress.java test case is closest to what we are testing 
here. One thing I need input on, for the GetLoopbackAddress.java test 
class, is the value of @summary. Should I update it to include a 
summary of this new test too, or should I remove it altogether? I 
have anyway updated the @bug to include the JIRA id of this issue. I 
am open to creating a fresh new test case class just for this issue, 
if that's better.


[1] https://bugs.openjdk.java.net/browse/JDK-8201545

-Jaikiran

RFR 8202154 : Remove unused code in TwoStacksPlainDatagramSocketImpl.c

[Ivan Gerasimov]

Hello!

Some code in TwoStacksPlainDatagramSocketImpl.c is only relevant for 
earlier versions of Windows, which are no longer supported as of JDK 11.

This code can be safely removed.

Also removing an unused argument at 
DualStackPlainDatagramSocketImpl.socketCreate().


Would you please help review this cleanup?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202154
WEBREV: http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/

Thanks in advance!

--
With kind regards,
Ivan Gerasimov

Re: RFR 8202154 : Remove unused code in java.base/windows/native/libnet

[Ivan Gerasimov]

Hello again!

A few other files contain obsolete code, so they can be combined 
together in one fix.


The webrev was updated in place:
http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/

Here's the summary of additional changes:
- sun.net.PortConfig.getLower()/getUpper() always return the same range, 
so it can be defined with constants,

- NET_GetDefaultTOS() always returns zero, so it can be removed.

Would you please help review this?

With kind regards,
Ivan

On 4/23/18 2:29 PM, Ivan Gerasimov wrote:

Hello!

Some code in TwoStacksPlainDatagramSocketImpl.c is only relevant for 
earlier versions of Windows, which are no longer supported as of JDK 11.

This code can be safely removed.

Also removing an unused argument at 
DualStackPlainDatagramSocketImpl.socketCreate().


Would you please help review this cleanup?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202154
WEBREV: http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/

Thanks in advance!



--
With kind regards,
Ivan Gerasimov

Re: RFR 8202154 : Remove unused code in java.base/windows/native/libnet

[vyom tewari]

Hi Ivan,

code looks good to me, thanks for doing this cleanup. One minor comment, 
in PortConfig.java you can make defaultUpper& defaultLower as final.I 
see that Microsoft changed dynamic port range recently do we need to put 
some comment in PortConfig.java ?


Thanks, Vyom


On Tuesday 24 April 2018 09:47 AM, Ivan Gerasimov wrote:

Hello again!

A few other files contain obsolete code, so they can be combined 
together in one fix.


The webrev was updated in place:
http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/

Here's the summary of additional changes:
- sun.net.PortConfig.getLower()/getUpper() always return the same 
range, so it can be defined with constants,

- NET_GetDefaultTOS() always returns zero, so it can be removed.

Would you please help review this?

With kind regards,
Ivan

On 4/23/18 2:29 PM, Ivan Gerasimov wrote:

Hello!

Some code in TwoStacksPlainDatagramSocketImpl.c is only relevant for 
earlier versions of Windows, which are no longer supported as of JDK 11.

This code can be safely removed.

Also removing an unused argument at 
DualStackPlainDatagramSocketImpl.socketCreate().


Would you please help review this cleanup?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202154
WEBREV: http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/

Thanks in advance!