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

[Matthew Donovan]

On Fri, 16 Jun 2023 13:54:37 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> `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.

I see what you're saying with respect to SSLEngineImpl. It looks like all of the public methods are synchronized with `engineLock` so I shouldn't have made any changes there. I will revert them.

Are you ok with the changes in SSLSocketImpl?

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

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