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

handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java

@@ -73,6 +73,7 @@ public abstract class ApplicationProtocolNegotiationHandler implements ChannelHa
     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.
@@ -99,6 +100,14 @@ public abstract class ApplicationProtocolNegotiationHandler implements ChannelHa
     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);
+            }
+        }
     }
 
     /**

handler/src/test/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandlerTest.java

@@ -42,6 +42,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) {
@@ -62,6 +89,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
@@ -76,7 +112,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