Fixes #4209 - Unused TLS connection is not closed in Java 11.

Added workarounds for the Java 11 behavior.

In fill(), call closeInbound() if we filled -1 and the handshake
did not start yet. This avoids to send a ClientHello to the peer
even if we are closing.

In flush(), if the handshake status is NEED_UNWRAP but we are
closing, force a wrap().

Added test cases.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java

@@ -19,17 +19,22 @@
 package org.eclipse.jetty.client;
 
 import java.io.BufferedReader;
+import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.net.ServerSocket;
 import java.net.Socket;
 import java.net.SocketTimeoutException;
+import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
+import javax.net.ssl.SSLEngine;
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLPeerUnverifiedException;
 import javax.net.ssl.SSLSocket;
@@ -38,12 +43,20 @@ import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpScheme;
 import org.eclipse.jetty.http.HttpStatus;
+import org.eclipse.jetty.io.ByteBufferPool;
 import org.eclipse.jetty.io.ClientConnectionFactory;
+import org.eclipse.jetty.io.EndPoint;
 import org.eclipse.jetty.io.ssl.SslClientConnectionFactory;
+import org.eclipse.jetty.io.ssl.SslConnection;
 import org.eclipse.jetty.io.ssl.SslHandshakeListener;
+import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.Handler;
+import org.eclipse.jetty.server.HttpConfiguration;
+import org.eclipse.jetty.server.HttpConnectionFactory;
+import org.eclipse.jetty.server.SecureRequestCustomizer;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.server.SslConnectionFactory;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.thread.ExecutorThreadPool;
@@ -555,4 +568,196 @@ public class HttpClientTLSTest
 
         assertTrue(latch.await(5, TimeUnit.SECONDS));
     }
