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> 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> 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> 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> 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 >>>>>>>
signature.asc
Description: Message signed with OpenPGP
