> http://cr.openjdk.java.net/~bae/8239788/webrev.v4/
SSLSocketInputRecord:
54 // Cache for incomplete input record.
55 private ByteBuffer inputBuffer = null;
This variable is used for record body, I may use a instinctive name, for
example recordBody.
Otherwise, looks good to me.
I think, for performance, it may be possible to reuse this buffer for
multiple records. I'd appreciate if you want to make an improvement in
this update as well.
Thanks,
Xuelei
On 3/2/2020 7:17 AM, Alexey Bakhtin wrote:
Hello Xuelei,
Could you please review new version of the patch :
http://cr.openjdk.java.net/~bae/8239788/webrev.v4/
I did not find any reasons for such getSession() behaviour. This code
seems exists since initial TLSv1.2 implementation. As you suggested,
I’ve changed getSession() to throw InterruptedIOException. It makes
changes in SSLSocketImpl.java and SSLTransport.java much simple.
I also updated inputBuffer implementation to use ByteBuffer. It stores
incoming data to handle socket timeouts and cleared after record is
processed.
Thank you
Alexey
On 29 Feb 2020, at 03:24, Xuelei Fan <xuelei....@oracle.com
<mailto:xuelei....@oracle.com>> wrote:
> Webrev JDK11: http://cr.openjdk.java.net/~bae/8239788/webrev.v3/
> getSession() method is implemented identically to JDK8, so it's
> behaviour is backward compatible to JDK8
I know, but I would like to see if there is really a compatibility
impact if the getSession() is consistent with other IO operations. We
could fix the problem later if there is a need.
> I may try to catch the InterruptedIOException, rather than handle it
> in the IOException catching block or the Exception catching block.
try {
...
} catch (Exception e) {
if (e instanceof ... ) {
...
} else (e instanceof ...) {
...
}
}
vs
try {
} catch (AException e) {
...
} catch (IOException e) {
...
} catch (Exception e) {
...
}
the later is the common coding style
SSLSocketInputRecord:
52 private byte[] inputBuffer = new byte[maxRecordSize];
Would you mind consider an improvement to use less memory?
If you have webrev for JDK 15, I could help to run more testing for you.
Thanks,
Xuelei
On 2/27/2020 9:05 AM, Alexey Bakhtin wrote:
Hello Xuelei,
You are right, SSLSocketInputRecord.read() method could be
interrupted before all requested data is received.
I have updated SSLSocketInputRecord class to use single buffer for
incoming data. Also I’ve updated read() method to take into account
every chunk of incoming data. It should prevent possible loss of data
if socket timed out.
Webrev JDK11: http://cr.openjdk.java.net/~bae/8239788/webrev.v3/
Tested with sun/security/ssl and javax/net/ssl jtreg tests
Thank you
Alexey
On 27 Feb 2020, at 01:04, Xuelei Fan <xuelei....@oracle.com
<mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com>> wrote:
> Webrev for JDK15 : http://cr.openjdk.java.net/~yan/8239788/webrev.2/
For the SSLSocketInputRecord.java update, is it possible that the
read() implementation interrupted before reading the exact specified
bytes of data? The received data may be lost if it is interrupted.
BTW, it might not be effective to use three fields to store the
data, temporary, header and inputBuffer. Is it possible to use just
one buffer (temporary, for example) and one integer to remember the
received data length?
Thanks,
Xuelei
On 2/26/2020 4:22 AM, Alexey Bakhtin wrote:
Hello Xuelei,
Thank you for review.
getSession() method is implemented identically to JDK8, so it's
behaviour is backward compatible to JDK8
I have updated review with some modifications:
1) Enabled sun/security/ssl/SSLSocketImpl/ClientTimeout.java jtreg
test. This test emulates socket timeout during handshake and app
data transfer.
2) I have added cache for incoming data received from socket
(sun.security.ssl.SSLSocketInputRecord class). Socket timeout could
happen while reading single handshake message by small chunks. It
is implemented similarly to JDK8 and allows to pass
sun/security/ssl/SSLSocketImpl/ClientTimeout test
3) I have added SocketTimeoutException handling into
sun/security/ssl/SSLSocketImpl/SSLExceptionForIOIssue.java jtreg
test. This test also verifies SocketExceptions during handshake.
Webrev for JDK11 : http://cr.openjdk.java.net/~yan/8239788/webrev.1/
Webrev for JDK15 : http://cr.openjdk.java.net/~yan/8239788/webrev.2/
Tested with sun/security/ssl and javax/net/ssl jtreg tests
Thank you
Alexey
On 25 Feb 2020, at 19:42, Xuelei Fan <xuelei....@oracle.com
<mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com>> wrote:
JDK11 webrev: http://cr.openjdk.java.net/~yan/8239788/webrev.0/
Maybe, the getSession() could throw InterruptedIOException as well.
I may try to catch the InterruptedIOException, rather than handle
it in the IOException catching block.
Thanks,
Xuelei
On 2/24/2020 11:04 AM, Alexey Bakhtin wrote:
Hi Xuelei,
Thank you. It would be glad if you can review this fix.
The patch almost cleanly applied to JDK15.
Also, As Kumar mentioned, the patch does not include regression test.
I’m going to add regression test and patch for JDK15 tomorrow.
Thank you.
Alexey
On 24 Feb 2020, at 21:41, Xuelei Fan <xuelei....@oracle.com
<mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com>>
wrote:
Hi Alexey,
Thanks for working on this issue. Do you plan to fix it for JDK
15, the current JDK reposiroty?
I need more time for evaluate the fix. For example, I'm not
sure if we could always throw InterruptedIOException to
applications, even for getSession().
Regards,
Xuelei
On 2/24/2020 7:58 AM, Alexey Bakhtin wrote:
Hello,
I have been working on this issue for some time already.
The patch below adds java.net.SocketTimeoutException handling
during TLS handshake negotiation. This functionality seems to
have been missed during the TLSv1.3 implementation ( JDK-8196584 )
Tested with JDK11 and higher.
JDK11 webrev: http://cr.openjdk.java.net/~yan/8239788/webrev.0/
jbs: https://bugs.openjdk.java.net/browse/JDK-8239798 and
https://bugs.openjdk.java.net/browse/JDK-8239788
Thank you,
Alexey
On 22 Feb 2020, at 15:00,
security-dev-requ...@openjdk.java.net
<mailto:security-dev-requ...@openjdk.java.net>
<mailto:security-dev-requ...@openjdk.java.net>
<mailto:security-dev-requ...@openjdk.java.net> wrote:
I will look into the issue.
BTW, I closed JDK-8239788 as duplicate of JDK-8239798.
Thanks,
Xuelei
On 2/21/2020 9:24 AM, Kumar Srinivasan wrote:
Hi security-folk,
At VMware while upgrading our application to OpenJDK11u, we have
encountered what seems to be a serious behavior issue.
The issue AFAICT seems to have stemmed from the work for
TLS1.3 and
JDK-8196584 <https://bugs.openjdk.java.net/browse/JDK-8196584>.
Overview:
With OpenJDK11 the end-points are closed immediately with TLS
alerts
raised when an exception is received.
This is not the case with JDK8 the socket is not closed
allowing retries.
I have filed: JDK-8239798 (with a reproducer), this issue was
also
reported ?to Azul and they have filed: JDK-8239788.
Can you please evaluate this at the earliest, this is a
serious show
stopper for VMware.
Thank
Kumar Srinivasan
VMware
