8224829: AsyncSSLSocketClose.java has timing issue
authorxuelei
Fri, 14 Jun 2019 12:19:14 -0700
changeset 55411 328d4a455e4b
parent 55410 c3b354fdbaa4
child 55412 55a79ffab804
8224829: AsyncSSLSocketClose.java has timing issue Reviewed-by: jnimeh, dfuchs
src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java
test/jdk/javax/net/ssl/SSLSocket/Tls13PacketSize.java
test/jdk/sun/security/ssl/SSLSocketImpl/BlockedAsyncClose.java
--- a/src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java	Fri Jun 14 10:02:57 2019 +0200
+++ b/src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java	Fri Jun 14 12:19:14 2019 -0700
@@ -38,6 +38,7 @@
 import java.net.UnknownHostException;
 import java.nio.ByteBuffer;
 import java.util.List;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.BiFunction;
 import javax.net.ssl.HandshakeCompletedListener;
@@ -618,27 +619,76 @@
 
         // Need a lock here so that the user_canceled alert and the
         // close_notify alert can be delivered together.
-        conContext.outputRecord.recordLock.lock();
-        try {
+        int linger = getSoLinger();
+        if (linger >= 0) {
+            // don't wait more than SO_LINGER for obtaining the
+            // the lock.
+            //
+            // keep and clear the current thread interruption status.
+            boolean interrupted = Thread.interrupted();
             try {
-                // send a user_canceled alert if needed.
-                if (useUserCanceled) {
-                    conContext.warning(Alert.USER_CANCELED);
+                if (conContext.outputRecord.recordLock.tryLock() ||
+                        conContext.outputRecord.recordLock.tryLock(
+                                linger, TimeUnit.SECONDS)) {
+                    try {
+                        handleClosedNotifyAlert(useUserCanceled);
+                    } finally {
+                        conContext.outputRecord.recordLock.unlock();
+                    }
+                } else {
+                    // For layered, non-autoclose sockets, we are not
+                    // able to bring them into a usable state, so we
+                    // treat it as fatal error.
+                    if (!super.isOutputShutdown()) {
+                        if (isLayered() && !autoClose) {
+                            throw new SSLException(
+                                    "SO_LINGER timeout, " +
+                                    "close_notify message cannot be sent.");
+                        } else {
+                            super.shutdownOutput();
+                            if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
+                                SSLLogger.warning(
+                                    "SSLSocket output duplex close failed: " +
+                                    "SO_LINGER timeout, " +
+                                    "close_notify message cannot be sent.");
+                            }
+                        }
+                    }
+
+                    // RFC2246 requires that the session becomes
+                    // unresumable if any connection is terminated
+                    // without proper close_notify messages with
+                    // level equal to warning.
+                    //
+                    // RFC4346 no longer requires that a session not be
+                    // resumed if failure to properly close a connection.
+                    //
+                    // We choose to make the session unresumable if
+                    // failed to send the close_notify message.
+                    //
+                    conContext.conSession.invalidate();
+                    if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
+                        SSLLogger.warning(
+                                "Invalidate the session: SO_LINGER timeout, " +
+                                "close_notify message cannot be sent.");
+                    }
                 }
+            } catch (InterruptedException ex) {
+                // keep interrupted status
+                interrupted = true;
+            }
 
-                // send a close_notify alert
-                conContext.warning(Alert.CLOSE_NOTIFY);
+            // restore the interrupted status
+            if (interrupted) {
+                Thread.currentThread().interrupt();
+            }
+        } else {
+            conContext.outputRecord.recordLock.lock();
+            try {
+                handleClosedNotifyAlert(useUserCanceled);
             } finally {
-                if (!conContext.isOutboundClosed()) {
-                    conContext.outputRecord.close();
-                }
-
-                if ((autoClose || !isLayered()) && !super.isOutputShutdown()) {
-                    super.shutdownOutput();
-                }
+                conContext.outputRecord.recordLock.unlock();
             }
-        } finally {
-            conContext.outputRecord.recordLock.unlock();
         }
 
         if (!isInputShutdown()) {
@@ -646,6 +696,28 @@
         }
     }
 
