From 93306310972cb36c8cb04ac0bb4e3b9ef58c410d Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 17 Mar 2016 21:17:36 +0100 Subject: [PATCH] Ensure all pending SSL data is written before closing channel during handshake error. Motivation: We need to ensure we call ctx.flush() before closing the actual channel when an handshake failure took place. If we miss to do so we may not send all pending data to the remote peer which also include SSL alerts. Modifications: Ensure we call ctx.flush() before ctx.close() on a handshake error. Result: All pending data (including SSL alerts) are written to the remote peer on a handshake error. --- .../main/java/io/netty/handler/ssl/SslHandler.java | 5 +---- .../src/main/java/io/netty/handler/ssl/SslUtils.java | 3 +++ .../java/io/netty/handler/ssl/SniHandlerTest.java | 11 ++++++++++- 3 files changed, 14 insertions(+), 5 deletions(-) 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 b5a1360a11..7bcc6255fb 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -1342,12 +1342,9 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH try { engine.beginHandshake(); wrapNonAppData(ctx, false); + ctx.flush(); } catch (Exception e) { notifyHandshakeFailure(e); - } finally { - // 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(); } // Set timeout if necessary. 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 6bf75d38d1..a70c00c2e0 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java @@ -118,6 +118,9 @@ final class SslUtils { } static void notifyHandshakeFailure(ChannelHandlerContext ctx, Throwable cause) { + // 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(); ctx.fireUserEventTriggered(new SslHandshakeCompletionEvent(cause)); ctx.close(); } diff --git a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java index ea28b412b5..a6ae52345f 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java @@ -20,6 +20,7 @@ import io.netty.buffer.Unpooled; import io.netty.channel.embedded.EmbeddedChannel; import io.netty.handler.codec.DecoderException; import io.netty.util.DomainNameMapping; +import io.netty.util.ReferenceCountUtil; import org.junit.Test; import javax.xml.bind.DatatypeConverter; @@ -76,9 +77,17 @@ public class SniHandlerTest { // expected } - assertThat(ch.finish(), is(false)); + assertThat(ch.finish(), is(true)); assertThat(handler.hostname(), is("chat4.leancloud.cn")); assertThat(handler.sslContext(), is(leanContext)); + + for (;;) { + Object msg = ch.readOutbound(); + if (msg == null) { + break; + } + ReferenceCountUtil.release(msg); + } } @Test