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

[Matthew Donovan]

On Fri, 16 Jun 2023 06:24:36 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Matthew Donovan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>> 
>>  - I missed a method that needed updating
>>  - Merge branch 'master' into socket-close-null
>>  - made handshake context lock final
>>  - using try/finally in terminateHandshakeContext and using local context variable in all places it should be
>>  - 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException
>
> 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.

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

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