+    private void handleClosedNotifyAlert(
+            boolean useUserCanceled) throws IOException {
+        try {
+            // send a user_canceled alert if needed.
+            if (useUserCanceled) {
+                conContext.warning(Alert.USER_CANCELED);
+            }
+
+            // send a close_notify alert
+            conContext.warning(Alert.CLOSE_NOTIFY);
+        } finally {
+            if (!conContext.isOutboundClosed()) {
+                conContext.outputRecord.close();
+            }
+
+            if (!super.isOutputShutdown() &&
+                    (autoClose || !isLayered())) {
+                super.shutdownOutput();
+            }
+        }
+    }
+
     /**
      * Duplex close, start from closing inbound.
      *
--- a/test/jdk/javax/net/ssl/SSLSocket/Tls13PacketSize.java	Fri Jun 14 10:02:57 2019 +0200
+++ b/test/jdk/javax/net/ssl/SSLSocket/Tls13PacketSize.java	Fri Jun 14 12:19:14 2019 -0700
@@ -53,6 +53,9 @@
 
     @Override
     protected void runServerApplication(SSLSocket socket) throws Exception {
+        // Set SO_LINGER in case of slow socket
+        socket.setSoLinger(true, 10);
+
         // here comes the test logic
         InputStream sslIS = socket.getInputStream();
         OutputStream sslOS = socket.getOutputStream();
@@ -81,6 +84,9 @@
      * @see #isCustomizedClientConnection()
      */
     protected void runClientApplication(SSLSocket socket) throws Exception {
+        // Set SO_LINGER in case of slow socket
+        socket.setSoLinger(true, 10);
+
         socket.setEnabledProtocols(new String[] {"TLSv1.3"});
         InputStream sslIS = socket.getInputStream();
         OutputStream sslOS = socket.getOutputStream();
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/sun/security/ssl/SSLSocketImpl/BlockedAsyncClose.java	Fri Jun 14 12:19:14 2019 -0700
@@ -0,0 +1,147 @@
+/*
+ * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+//
+// SunJSSE does not support dynamic system properties, no way to re-use
+// system properties in samevm/agentvm mode.
+//
+
+/*
+ * @test
+ * @bug 8224829
+ * @summary AsyncSSLSocketClose.java has timing issue
+ * @run main/othervm BlockedAsyncClose
+ */
+
+import javax.net.ssl.*;
+import java.io.*;
+import java.net.SocketException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+public class BlockedAsyncClose implements Runnable {
+    SSLSocket socket;
+    SSLServerSocket ss;
+
+    // Is the socket ready to close?
+    private final CountDownLatch closeCondition = new CountDownLatch(1);
+
+    // Where do we find the keystores?
+    static String pathToStores = "../../../../javax/net/ssl/etc";
+    static String keyStoreFile = "keystore";
+    static String trustStoreFile = "truststore";
+    static String passwd = "passphrase";
+
+    public static void main(String[] args) throws Exception {
+        String keyFilename =
+            System.getProperty("test.src", "./") + "/" + pathToStores +
+                "/" + keyStoreFile;
+        String trustFilename =
+            System.getProperty("test.src", "./") + "/" + pathToStores +
+                "/" + trustStoreFile;
+
+        System.setProperty("javax.net.ssl.keyStore", keyFilename);
+        System.setProperty("javax.net.ssl.keyStorePassword", passwd);
+        System.setProperty("javax.net.ssl.trustStore", trustFilename);
+        System.setProperty("javax.net.ssl.trustStorePassword", passwd);
+
+        new BlockedAsyncClose();
+    }
+
+    public BlockedAsyncClose() throws Exception {
+        SSLServerSocketFactory sslssf =
+                (SSLServerSocketFactory)SSLServerSocketFactory.getDefault();
+        InetAddress loopback = InetAddress.getLoopbackAddress();
+        ss = (SSLServerSocket)sslssf.createServerSocket();
+        ss.bind(new InetSocketAddress(loopback, 0));
+
+        SSLSocketFactory sslsf =
+            (SSLSocketFactory)SSLSocketFactory.getDefault();
+        socket = (SSLSocket)sslsf.createSocket(loopback, ss.getLocalPort());
+        SSLSocket serverSoc = (SSLSocket)ss.accept();
+        ss.close();
+
+        (new Thread(this)).start();
+        serverSoc.startHandshake();
+
+        boolean closeIsReady = closeCondition.await(90L, TimeUnit.SECONDS);
+        if (!closeIsReady) {
+            System.out.println(
+                    "Ignore, the closure is not ready yet in 90 seconds.");
+            return;
+        }
+
+        socket.setSoLinger(true, 10);
+        System.out.println("Calling Socket.close");
+
+        // Sleep for a while so that the write thread blocks by hitting the
+        // output stream buffer limit.
+        Thread.sleep(1000);
+
+        socket.close();
+        System.out.println("ssl socket get closed");
+        System.out.flush();
+    }
+
+    // block in write
+    public void run() {
+        byte[] ba = new byte[1024];
+        for (int i = 0; i < ba.length; i++) {
+            ba[i] = 0x7A;
+        }
+
+        try {
+            OutputStream os = socket.getOutputStream();
+            int count = 0;
+
+            // 1st round write
+            count += ba.length;
+            System.out.println(count + " bytes to be written");
+            os.write(ba);
+            System.out.println(count + " bytes written");
+
+            // Signal, ready to close.
+            closeCondition.countDown();
+
+            // write more
+            while (true) {
+                count += ba.length;
+                System.out.println(count + " bytes to be written");
+                os.write(ba);
+                System.out.println(count + " bytes written");
+            }
+        } catch (SocketException se) {
+            // the closing may be in progress
+            System.out.println("interrupted? " + se);
+        } catch (Exception e) {
+            if (socket.isClosed() || socket.isOutputShutdown()) {
+                System.out.println("interrupted, the socket is closed");
+            } else {
+                throw new RuntimeException("interrupted?", e);
+            }
+        }
+    }
+}
+