+
+    @Test
+    public void testNeverUsedConnectionThenServerIdleTimeout() throws Exception
+    {
+        long idleTimeout = 2000;
+
+        SslContextFactory serverTLSFactory = createServerSslContextFactory();
+        QueuedThreadPool serverThreads = new QueuedThreadPool();
+        serverThreads.setName("server");
+        server = new Server(serverThreads);
+        HttpConfiguration httpConfig = new HttpConfiguration();
+        httpConfig.addCustomizer(new SecureRequestCustomizer());
+        HttpConnectionFactory http = new HttpConnectionFactory(httpConfig);
+        AtomicLong serverBytes = new AtomicLong();
+        SslConnectionFactory ssl = new SslConnectionFactory(serverTLSFactory, http.getProtocol())
+        {
+            @Override
+            protected SslConnection newSslConnection(Connector connector, EndPoint endPoint, SSLEngine engine)
+            {
+                return new SslConnection(connector.getByteBufferPool(), connector.getExecutor(), endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
+                {
+                    @Override
+                    protected int networkFill(ByteBuffer input) throws IOException
+                    {
+                        int n = super.networkFill(input);
+                        if (n > 0)
+                            serverBytes.addAndGet(n);
+                        return n;
+                    }
+                };
+            }
+        };
+        connector = new ServerConnector(server, 1, 1, ssl, http);
+        connector.setIdleTimeout(idleTimeout);
+        server.addConnector(connector);
+        server.setHandler(new EmptyServerHandler());
+        server.start();
+
+        SslContextFactory clientTLSFactory = createClientSslContextFactory();
+        QueuedThreadPool clientThreads = new QueuedThreadPool();
+        clientThreads.setName("client");
+        AtomicLong clientBytes = new AtomicLong();
+        client = new HttpClient(clientTLSFactory)
+        {
+            @Override
+            protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory)
+            {
+                if (sslContextFactory == null)
+                    sslContextFactory = getSslContextFactory();
+                return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory)
+                {
+                    @Override
+                    protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine)
+                    {
+                        return new SslConnection(byteBufferPool, executor, endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
+                        {
+                            @Override
+                            protected int networkFill(ByteBuffer input) throws IOException
+                            {
+                                int n = super.networkFill(input);
+                                if (n > 0)
+                                    clientBytes.addAndGet(n);
+                                return n;
+                            }
+                        };
+                    }
+                };
+            }
+        };
+        client.setExecutor(clientThreads);
+        client.start();
+
+        // Create a connection but don't use it.
+        String scheme = HttpScheme.HTTPS.asString();
+        String host = "localhost";
+        int port = connector.getLocalPort();
+        HttpDestination destination = (HttpDestination)client.getDestination(scheme, host, port);
+        DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool();
+        // Trigger the creation of a new connection, but don't use it.
+        connectionPool.tryCreate(-1);
+        // Verify that the connection has been created.
+        while (true)
+        {
+            Thread.sleep(50);
+            if (connectionPool.getConnectionCount() == 1)
+                break;
+        }
+
+        // Wait for the server to idle timeout the connection.
+        Thread.sleep(idleTimeout + idleTimeout / 2);
+
+        // The connection should be gone from the connection pool.
+        assertEquals(0, connectionPool.getConnectionCount(), connectionPool.dump());
+        assertEquals(0, serverBytes.get());
+        assertEquals(0, clientBytes.get());
+    }
+
+    @Test
+    public void testNeverUsedConnectionThenClientIdleTimeout() throws Exception
+    {
+        SslContextFactory serverTLSFactory = createServerSslContextFactory();
+        QueuedThreadPool serverThreads = new QueuedThreadPool();
+        serverThreads.setName("server");
+        server = new Server(serverThreads);
+        HttpConfiguration httpConfig = new HttpConfiguration();
+        httpConfig.addCustomizer(new SecureRequestCustomizer());
+        HttpConnectionFactory http = new HttpConnectionFactory(httpConfig);
+        AtomicLong serverBytes = new AtomicLong();
+        SslConnectionFactory ssl = new SslConnectionFactory(serverTLSFactory, http.getProtocol())
+        {
+            @Override
+            protected SslConnection newSslConnection(Connector connector, EndPoint endPoint, SSLEngine engine)
+            {
+                return new SslConnection(connector.getByteBufferPool(), connector.getExecutor(), endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
+                {
+                    @Override
+                    protected int networkFill(ByteBuffer input) throws IOException
+                    {
+                        int n = super.networkFill(input);
+                        if (n > 0)
+                            serverBytes.addAndGet(n);
+                        return n;
+                    }
+                };
+            }
+        };
+        connector = new ServerConnector(server, 1, 1, ssl, http);
+        server.addConnector(connector);
+        server.setHandler(new EmptyServerHandler());
+        server.start();
+
+        long idleTimeout = 2000;
+
+        SslContextFactory clientTLSFactory = createClientSslContextFactory();
+        QueuedThreadPool clientThreads = new QueuedThreadPool();
+        clientThreads.setName("client");
+        AtomicLong clientBytes = new AtomicLong();
+        client = new HttpClient(clientTLSFactory)
+        {
+            @Override
+            protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory)
+            {
+                if (sslContextFactory == null)
+                    sslContextFactory = getSslContextFactory();
+                return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory)
+                {
+                    @Override
+                    protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine)
+                    {
+                        return new SslConnection(byteBufferPool, executor, endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
+                        {
+                            @Override
+                            protected int networkFill(ByteBuffer input) throws IOException
+                            {
+                                int n = super.networkFill(input);
+                                if (n > 0)
+                                    clientBytes.addAndGet(n);
+                                return n;
+                            }
+                        };
+                    }
+                };
+            }
+        };
+        client.setIdleTimeout(idleTimeout);
+        client.setExecutor(clientThreads);
+        client.start();
+
+        // Create a connection but don't use it.
+        String scheme = HttpScheme.HTTPS.asString();
+        String host = "localhost";
+        int port = connector.getLocalPort();
+        HttpDestination destination = (HttpDestination)client.getDestination(scheme, host, port);
+        DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool();
+        // Trigger the creation of a new connection, but don't use it.
+        connectionPool.tryCreate(-1);
+        // Verify that the connection has been created.
+        while (true)
+        {
+            Thread.sleep(50);
+            if (connectionPool.getConnectionCount() == 1)
+                break;
+        }
+
+        // Wait for the client to idle timeout the connection.
+        Thread.sleep(idleTimeout + idleTimeout / 2);
+
+        // The connection should be gone from the connection pool.
+        assertEquals(0, connectionPool.getConnectionCount(), connectionPool.dump());
+        assertEquals(0, serverBytes.get());
+        assertEquals(0, clientBytes.get());
+    }
 }

jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java

@@ -80,9 +80,10 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
     private static final Logger LOG = Log.getLogger(SslConnection.class);
     private static final String TLS_1_3 = "TLSv1.3";
 
-    private enum Handshake
+    private enum HandshakeState
     {
         INITIAL,
+        HANDSHAKE,
         SUCCEEDED,
         FAILED
     }
@@ -116,7 +117,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
     private boolean _requireCloseMessage;
     private FlushState _flushState = FlushState.IDLE;
     private FillState _fillState = FillState.IDLE;
-    private AtomicReference<Handshake> _handshake = new AtomicReference<>(Handshake.INITIAL);
+    private AtomicReference<HandshakeState> _handshake = new AtomicReference<>(HandshakeState.INITIAL);
     private boolean _underflown;
 
     private abstract class RunnableTask implements Runnable, Invocable
@@ -291,6 +292,22 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
         _requireCloseMessage = requireCloseMessage;
     }
 
+    private boolean isHandshakeInitial()
+    {
+        return _handshake.get() == HandshakeState.INITIAL;
+    }
+
+    private boolean isHandshakeSucceeded()
+    {
+        return _handshake.get() == HandshakeState.SUCCEEDED;
+    }
+
+    private boolean isHandshakeComplete()
+    {
+        HandshakeState state = _handshake.get();
+        return state == HandshakeState.SUCCEEDED || state == HandshakeState.FAILED;
+    }
+
     private void acquireEncryptedInput()
     {
         if (_encryptedInput == null)
@@ -393,6 +410,16 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
         }
     }
 
