RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v4]

[Xue-Lei Andrew Fan]

On Fri, 16 Jun 2023 11:59:01 GMT, Matthew Donovan <mdonovan at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 914:
>> 
>>> 912:     @Override
>>> 913:     public SSLSession getHandshakeSession() {
>>> 914:         engineLock.lock();
>> 
>> I'm not very sure of this update. By removing the enginLock on socket/engine level here,  is it possible there are two threads that one read the handshake session and another on write?
>
> `engineLock` was used to here to make sure that `conContext.handshakeContext` is not null before accessing it. The problem is that the `handshakeContext` is modified (set to null) by the `TransportContext` class. So using a lock in SSLEngineImpl or SSLSocketImpl doesn't protect `handshakeContext` from becoming null between the check and use.
> 
> The actual SSLSession object was never protected from concurrent modification (unless it has its own lock.)
> 
> When reviewing the code to answer this, I noticed that I needed to update `SSLEngineImpl.getHandshakeApplicationProtocol()` so there is a new commit.

I had a search of the value assignment of handshakeSession.  Here is one example of the calling stack:
SSLEngineImpl.DelegatedTask.run()->TransportContext.dispatch()->SSLHandshake().consume()->HandshakeConsumer.consume()->SSLExtensions.product()->ServerHello.produce()->shc.handshakeSession = session

The engineLock was placed on SSLEngineImpl.DelegatedTask.run() and released after complete the job, which means the value assignment of handshakeSession is synchronized.

The locks in SSLEngineImpl and SSLSocketImpl are used for operations synchronization so that other classes may not need additional locks in most cases.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1232286405