From c423891fb51aaa4e9063c53be7e4089c70d1937b Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Mon, 26 Jul 2021 20:20:40 +0200
Subject: [PATCH] Remove ApplicationProtocolNegotiationHandler when no
 SslHandler is present (#11503)

Motivation:

750d235 did introduce a change in ApplicationProtocolNegotiationHandler that let the handler buffer all inbound data until the SSL handshake was completed. Due of this change a user might get buffer data forever if the SslHandler is not in the pipeline but the ApplicationProtocolNegotiationHandler was added. We should better guard the user against this "missconfiguration" of the ChannelPipeline.

Modifications:

- Remove the ApplicationProtocolNegotiationHandler when no SslHandler is present when the first message was received
- Add unit test

Result:

No possible that we buffer forever
---
 ...ApplicationProtocolNegotiationHandler.java |  9 ++++
 ...icationProtocolNegotiationHandlerTest.java | 45 ++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java b/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java
index f079aac2d1..29bf32c8c4 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java
@@ -72,6 +72,7 @@ public abstract class ApplicationProtocolNegotiationHandler extends ChannelInbou
     private final String fallbackProtocol;
     private final RecyclableArrayList bufferedMessages = RecyclableArrayList.newInstance();
     private ChannelHandlerContext ctx;
+    private boolean sslHandlerChecked;
 
     /**
      * Creates a new instance with the specified fallback protocol name.
@@ -100,6 +101,14 @@ public abstract class ApplicationProtocolNegotiationHandler extends ChannelInbou
     public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
         // Let's buffer all data until this handler will be removed from the pipeline.
         bufferedMessages.add(msg);
+        if (!sslHandlerChecked) {
+            sslHandlerChecked = true;
+            if (ctx.pipeline().get(SslHandler.class) == null) {
+                // Just remove ourself if there is no SslHandler in the pipeline and so we would otherwise
+                // buffer forever.
+                removeSelfIfPresent(ctx);
+            }
+        }
     }
 
     /**
diff --git a/handler/src/test/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandlerTest.java
index 406d84c341..66a20b3af3 100644
--- a/handler/src/test/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandlerTest.java
@@ -43,6 +43,33 @@ import static org.junit.jupiter.api.Assertions.fail;
 
 public class ApplicationProtocolNegotiationHandlerTest {
 
+    @Test
+    public void testRemoveItselfIfNoSslHandlerPresent() throws NoSuchAlgorithmException {
+        ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) {
+            @Override
+            protected void configurePipeline(ChannelHandlerContext ctx, String protocol) {
+                fail();
+            }
+        };
+
+        SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        // This test is mocked/simulated and doesn't go through full TLS handshake. Currently only JDK SSLEngineImpl
+        // client mode will generate a close_notify.
+        engine.setUseClientMode(true);
+
+        EmbeddedChannel channel = new EmbeddedChannel(alpnHandler);
+        String msg = "msg";
+        String msg2 = "msg2";
+
+        assertTrue(channel.writeInbound(msg));
+        assertTrue(channel.writeInbound(msg2));
+        assertNull(channel.pipeline().context(alpnHandler));
+        assertEquals(msg, channel.readInbound());
+        assertEquals(msg2, channel.readInbound());
+
+        assertFalse(channel.finishAndReleaseAll());
+    }
+
     @Test
     public void testHandshakeFailure() {
         ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) {
@@ -63,6 +90,15 @@ public class ApplicationProtocolNegotiationHandlerTest {
 
     @Test
     public void testHandshakeSuccess() throws NoSuchAlgorithmException {
+        testHandshakeSuccess0(false);
+    }
+
+    @Test
+    public void testHandshakeSuccessWithSslHandlerAddedLater() throws NoSuchAlgorithmException {
+        testHandshakeSuccess0(true);
+    }
+
+    private static void testHandshakeSuccess0(boolean addLater) throws NoSuchAlgorithmException {
         final AtomicBoolean configureCalled = new AtomicBoolean(false);
         ChannelHandler alpnHandler = new ApplicationProtocolNegotiationHandler(ApplicationProtocolNames.HTTP_1_1) {
             @Override
@@ -77,7 +113,14 @@ public class ApplicationProtocolNegotiationHandlerTest {
         // client mode will generate a close_notify.
         engine.setUseClientMode(true);
 
-        EmbeddedChannel channel = new EmbeddedChannel(new SslHandler(engine), alpnHandler);
+        EmbeddedChannel channel = new EmbeddedChannel();
+        if (addLater) {
+            channel.pipeline().addLast(alpnHandler);
+            channel.pipeline().addFirst(new SslHandler(engine));
+        } else {
+            channel.pipeline().addLast(new SslHandler(engine));
+            channel.pipeline().addLast(alpnHandler);
+        }
         channel.pipeline().fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS);
         assertNull(channel.pipeline().context(alpnHandler));
         // Should produce the close_notify messages