On Fri, 7 May 2021 12:01:16 GMT, Alexey Bakhtin <abakh...@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 8u: 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 > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Add Cache.pull method Thank you for take my comments. The code looks nice to me, except a few trivial comments. 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: s = sessionCache.pull(requestedId.identity); Would you please add a comment here so as we know why a pull method could be used here? For example: - // If we are keeping state, see if the identity is in the cache + // If we are keeping state, see if the identity is in the + // cache. Note that for TLS 1.3, we would also clean + // up the cached session if it is not rejoinable. src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 183: > 181: if (id != null) > 182: return sessionCache.pull(new SessionId(id)); > 183: return null; As this is an internal method, it should be safe to assume that the id is non-null. I'm fine if you want to keep the non-null check, but please use braces for if-statement (see also https://www.oracle.com/java/technologies/javase/codeconventions-statements.html#449). src/java.base/share/classes/sun/security/util/Cache.java line 430: > 428: return null; > 429: } > 430: V value; I may add a blank line before this line. src/java.base/share/classes/sun/security/util/Cache.java line 442: > 440: entry.invalidate(); > 441: return value; > 442: } I may adjust the lines a little bit so as to avoid duplicated operations (see the implementation code of isValid()). long time = (lifetime == 0) ? 0 : System.currentTimeMillis(); if (entry.isValid(time)) { V value = entry.getValue(); entry.invalidate(); return value; } else { if (DEBUG) { System.out.println("Ignoring expired entry"); } return null; } ------------- PR: https://git.openjdk.java.net/jdk/pull/3664
