From 1453f8d18b635bdd2d0ef9df9334c88c993a7092 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Tue, 5 Dec 2017 15:19:33 +0100
Subject: [PATCH] Ensure we not try to call `select` when the
 `AbstractSniHandler` was already removed from the pipeline.

Motivation:

We tried to call `select` after we closed the channel (and so removed all the handlers from the pipeline) when we detected a non SSL record. This would cause an exception like this:

```
Caused by: java.util.NoSuchElementException: io.netty.handler.ssl.SniHandler
	at io.netty.channel.DefaultChannelPipeline.getContextOrDie(DefaultChannelPipeline.java:1098)
	at io.netty.channel.DefaultChannelPipeline.replace(DefaultChannelPipeline.java:506)
	at io.netty.handler.ssl.SniHandler.replaceHandler(SniHandler.java:133)
	at io.netty.handler.ssl.SniHandler.onLookupComplete(SniHandler.java:113)
	at io.netty.handler.ssl.AbstractSniHandler.select(AbstractSniHandler.java:225)
	at io.netty.handler.ssl.AbstractSniHandler.decode(AbstractSniHandler.java:218)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
	... 40 more
```

Modifications:

- Ensure we rethrow the NotSslRecordException when detecting it (and closing the channel). This will also ensure we not call `select(...)`
- Not catch `Throwable` but only `Exception`
- Add test case.

Result:

Correctly handle the case of an non SSL record.
---
 .../netty/handler/ssl/AbstractSniHandler.java |  9 +++--
 .../io/netty/handler/ssl/SniHandlerTest.java  | 36 +++++++++++++++++++
 2 files changed, 42 insertions(+), 3 deletions(-)

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 c65eabd3d7..ecbf8263c1 100644
--- a/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java
@@ -98,7 +98,7 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
                             // SSLv3 or TLS
                             if (majorVersion == 3) {
                                 final int packetLength = in.getUnsignedShort(readerIndex + 3) +
-                                                         SslUtils.SSL_RECORD_HEADER_LENGTH;
+                                        SslUtils.SSL_RECORD_HEADER_LENGTH;
 
                                 if (readableBytes < packetLength) {
                                     // client hello incomplete; try again to decode once more data is ready.
@@ -185,7 +185,7 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
                                             }
 
                                             final String hostname = in.toString(offset, serverNameLength,
-                                                                                CharsetUtil.US_ASCII);
+                                                    CharsetUtil.US_ASCII);
 
                                             try {
                                                 select(ctx, hostname.toLowerCase(Locale.US));
@@ -208,7 +208,10 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
                             break loop;
                     }
                 }
-            } catch (Throwable e) {
+            } catch (NotSslRecordException e) {
+                // Just rethrow as in this case we also closed the channel and this is consistent with SslHandler.
+                throw e;
+            } catch (Exception e) {
                 // unexpected encoding, ignore sni and use default
                 if (logger.isDebugEnabled()) {
                     logger.debug("Unexpected client hello packet: " + ByteBufUtil.hexDump(in), e);
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 3ba1c0ca6f..a6dceb364e 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java
@@ -140,6 +140,42 @@ public class SniHandlerTest {
         this.provider = provider;
     }
 
+    @Test
+    public void testNonSslRecord() throws Exception {
+        SslContext nettyContext = makeSslContext(provider, false);
+        try {
+            final AtomicReference<SslHandshakeCompletionEvent> evtRef =
+                    new AtomicReference<SslHandshakeCompletionEvent>();
+            SniHandler handler = new SniHandler(new DomainNameMappingBuilder<SslContext>(nettyContext).build());
+            EmbeddedChannel ch = new EmbeddedChannel(handler, new ChannelInboundHandlerAdapter() {
+                @Override
+                public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
+                    if (evt instanceof SslHandshakeCompletionEvent) {
+                        assertTrue(evtRef.compareAndSet(null, (SslHandshakeCompletionEvent) evt));
+                    }
+                }
+            });
+
+            try {
+                byte[] bytes = new byte[1024];
+                bytes[0] = SslUtils.SSL_CONTENT_TYPE_ALERT;
+
+                try {
+                    ch.writeInbound(Unpooled.wrappedBuffer(bytes));
+                    fail();
+                } catch (DecoderException e) {
+                    assertTrue(e.getCause() instanceof NotSslRecordException);
+                }
+                assertFalse(ch.finish());
+            } finally {
+                ch.finishAndReleaseAll();
+            }
+            assertTrue(evtRef.get().cause() instanceof NotSslRecordException);
+        } finally {
+            releaseAll(nettyContext);
+        }
+    }
+
     @Test
     public void testServerNameParsing() throws Exception {
         SslContext nettyContext = makeSslContext(provider, false);