RFR: 8275811 Incorrect instance to dispose [v5]

[Daniel Jeliński]

On Wed, 3 Nov 2021 09:12:29 GMT, Daniel Jeliński <duke at openjdk.java.net> wrote:

>> src/java.base/share/classes/sun/security/ssl/SSLEngineOutputRecord.java line 436:
>> 
>>> 434: 
>>> 435:         void queueUpCipherDispose() {
>>> 436:             RecordMemo lastMemo = handshakeMemos.getLast();
>> 
>> Sorry, I missed that the getLast could throw exception if it is empty.  I may check it before the call to getLast.
>> 
>> +           if (handshakeMemos.isEmpty()) {
>> +              return;
>> +          }
>
> my mistake. Replaced with `peekLast`, should be better now.

Well, looks like this uncovered another, unrelated bug. `handshakeMemos` is only empty here when we handle TLS1.3 server hello and session ID is empty, see stack trace:

Caused by: java.util.NoSuchElementException
	at java.base/java.util.LinkedList.getLast(LinkedList.java:261)
	at java.base/sun.security.ssl.SSLEngineOutputRecord$HandshakeFragment.queueUpCipherDispose(SSLEngineOutputRecord.java:436)
	at java.base/sun.security.ssl.SSLEngineOutputRecord.disposeWriteCipher(SSLEngineOutputRecord.java:159)
	at java.base/sun.security.ssl.OutputRecord.changeWriteCiphers(OutputRecord.java:198)
	at java.base/sun.security.ssl.ServerHello$T13ServerHelloConsumer.consume(ServerHello.java:1372)

This is only supposed to be empty when [jdk.tls.client.useCompatibilityMode](https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L101) is false, which it never is (also the comment above that line is copy/pasted, should be fixed). So I did some more digging and found that we do not set sessionId in clientHello when resuming TLS1.3 session:
- sessionId is set to empty [here](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L406)
- not updated [here](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L535) because it's TLS 1.3
- not updated [here](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L577) because session is not null

Apparently we have no tests for TLS1.3 session resumption in jdk_security and no tests for useCompatibilityMode=false (otherwise I would have noticed this sooner).

Let me know if I should fix these issues here or in a separate PR.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6084