Mercurial > hg > icedtea8-forest > jdk

changeset 14945:3c8618453098

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
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
author goetz
date Sun, 15 Mar 2020 22:00:43 +0100
parents 97e9e5b6489b
children 7611b664f522
files src/share/classes/sun/security/ssl/SSLSocketImpl.java
diffstat 1 files changed, 294 insertions(+), 131 deletions(-) [+]
line wrap: on
line diff
--- 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<SSLSocket, List<String>, String> selector) {
-        conContext.sslConfig.socketAPSelector = selector;
+        socketLock.lock();
+        try {
+            conContext.sslConfig.socketAPSelector = selector;
+        } finally {
+            socketLock.unlock();
+        }
     }
 
     @Override
-    public synchronized BiFunction<SSLSocket, List<String>, String>
+    public BiFunction<SSLSocket, List<String>, 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();
+        }
     }
 
     /**