Repository: netty/netty
Issue: SSL connection not closed properly after handshake failure in v4.1.21
Labels: defect

### Expected behavior
When SSL handshake fails, the connection is closed.

### Actual behavior
Even though the SSL engine is closed, the connection itself is not closed.

### Steps to reproduce
See below.

### Minimal yet complete reproducer code (or URL to code)
Run the demo code [here](https://github.com/jianglai/netty-bug/blob/master/src/main/java/test/Main.java). The program hangs and "Channel closed." is never printed to stdout. If you change the Netty version in the pom.xml file to 4.1.17, the channel is closed as expected.

### Netty version
4.1.21

### JVM version (e.g. `java -version`)
openjdk version "1.8.0_112-google-v7"
OpenJDK Runtime Environment (build 1.8.0_112-google-v7-181886826-181741640)
OpenJDK 64-Bit Server VM (build 25.112-b01, mixed mode)

### OS version (e.g. `uname -a`)
Linux 4.9.0-5-amd64 SMP Debian 4.9.65-3+deb9u2 (2018-01-04) x86_64 GNU/Linux

I think this is a regression introduced by https://github.com/netty/netty/commit/978a46cc0a5242702f248842f6feb97b8fbe0123.

More specifically, [`handshakePromise.tryFailure`](https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L1183)  is called within the `try` clause, and the promise is set to failure. Later in the `finally` clause, [`setHandshakeFailure`](https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L1529) calls [`notifyHandshakeFailure`](https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L1542]), which is no longer calling [`SslUtils.notifyHandshakeFailure`](https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslUtils.java#L320) because the promise has been marked as finished. Therefore `ctx.close()` is never called to close the channel.

Issue comments:
@jianglai thanks I will check.
@jianglai should be fixed by https://github.com/netty/netty/pull/7727.
Thanks for the quick turnaround!
@jianglai no problem... would be nice if you can verify the fix
fixed by #7727
Confirmed that with 4.1.22.Final-SNAPSHOT, the demo code works as intended.

Thanks!
@jianglai thanks a lot !

Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java b/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java
index ecbf8263c18..05fcaffdfc3 100644
--- a/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java
@@ -81,7 +81,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
                                         "not an SSL/TLS record: " + ByteBufUtil.hexDump(in));
                                 in.skipBytes(in.readableBytes());
                                 ctx.fireUserEventTriggered(new SniCompletionEvent(e));
-                                SslUtils.notifyHandshakeFailure(ctx, e, true);
+                                SslUtils.handleHandshakeFailure(ctx, e, true);
                                 throw e;
                             }
                             if (len == SslUtils.NOT_ENOUGH_DATA ||
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
index a5ebc363f37..1f22943caf4 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -865,7 +865,7 @@ private void finishWrap(ChannelHandlerContext ctx, ByteBuf out, ChannelPromise p
 
     /**
      * This method will not call
-     * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable, boolean, boolean)} or
+     * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable, boolean, boolean, boolean)} or
      * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable)}.
      * @return {@code true} if this method ends on {@link SSLEngineResult.HandshakeStatus#NOT_HANDSHAKING}.
      */
@@ -1002,7 +1002,7 @@ private SSLEngineResult wrap(ByteBufAllocator alloc, SSLEngine engine, ByteBuf i
     public void channelInactive(ChannelHandlerContext ctx) throws Exception {
         // Make sure to release SSLEngine,
         // and notify the handshake future if the connection has been closed during handshake.
-        setHandshakeFailure(ctx, CHANNEL_CLOSED, !outboundClosed, handshakeStarted);
+        setHandshakeFailure(ctx, CHANNEL_CLOSED, !outboundClosed, handshakeStarted, false);
 
         // Ensure we always notify the sslClosePromise as well
         notifyClosePromise(CHANNEL_CLOSED);
@@ -1191,7 +1191,8 @@ private void handleUnwrapThrowable(ChannelHandlerContext ctx, Throwable cause) {
             logger.debug("SSLException during trying to call SSLEngine.wrap(...)" +
                     " because of an previous SSLException, ignoring...", ex);
         } finally {
-            setHandshakeFailure(ctx, cause, true, false);
+            // ensure we always flush and close the channel.
+            setHandshakeFailure(ctx, cause, true, false, true);
         }
         PlatformDependent.throwException(cause);
     }
@@ -1498,13 +1499,14 @@ private void setHandshakeSuccess() {
      * Notify all the handshake futures about the failure during the handshake.
      */
     private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause) {
-        setHandshakeFailure(ctx, cause, true, true);
+        setHandshakeFailure(ctx, cause, true, true, false);
     }
 
     /**
      * Notify all the handshake futures about the failure during the handshake.
      */
-    private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean closeInbound, boolean notify) {
+    private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean closeInbound,
+                                     boolean notify, boolean alwaysFlushAndClose) {
         try {
             // Release all resources such as internal buffers that SSLEngine
             // is managing.
@@ -1526,7 +1528,9 @@ private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boo
                     }
                 }
             }
-            notifyHandshakeFailure(cause, notify);
+            if (handshakePromise.tryFailure(cause) || alwaysFlushAndClose) {
+                SslUtils.handleHandshakeFailure(ctx, cause, notify);
+            }
         } finally {
             // Ensure we remove and fail all pending writes in all cases and so release memory quickly.
             releaseAndFailAll(cause);
@@ -1539,12 +1543,6 @@ private void releaseAndFailAll(Throwable cause) {
         }
     }
 
