OpenJDK11u: Backward incompatible behavior

Wed Mar 4 18:23:24 UTC 2020

 > 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.fan at oracle.com 
>> <mailto:xuelei.fan at 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.fan at oracle.com 
>>>> <mailto:xuelei.fan at oracle.com> <mailto:xuelei.fan at 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.fan at oracle.com 
>>>>>> <mailto:xuelei.fan at oracle.com> <mailto:xuelei.fan at 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.fan at oracle.com 
>>>>>>>> <mailto:xuelei.fan at oracle.com> <mailto:xuelei.fan at 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-request at openjdk.java.net 
>>>>>>>>>> <mailto:security-dev-request at openjdk.java.net> 
>>>>>>>>>> <mailto:security-dev-request at openjdk.java.net> 
>>>>>>>>>> <mailto:security-dev-request at 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
>>>>>>>>>>
> 