+    protected int networkFill(ByteBuffer input) throws IOException
+    {
+        return getEndPoint().fill(input);
+    }
+
+    protected boolean networkFlush(ByteBuffer output) throws IOException
+    {
+        return getEndPoint().flush(output);
+    }
+
     public class DecryptedEndPoint extends AbstractEndPoint
     {
         private final Callback _incompleteWriteCallback = new IncompleteWriteCallback();
@@ -590,14 +617,23 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
                             }
 
                             // Let's try reading some encrypted data... even if we have some already.
-                            int netFilled = getEndPoint().fill(_encryptedInput);
+                            int netFilled = networkFill(_encryptedInput);
-
                             if (LOG.isDebugEnabled())
                                 LOG.debug("net filled={}", netFilled);
 
-                            if (netFilled > 0 && _handshake.get() == Handshake.INITIAL && isOutboundDone())
+                            // Workaround for Java 11 behavior.
+                            if (netFilled < 0 && isHandshakeInitial() && BufferUtil.isEmpty(_encryptedInput))
+                                closeInbound();
+
+                            if (netFilled > 0 && !isHandshakeComplete() && isOutboundDone())
                                 throw new SSLHandshakeException("Closed during handshake");
 
+                            if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE))
+                            {
+                                if (LOG.isDebugEnabled())
+                                    LOG.debug("fill starting handshake {}", SslConnection.this);
+                            }
+
                             // Let's unwrap even if we have no net data because in that
                             // case we want to fall through to the handshake handling
                             int pos = BufferUtil.flipToFill(appIn);
@@ -803,7 +839,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
 
         private void handshakeSucceeded() throws SSLException
         {
-            if (_handshake.compareAndSet(Handshake.INITIAL, Handshake.SUCCEEDED))
+            if (_handshake.compareAndSet(HandshakeState.HANDSHAKE, HandshakeState.SUCCEEDED))
             {
                 if (LOG.isDebugEnabled())
                     LOG.debug("handshake succeeded {} {} {}/{}", SslConnection.this,
@@ -811,7 +847,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
                         _sslEngine.getSession().getProtocol(), _sslEngine.getSession().getCipherSuite());
                 notifyHandshakeSucceeded(_sslEngine);
             }
-            else if (_handshake.get() == Handshake.SUCCEEDED)
+            else if (isHandshakeSucceeded())
             {
                 if (_renegotiationLimit > 0)
                     _renegotiationLimit--;
@@ -820,7 +856,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
 
         private void handshakeFailed(Throwable failure)
         {
-            if (_handshake.compareAndSet(Handshake.INITIAL, Handshake.FAILED))
+            if (_handshake.compareAndSet(HandshakeState.HANDSHAKE, HandshakeState.FAILED))
             {
                 if (LOG.isDebugEnabled())
                     LOG.debug("handshake failed {} {}", SslConnection.this, failure);
@@ -852,7 +888,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
             }
             catch (SSLException x)
             {
-                if (handshakeStatus == HandshakeStatus.NOT_HANDSHAKING && !isAllowMissingCloseMessage())
+                if (handshakeStatus == HandshakeStatus.NOT_HANDSHAKING && isRequireCloseMessage())
                     throw x;
                 LOG.ignore(x);
                 return x;
@@ -882,7 +918,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
                     }
 
                     // finish of any previous flushes
-                    if (BufferUtil.hasContent(_encryptedOutput) && !getEndPoint().flush(_encryptedOutput))
+                    if (BufferUtil.hasContent(_encryptedOutput) && !networkFlush(_encryptedOutput))
                         return false;
 
                     boolean isEmpty = BufferUtil.isEmpty(appOuts);
@@ -910,6 +946,9 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
                                     continue;
 
                                 case NEED_UNWRAP:
+                                    // Workaround for Java 11 behavior.
+                                    if (isHandshakeInitial() && isOutboundDone())
+                                        break;
                                     if (_fillState == FillState.IDLE)
                                     {
                                         int filled = fill(BufferUtil.EMPTY_BUFFER);
@@ -927,6 +966,12 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
                             if (_encryptedOutput == null)
                                 _encryptedOutput = _bufferPool.acquire(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers);
 
+                            if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE))
+                            {
+                                if (LOG.isDebugEnabled())
+                                    LOG.debug("flush starting handshake {}", SslConnection.this);
+                            }
+
                             // We call sslEngine.wrap to try to take bytes from appOut buffers and encrypt them into the _netOut buffer
                             BufferUtil.compact(_encryptedOutput);
                             int pos = BufferUtil.flipToFill(_encryptedOutput);
@@ -952,7 +997,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
                             // if we have net bytes, let's try to flush them
                             boolean flushed = true;
                             if (BufferUtil.hasContent(_encryptedOutput))
-                                flushed = getEndPoint().flush(_encryptedOutput);
+                                flushed = networkFlush(_encryptedOutput);
 
                             if (LOG.isDebugEnabled())
                                 LOG.debug("net flushed={}, ac={}", flushed, isEmpty);
@@ -1291,7 +1336,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
 
         private boolean isRenegotiating()
         {
-            if (_handshake.get() == Handshake.INITIAL)
+            if (!isHandshakeComplete())
                 return false;
             if (isTLS13())
                 return false;