-    private void notifyHandshakeFailure(Throwable cause, boolean notify) {
-        if (handshakePromise.tryFailure(cause)) {
-            SslUtils.notifyHandshakeFailure(ctx, cause, notify);
-        }
-    }
-
     private void notifyClosePromise(Throwable cause) {
         if (cause == null) {
             if (sslClosePromise.trySuccess(ctx.channel())) {
@@ -1725,7 +1723,9 @@ public void run() {
                     return;
                 }
                 try {
-                    notifyHandshakeFailure(HANDSHAKE_TIMED_OUT, true);
+                    if (handshakePromise.tryFailure(HANDSHAKE_TIMED_OUT)) {
+                        SslUtils.handleHandshakeFailure(ctx, HANDSHAKE_TIMED_OUT, true);
+                    }
                 } finally {
                     releaseAndFailAll(HANDSHAKE_TIMED_OUT);
                 }
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java
index 175238d41ed..3dd40fdf37b 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java
@@ -310,7 +310,7 @@ private static int getEncryptedPacketLength(ByteBuffer buffer) {
         return packetLength;
     }
 
-    static void notifyHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean notify) {
+    static void handleHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean notify) {
         // We have may haven written some parts of data before an exception was thrown so ensure we always flush.
         // See https://github.com/netty/netty/issues/3900#issuecomment-172481830
         ctx.flush();


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
index 5366de289f7..f3c31ed6dd4 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -599,4 +599,58 @@ protected void initChannel(Channel ch) throws Exception {
             ReferenceCountUtil.release(sslClientCtx);
         }
     }
+
+    @Test(timeout = 10000)
+    public void testCloseOnHandshakeFailure() throws Exception {
+        final SelfSignedCertificate ssc = new SelfSignedCertificate();
+
+        final SslContext sslServerCtx = SslContextBuilder.forServer(ssc.key(), ssc.cert()).build();
+        final SslContext sslClientCtx = SslContextBuilder.forClient()
+                .trustManager(new SelfSignedCertificate().cert())
+                .build();
+
+        EventLoopGroup group = new NioEventLoopGroup(1);
+        Channel sc = null;
+        Channel cc = null;
+        try {
+            LocalAddress address = new LocalAddress(getClass().getSimpleName() + ".testCloseOnHandshakeFailure");
+            ServerBootstrap sb = new ServerBootstrap()
+                    .group(group)
+                    .channel(LocalServerChannel.class)
+                    .childHandler(new ChannelInitializer<Channel>() {
+                        @Override
+                        protected void initChannel(Channel ch) {
+                            ch.pipeline().addLast(sslServerCtx.newHandler(ch.alloc()));
+                        }
+                    });
+            sc = sb.bind(address).syncUninterruptibly().channel();
+
+            Bootstrap b = new Bootstrap()
+                    .group(group)
+                    .channel(LocalChannel.class)
+                    .handler(new ChannelInitializer<Channel>() {
+                        @Override
+                        protected void initChannel(Channel ch) {
+                            ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc()));
+                        }
+                    });
+            cc = b.connect(sc.localAddress()).syncUninterruptibly().channel();
+            SslHandler handler = cc.pipeline().get(SslHandler.class);
+            handler.handshakeFuture().awaitUninterruptibly();
+            assertFalse(handler.handshakeFuture().isSuccess());
+
+            cc.closeFuture().syncUninterruptibly();
+        } finally {
+            if (cc != null) {
+                cc.close().syncUninterruptibly();
+            }
+            if (sc != null) {
+                sc.close().syncUninterruptibly();
+            }
+            group.shutdownGracefully();
+
+            ReferenceCountUtil.release(sslServerCtx);
+            ReferenceCountUtil.release(sslClientCtx);
+        }
+    }
 }