On Fri, 16 Jun 2023 13:54:37 GMT, Xue-Lei Andrew Fan <xue...@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
