RFR: 8241248: NullPointerException in sun.security.ssl.HKDF.extract(HKDF.java:93)

[Alexey Bakhtin]

On Tue, 27 Apr 2021 16:13:31 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Hello All,
>> 
>> Could you please review the fix for the JDK-8241248?
>> The issue happens during the TLSv1.3 handshake without server stateless session resumption in case of server receives several parallel requests with the same pre_shared_key.
>> The main idea of the fix is to remove resuming session from the session cache in the early stage.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8241248
>> Webrev: http://cr.openjdk.java.net/~abakhtin/8241248/webrev.v0/
>> 
>> The test from the bug report using OpenSSL is passed ( -Djdk.tls.server.enableSessionTicketExtension=false )
>> javax/net/ssl and sun/security/ssl jtreg tests passed
>> 
>> Regards
>> Alexey
>
> src/java.base/share/classes/sun/security/ssl/PreSharedKeyExtension.java line 377:
> 
>> 375:                     // If we are keeping state, see if the identity is in the cache
>> 376:                     if (requestedId.identity.length == SessionId.MAX_LENGTH) {
>> 377:                         synchronized (sessionCache) {
> 
> Did you have a test if there is a performance regression by introducing the synchronization here?
> 
> I have to check the engineGetServerSessionContext() specification and implementation (if the sessionCache is a reference?), and the session cache implementation to make sure the synchronization works (if the synchronization on sessionCache is the same as the synchronization on the cache internal implementation) .  Maybe, the improvement could in the cache implementation for readability?

Yes, I’ve made a test that calculates total time spent by server to receive <N> connections. Every server handshake is performed in a separate thread
The client starts <T> threads. Every thread sends one initial connection and <R-1> renegotiated connections. So, the total number of connections is  <N>=<T>*<R>

Results for the original and JDK-8241248 code are almost identical:
<T>=10 <R>=100
Original OpenJDK: 1140 ms
JDK-8241248 code: 1090 ms

<T>=50 <R>=100
Original OpenJDK: 1164 ms
JDK-8241248 code: 1108 ms

<T>=60 <R>=100
Original OpenJDK: 1461 ms
JDK-8241248 code: 1579 ms

<T>=70 <R>=100
Original OpenJDK: 2058 ms
JDK-8241248 code: 1999 ms

<T>=80 <R>=100
Original OpenJDK: 2148 ms
JDK-8241248 code: 2125 ms

<T>=90 <R>=100
Original OpenJDK: 2540 ms
JDK-8241248 code: 2514 ms

<T>=90 <R>=100
Original OpenJDK: 3011 ms
JDK-8241248 code: 3010 ms

I can confirm that the synchronization code works. Without it I still catches exception from different threads trying to resume the same session from the cache

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

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