Repository: netty/netty
Issue: HttpServerUpgradeHandler causing StringIndexOutOfBoundsException

We are using netty as a proxy server. And added HttpServerUpgradeHandler incase client wants to talk in h2c.
But with a normal http1 request, we see the following exceptions.

```io.netty.handler.codec.DecoderException: java.lang.StringIndexOutOfBoundsException: String index out of range: -1
	at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:98)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
	at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436)
	at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:324)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:296)
	at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:714)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:650)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:576)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:493)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.StringIndexOutOfBoundsException: String index out of range: -1
	at java.base/java.lang.AbstractStringBuilder.setLength(AbstractStringBuilder.java:275)
	at java.base/java.lang.StringBuilder.setLength(StringBuilder.java:85)
	at io.netty.handler.codec.http.HttpServerUpgradeHandler.upgrade(HttpServerUpgradeHandler.java:297)
	at io.netty.handler.codec.http.HttpServerUpgradeHandler.decode(HttpServerUpgradeHandler.java:239)
	at io.netty.handler.codec.http.HttpServerUpgradeHandler.decode(HttpServerUpgradeHandler.java:40)
	at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:88)
	... 22 more
```



As far as I can see, it seems like the if conditon should also add isEmpty().
```        if (connectionHeaderValues == null || connectionHeaderValues.isEmpty()) {
            return false;
        }
```

https://github.com/netty/netty/blob/3f8dab5ad00a718dc8ad6c00e27b740783ea23c5/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java#L326

Issue comments:
Work around can be fond in #11267
Implement your own `shouldHandleUpgradeRequest`, and check `connection` header there.
@dpy1123 Yeah, a missing `isEmpty()` looks about right. Would you like to make a PR for this?

Pull request review comments:
Add a call to `ReferenceCountUtil.release(req)` to prevent the contained buffer from leaking.

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
index 88aa739b298..2aa6e157d56 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
@@ -323,7 +323,7 @@ private boolean upgrade(final ChannelHandlerContext ctx, final FullHttpRequest r
         // Make sure the CONNECTION header is present.
         List<String> connectionHeaderValues = request.headers().getAll(HttpHeaderNames.CONNECTION);
 
-        if (connectionHeaderValues == null) {
+        if (connectionHeaderValues == null || connectionHeaderValues.isEmpty()) {
             return false;
         }
 


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerUpgradeHandlerTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerUpgradeHandlerTest.java
index bc4552afb4f..a2e27e787f6 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerUpgradeHandlerTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpServerUpgradeHandlerTest.java
@@ -31,6 +31,7 @@
 import io.netty.handler.codec.http.HttpServerUpgradeHandler.UpgradeCodec;
 import io.netty.handler.codec.http.HttpServerUpgradeHandler.UpgradeCodecFactory;
 import io.netty.util.CharsetUtil;
+import io.netty.util.ReferenceCountUtil;
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -189,4 +190,42 @@ protected boolean shouldHandleUpgradeRequest(HttpRequest req) {
         assertNull(channel.readOutbound());
         assertFalse(channel.finishAndReleaseAll());
     }
+
+    @Test
+    public void upgradeFail() {
+        final HttpServerCodec httpServerCodec = new HttpServerCodec();
+        final UpgradeCodecFactory factory = new UpgradeCodecFactory() {
+            @Override
+            public UpgradeCodec newUpgradeCodec(CharSequence protocol) {
+                return new TestUpgradeCodec();
+            }
+        };
+
+        HttpServerUpgradeHandler upgradeHandler = new HttpServerUpgradeHandler(httpServerCodec, factory);
+
+        EmbeddedChannel channel = new EmbeddedChannel(httpServerCodec, upgradeHandler);
+
+        // Build a h2c upgrade request, but without connection header.
+        String upgradeString = "GET / HTTP/1.1\r\n" +
+                               "Host: example.com\r\n" +
+                               "Upgrade: h2c\r\n\r\n";
+        ByteBuf upgrade = Unpooled.copiedBuffer(upgradeString, CharsetUtil.US_ASCII);
+
+        assertTrue(channel.writeInbound(upgrade));
+        assertNotNull(channel.pipeline().get(HttpServerCodec.class));
+        assertNotNull(channel.pipeline().get(HttpServerUpgradeHandler.class)); // Should not be removed.
+        assertNull(channel.pipeline().get("marker"));
+
+        HttpRequest req = channel.readInbound();
+        assertEquals(HttpVersion.HTTP_1_1, req.protocolVersion());
+        assertTrue(req.headers().contains(HttpHeaderNames.UPGRADE, "h2c", false));
+        assertFalse(req.headers().contains(HttpHeaderNames.CONNECTION));
+        ReferenceCountUtil.release(req);
+        assertNull(channel.readInbound());
+
+        // No response should be written because we're just passing through.
+        channel.flushOutbound();
+        assertNull(channel.readOutbound());
+        assertFalse(channel.finishAndReleaseAll());
+    }
 }