On Wed, 21 Dec 2022 00:10:08 GMT, David Schlosnagle <d...@openjdk.org> wrote:
>> Volker Simonis has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Some refactoring and simplification. Moved most of the implementation >> from SessionTicketExtension to SSLSessionContextImpl >> - Moving currentKeyID/keyHashMap to SSLSessionContextImpl as requested by >> @XueleiFan > > src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line > 195: > >> 193: // Suppress >> 194: } >> 195: keyHashMap.remove(entry.getKey()); > > Would it be worth using the entrySet iterator to remove the entry as opposed > to looking it up? Should the key be removed from entries before destroying so > that another thread doesn't read a destroyed key? I've did the changes you've proposed. But notice that already the initial implementation had a race between key usage and destruction (because there hasn't been and still is no synchronization between getting and deleting a session key) and even with your proposed change the race still exists. I.e. a thread A can get a key which was is about to expire. Then, just before A uses the key for decrypting a session ticket, another thread B destroys the key just after it expired but before it is used by A. However, as I've already explained before, that's not fatal. It will just lead to the rejection of the session and re-negotiation of the connection. With the default settings where keys IDs are changed (and expired session keys are deleted) every 60 minutes and session keys remain valid for 24 hours, this will also be a very unlikely event, so I suppose it is acceptable. ------------- PR: https://git.openjdk.org/jdk/pull/11590
