# HG changeset patch # User goetz # Date 1584306043 -3600 # Node ID 3c8618453098c65e1dd2c823b45ae8de737fe5ee # Parent 97e9e5b6489b6aa1c76d90f8674b15f3b10b294b 8240827: Downport SSLSocketImpl.java from "8221882: Use fiber-friendly java.util.concurrent.locks in JSSE" Summary: This fosters downport of "8219991: New fix of the deadlock in sun.security.ssl.SSLSocketImpl" Reviewed-by: clanger, mdoerr diff -r 97e9e5b6489b -r 3c8618453098 src/share/classes/sun/security/ssl/SSLSocketImpl.java --- a/src/share/classes/sun/security/ssl/SSLSocketImpl.java Tue Dec 18 15:18:44 2018 -0800 +++ b/src/share/classes/sun/security/ssl/SSLSocketImpl.java Sun Mar 15 22:00:43 2020 +0100 @@ -38,6 +38,7 @@ import java.net.UnknownHostException; import java.nio.ByteBuffer; import java.util.List; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.BiFunction; import javax.net.ssl.HandshakeCompletedListener; import javax.net.ssl.SSLException; @@ -83,6 +84,9 @@ private boolean isConnected = false; private volatile boolean tlsIsClosed = false; + private final ReentrantLock socketLock = new ReentrantLock(); + private final ReentrantLock handshakeLock = new ReentrantLock(); + /* * Is the local name service trustworthy? * @@ -291,14 +295,25 @@ } @Override - public synchronized String[] getEnabledCipherSuites() { - return CipherSuite.namesOf(conContext.sslConfig.enabledCipherSuites); + public String[] getEnabledCipherSuites() { + socketLock.lock(); + try { + return CipherSuite.namesOf( + conContext.sslConfig.enabledCipherSuites); + } finally { + socketLock.unlock(); + } } @Override - public synchronized void setEnabledCipherSuites(String[] suites) { - conContext.sslConfig.enabledCipherSuites = - CipherSuite.validValuesOf(suites); + public void setEnabledCipherSuites(String[] suites) { + socketLock.lock(); + try { + conContext.sslConfig.enabledCipherSuites = + CipherSuite.validValuesOf(suites); + } finally { + socketLock.unlock(); + } } @Override @@ -308,19 +323,29 @@ } @Override - public synchronized String[] getEnabledProtocols() { - return ProtocolVersion.toStringArray( - conContext.sslConfig.enabledProtocols); + public String[] getEnabledProtocols() { + socketLock.lock(); + try { + return ProtocolVersion.toStringArray( + conContext.sslConfig.enabledProtocols); + } finally { + socketLock.unlock(); + } } @Override - public synchronized void setEnabledProtocols(String[] protocols) { + public void setEnabledProtocols(String[] protocols) { if (protocols == null) { throw new IllegalArgumentException("Protocols cannot be null"); } - conContext.sslConfig.enabledProtocols = - ProtocolVersion.namesOf(protocols); + socketLock.lock(); + try { + conContext.sslConfig.enabledProtocols = + ProtocolVersion.namesOf(protocols); + } finally { + socketLock.unlock(); + } } @Override @@ -340,29 +365,44 @@ } @Override - public synchronized SSLSession getHandshakeSession() { - return conContext.handshakeContext == null ? - null : conContext.handshakeContext.handshakeSession; + public SSLSession getHandshakeSession() { + socketLock.lock(); + try { + return conContext.handshakeContext == null ? + null : conContext.handshakeContext.handshakeSession; + } finally { + socketLock.unlock(); + } } @Override - public synchronized void addHandshakeCompletedListener( + public void addHandshakeCompletedListener( HandshakeCompletedListener listener) { if (listener == null) { throw new IllegalArgumentException("listener is null"); } - conContext.sslConfig.addHandshakeCompletedListener(listener); + socketLock.lock(); + try { + conContext.sslConfig.addHandshakeCompletedListener(listener); + } finally { + socketLock.unlock(); + } } @Override - public synchronized void removeHandshakeCompletedListener( + public void removeHandshakeCompletedListener( HandshakeCompletedListener listener) { if (listener == null) { throw new IllegalArgumentException("listener is null"); } - conContext.sslConfig.removeHandshakeCompletedListener(listener); + socketLock.lock(); + try { + conContext.sslConfig.removeHandshakeCompletedListener(listener); + } finally { + socketLock.unlock(); + } } @Override @@ -376,7 +416,8 @@ throw new SocketException("Socket has been closed or broken"); } - synchronized (conContext) { // handshake lock + handshakeLock.lock(); + try { // double check the context status if (conContext.isBroken || conContext.isInboundClosed() || conContext.isOutboundClosed()) { @@ -399,53 +440,95 @@ } catch (Exception oe) { // including RuntimeException handleException(oe); } + } finally { + handshakeLock.unlock(); + } + } + + @Override + public void setUseClientMode(boolean mode) { + socketLock.lock(); + try { + conContext.setUseClientMode(mode); + } finally { + socketLock.unlock(); + } + } + + @Override + public boolean getUseClientMode() { + socketLock.lock(); + try { + return conContext.sslConfig.isClientMode; + } finally { + socketLock.unlock(); + } + } + + @Override + public void setNeedClientAuth(boolean need) { + socketLock.lock(); + try { + conContext.sslConfig.clientAuthType = + (need ? ClientAuthType.CLIENT_AUTH_REQUIRED : + ClientAuthType.CLIENT_AUTH_NONE); + } finally { + socketLock.unlock(); } } @Override - public synchronized void setUseClientMode(boolean mode) { - conContext.setUseClientMode(mode); + public boolean getNeedClientAuth() { + socketLock.lock(); + try { + return (conContext.sslConfig.clientAuthType == + ClientAuthType.CLIENT_AUTH_REQUIRED); + } finally { + socketLock.unlock(); + } } @Override - public synchronized boolean getUseClientMode() { - return conContext.sslConfig.isClientMode; - } - - @Override - public synchronized void setNeedClientAuth(boolean need) { - conContext.sslConfig.clientAuthType = - (need ? ClientAuthType.CLIENT_AUTH_REQUIRED : - ClientAuthType.CLIENT_AUTH_NONE); + public void setWantClientAuth(boolean want) { + socketLock.lock(); + try { + conContext.sslConfig.clientAuthType = + (want ? ClientAuthType.CLIENT_AUTH_REQUESTED : + ClientAuthType.CLIENT_AUTH_NONE); + } finally { + socketLock.unlock(); + } } @Override - public synchronized boolean getNeedClientAuth() { - return (conContext.sslConfig.clientAuthType == - ClientAuthType.CLIENT_AUTH_REQUIRED); - } - - @Override - public synchronized void setWantClientAuth(boolean want) { - conContext.sslConfig.clientAuthType = - (want ? ClientAuthType.CLIENT_AUTH_REQUESTED : - ClientAuthType.CLIENT_AUTH_NONE); + public boolean getWantClientAuth() { + socketLock.lock(); + try { + return (conContext.sslConfig.clientAuthType == + ClientAuthType.CLIENT_AUTH_REQUESTED); + } finally { + socketLock.unlock(); + } } @Override - public synchronized boolean getWantClientAuth() { - return (conContext.sslConfig.clientAuthType == - ClientAuthType.CLIENT_AUTH_REQUESTED); + public void setEnableSessionCreation(boolean flag) { + socketLock.lock(); + try { + conContext.sslConfig.enableSessionCreation = flag; + } finally { + socketLock.unlock(); + } } @Override - public synchronized void setEnableSessionCreation(boolean flag) { - conContext.sslConfig.enableSessionCreation = flag; - } - - @Override - public synchronized boolean getEnableSessionCreation() { - return conContext.sslConfig.enableSessionCreation; + public boolean getEnableSessionCreation() { + socketLock.lock(); + try { + return conContext.sslConfig.enableSessionCreation; + } finally { + socketLock.unlock(); + } } @Override @@ -682,20 +765,25 @@ } @Override - public synchronized InputStream getInputStream() throws IOException { - if (isClosed()) { - throw new SocketException("Socket is closed"); - } + public InputStream getInputStream() throws IOException { + socketLock.lock(); + try { + if (isClosed()) { + throw new SocketException("Socket is closed"); + } - if (!isConnected) { - throw new SocketException("Socket is not connected"); - } + if (!isConnected) { + throw new SocketException("Socket is not connected"); + } - if (conContext.isInboundClosed() || isInputShutdown()) { - throw new SocketException("Socket input is already shutdown"); + if (conContext.isInboundClosed() || isInputShutdown()) { + throw new SocketException("Socket input is already shutdown"); + } + + return appInput; + } finally { + socketLock.unlock(); } - - return appInput; } private void ensureNegotiated() throws IOException { @@ -704,7 +792,8 @@ return; } - synchronized (conContext) { // handshake lock + handshakeLock.lock(); + try { // double check the context status if (conContext.isNegotiated || conContext.isBroken || conContext.isInboundClosed() || @@ -713,6 +802,8 @@ } startHandshake(); + } finally { + handshakeLock.unlock(); } } @@ -730,6 +821,9 @@ // Is application data available in the stream? private volatile boolean appDataIsAvailable; + // reading lock + private final ReentrantLock readLock = new ReentrantLock(); + AppInputStream() { this.appDataIsAvailable = false; this.buffer = ByteBuffer.allocate(4096); @@ -808,7 +902,8 @@ // // Note that the receiving and processing of post-handshake message // are also synchronized with the read lock. - synchronized (this) { + readLock.lock(); + try { int remains = available(); if (remains > 0) { int howmany = Math.min(remains, len); @@ -840,6 +935,8 @@ // dummy for compiler return -1; } + } finally { + readLock.unlock(); } } @@ -851,19 +948,24 @@ * things simpler. */ @Override - public synchronized long skip(long n) throws IOException { + public long skip(long n) throws IOException { // dummy array used to implement skip() byte[] skipArray = new byte[256]; + long skipped = 0; - long skipped = 0; - while (n > 0) { - int len = (int)Math.min(n, skipArray.length); - int r = read(skipArray, 0, len); - if (r <= 0) { - break; + readLock.lock(); + try { + while (n > 0) { + int len = (int)Math.min(n, skipArray.length); + int r = read(skipArray, 0, len); + if (r <= 0) { + break; + } + n -= r; + skipped += r; } - n -= r; - skipped += r; + } finally { + readLock.unlock(); } return skipped; @@ -911,8 +1013,18 @@ * Try the best to use up the input records so as to close the * socket gracefully, without impact the performance too much. */ - private synchronized void deplete() { - if (!conContext.isInboundClosed()) { + private void deplete() { + if (conContext.isInboundClosed()) { + return; + } + + readLock.lock(); + try { + // double check + if (conContext.isInboundClosed()) { + return; + } + if (!(conContext.inputRecord instanceof SSLSocketInputRecord)) { return; } @@ -928,25 +1040,32 @@ "input stream close depletion failed", ioe); } } + } finally { + readLock.unlock(); } } } @Override - public synchronized OutputStream getOutputStream() throws IOException { - if (isClosed()) { - throw new SocketException("Socket is closed"); - } + public OutputStream getOutputStream() throws IOException { + socketLock.lock(); + try { + if (isClosed()) { + throw new SocketException("Socket is closed"); + } - if (!isConnected) { - throw new SocketException("Socket is not connected"); - } + if (!isConnected) { + throw new SocketException("Socket is not connected"); + } - if (conContext.isOutboundDone() || isOutputShutdown()) { - throw new SocketException("Socket output is already shutdown"); + if (conContext.isOutboundDone() || isOutputShutdown()) { + throw new SocketException("Socket output is already shutdown"); + } + + return appOutput; + } finally { + socketLock.unlock(); } - - return appOutput; } @@ -1036,44 +1155,74 @@ } @Override - public synchronized SSLParameters getSSLParameters() { - return conContext.sslConfig.getSSLParameters(); - } - - @Override - public synchronized void setSSLParameters(SSLParameters params) { - conContext.sslConfig.setSSLParameters(params); - - if (conContext.sslConfig.maximumPacketSize != 0) { - conContext.outputRecord.changePacketSize( - conContext.sslConfig.maximumPacketSize); + public SSLParameters getSSLParameters() { + socketLock.lock(); + try { + return conContext.sslConfig.getSSLParameters(); + } finally { + socketLock.unlock(); } } @Override - public synchronized String getApplicationProtocol() { - return conContext.applicationProtocol; + public void setSSLParameters(SSLParameters params) { + socketLock.lock(); + try { + conContext.sslConfig.setSSLParameters(params); + + if (conContext.sslConfig.maximumPacketSize != 0) { + conContext.outputRecord.changePacketSize( + conContext.sslConfig.maximumPacketSize); + } + } finally { + socketLock.unlock(); + } } @Override - public synchronized String getHandshakeApplicationProtocol() { - if (conContext.handshakeContext != null) { - return conContext.handshakeContext.applicationProtocol; + public String getApplicationProtocol() { + socketLock.lock(); + try { + return conContext.applicationProtocol; + } finally { + socketLock.unlock(); + } + } + + @Override + public String getHandshakeApplicationProtocol() { + socketLock.lock(); + try { + if (conContext.handshakeContext != null) { + return conContext.handshakeContext.applicationProtocol; + } + } finally { + socketLock.unlock(); } return null; } @Override - public synchronized void setHandshakeApplicationProtocolSelector( + public void setHandshakeApplicationProtocolSelector( BiFunction, String> selector) { - conContext.sslConfig.socketAPSelector = selector; + socketLock.lock(); + try { + conContext.sslConfig.socketAPSelector = selector; + } finally { + socketLock.unlock(); + } } @Override - public synchronized BiFunction, String> + public BiFunction, String> getHandshakeApplicationProtocolSelector() { - return conContext.sslConfig.socketAPSelector; + socketLock.lock(); + try { + return conContext.sslConfig.socketAPSelector; + } finally { + socketLock.unlock(); + } } /** @@ -1143,8 +1292,11 @@ try { Plaintext plainText; - synchronized (this) { + socketLock.lock(); + try { plainText = decode(buffer); + } finally { + socketLock.unlock(); } if (plainText.contentType == ContentType.APPLICATION_DATA.id && buffer.position() > 0) { @@ -1223,27 +1375,33 @@ * * Called by connect, the layered constructor, and SSLServerSocket. */ - synchronized void doneConnect() throws IOException { - // In server mode, it is not necessary to set host and serverNames. - // Otherwise, would require a reverse DNS lookup to get the hostname. - if (peerHost == null || peerHost.isEmpty()) { - boolean useNameService = - trustNameService && conContext.sslConfig.isClientMode; - useImplicitHost(useNameService); - } else { - conContext.sslConfig.serverNames = - Utilities.addToSNIServerNameList( - conContext.sslConfig.serverNames, peerHost); + void doneConnect() throws IOException { + socketLock.lock(); + try { + // In server mode, it is not necessary to set host and serverNames. + // Otherwise, would require a reverse DNS lookup to get + // the hostname. + if (peerHost == null || peerHost.isEmpty()) { + boolean useNameService = + trustNameService && conContext.sslConfig.isClientMode; + useImplicitHost(useNameService); + } else { + conContext.sslConfig.serverNames = + Utilities.addToSNIServerNameList( + conContext.sslConfig.serverNames, peerHost); + } + + InputStream sockInput = super.getInputStream(); + conContext.inputRecord.setReceiverStream(sockInput); + + OutputStream sockOutput = super.getOutputStream(); + conContext.inputRecord.setDeliverStream(sockOutput); + conContext.outputRecord.setDeliverStream(sockOutput); + + this.isConnected = true; + } finally { + socketLock.unlock(); } - - InputStream sockInput = super.getInputStream(); - conContext.inputRecord.setReceiverStream(sockInput); - - OutputStream sockOutput = super.getOutputStream(); - conContext.inputRecord.setDeliverStream(sockOutput); - conContext.outputRecord.setDeliverStream(sockOutput); - - this.isConnected = true; } private void useImplicitHost(boolean useNameService) { @@ -1287,11 +1445,16 @@ // Please NOTE that this method MUST be called before calling to // SSLSocket.setSSLParameters(). Otherwise, the {@code host} parameter // may override SNIHostName in the customized server name indication. - public synchronized void setHost(String host) { - this.peerHost = host; - this.conContext.sslConfig.serverNames = - Utilities.addToSNIServerNameList( - conContext.sslConfig.serverNames, host); + public void setHost(String host) { + socketLock.lock(); + try { + this.peerHost = host; + this.conContext.sslConfig.serverNames = + Utilities.addToSNIServerNameList( + conContext.sslConfig.serverNames, host); + } finally { + socketLock.unlock(); + } } /**