Repository: netty/netty
Issue: Need better error when first HTTP/2 frame is not SETTINGS
Labels: improvement

Right now if the protocol upgrade to HTTP/2 fails, the server may send an HTTP 1.1 response leading to a frame parsing error at the client (due to frame length). It would be helpful for receipt of the first frame to verify that the frame type is SETTINGS, if not fail with a better error.


Issue comments:

Pull request review comments:
Can we move this logic into `PrefaceDecoder`?  One of the reasons why `FrameDecoder` was introduced was to avoid having a conditional statement evaluate on every `decode` which will only be relevant for the first (or first couple) decode(s).

I think my previous comment still applies https://github.com/netty/netty/pull/3894#discussion_r32673264. Can we move to `PrefaceDecoder`?

Agreed ... moving back into PrefaceDecoder.

can we hexDump without creating a slice and so reduce object allocation ?

Like here: https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java#L149

+1. This would apply to the other call to `hexDump` too I think.

done.


Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
index 4ce96fd0daa..042912eb6d2 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
@@ -14,6 +14,7 @@
  */
 package io.netty.handler.codec.http2;
 
+import static io.netty.buffer.ByteBufUtil.hexDump;
 import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_STREAM_ID;
 import static io.netty.handler.codec.http2.Http2CodecUtil.connectionPrefaceBuf;
 import static io.netty.handler.codec.http2.Http2CodecUtil.getEmbeddedHttp2Exception;
@@ -22,8 +23,10 @@
 import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
 import static io.netty.handler.codec.http2.Http2Exception.isStreamError;
+import static io.netty.handler.codec.http2.Http2FrameTypes.SETTINGS;
 import static io.netty.util.CharsetUtil.UTF_8;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
+import static java.lang.Math.min;
 import static java.lang.String.format;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufUtil;
@@ -216,10 +219,10 @@ public boolean prefaceSent() {
         @Override
         public void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
             try {
-                if (readClientPrefaceString(in)) {
+                if (readClientPrefaceString(in) && verifyFirstFrameIsSettings(in)) {
                     // After the preface is read, it is time to hand over control to the post initialized decoder.
-                    Http2ConnectionHandler.this.byteDecoder = new FrameDecoder();
-                    Http2ConnectionHandler.this.byteDecoder.decode(ctx, in, out);
+                    byteDecoder = new FrameDecoder();
+                    byteDecoder.decode(ctx, in, out);
                 }
             } catch (Throwable e) {
                 onException(ctx, e);
@@ -268,12 +271,15 @@ private boolean readClientPrefaceString(ByteBuf in) throws Http2Exception {
             }
 
             int prefaceRemaining = clientPrefaceString.readableBytes();
-            int bytesRead = Math.min(in.readableBytes(), prefaceRemaining);
+            int bytesRead = min(in.readableBytes(), prefaceRemaining);
 
             // If the input so far doesn't match the preface, break the connection.
             if (bytesRead == 0 || !ByteBufUtil.equals(in, in.readerIndex(),
                     clientPrefaceString, clientPrefaceString.readerIndex(), bytesRead)) {
-                throw connectionError(PROTOCOL_ERROR, "HTTP/2 client preface string missing or corrupt.");
+                String receivedBytes = hexDump(in, in.readerIndex(),
+                        min(in.readableBytes(), clientPrefaceString.readableBytes()));
+                throw connectionError(PROTOCOL_ERROR, "HTTP/2 client preface string missing or corrupt. " +
+                        "Hex dump for received bytes: %s", receivedBytes);
             }
             in.skipBytes(bytesRead);
             clientPrefaceString.skipBytes(bytesRead);
@@ -287,6 +293,28 @@ private boolean readClientPrefaceString(ByteBuf in) throws Http2Exception {
             return false;
         }
 
+        /**
+         * Peeks at that the next frame in the buffer and verifies that it is a {@code SETTINGS} frame.
+         *
+         * @param in the inbound buffer.
+         * @return {@code} true if the next frame is a {@code SETTINGS} frame, {@code false} if more
+         * data is required before we can determine the next frame type.
+         * @throws Http2Exception thrown if the next frame is NOT a {@code SETTINGS} frame.
+         */
+        private boolean verifyFirstFrameIsSettings(ByteBuf in) throws Http2Exception {
+            if (in.readableBytes() < 4) {
+                // Need more data before we can see the frame type for the first frame.
+                return false;
+            }
+
+            byte frameType = in.getByte(in.readerIndex() + 3);
+            if (frameType != SETTINGS) {
+                throw connectionError(PROTOCOL_ERROR, "First received frame was not SETTINGS. " +
+                        "Hex dump for first 4 bytes: %s", hexDump(in, in.readerIndex(), 4));
+            }
+            return true;
+        }
+
         /**
          * Sends the HTTP/2 connection preface upon establishment of the connection, if not already sent.
          */


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
index 7f44126aadb..4dc3feb9781 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
@@ -30,12 +30,14 @@
 import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyLong;
 import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
+
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.buffer.UnpooledByteBufAllocator;
@@ -47,9 +49,6 @@
 import io.netty.channel.DefaultChannelPromise;
 import io.netty.util.CharsetUtil;
 import io.netty.util.concurrent.GenericFutureListener;
-
-import java.util.List;
-
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -60,6 +59,8 @@
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
 
+import java.util.List;
+
 /**
  * Tests for {@link Http2ConnectionHandler}
  */
@@ -202,22 +203,27 @@ public void serverReceivingInvalidClientPrefaceStringShouldHandleException() thr
     }
 
     @Test
-    public void serverReceivingValidClientPrefaceStringShouldContinueReadingFrames() throws Exception {
+    public void serverReceivingClientPrefaceStringFollowedByNonSettingsShouldHandleException()
+            throws Exception {
         when(connection.isServer()).thenReturn(true);
         handler = newHandler();
-        ByteBuf preface = connectionPrefaceBuf();
-        ByteBuf prefacePlusSome = Unpooled.wrappedBuffer(new byte[preface.readableBytes() + 1]);
-        prefacePlusSome.resetWriterIndex().writeBytes(preface).writeByte(0);
-        handler.channelRead(ctx, prefacePlusSome);
-        verify(decoder, times(2)).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.<List<Object>>any());
+
+        // Create a connection preface followed by a bunch of zeros (i.e. not a settings frame).
+        ByteBuf buf = Unpooled.buffer().writeBytes(connectionPrefaceBuf()).writeZero(10);
+        handler.channelRead(ctx, buf);
+        ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
+        verify(frameWriter, atLeastOnce()).writeGoAway(eq(ctx), eq(0), eq(PROTOCOL_ERROR.code()),
+                captor.capture(), eq(promise));
+        assertEquals(0, captor.getValue().refCnt());
     }
 
     @Test
-    public void serverReceivingValidClientPrefaceStringShouldOnlyReadWholeFrame() throws Exception {
+    public void serverReceivingValidClientPrefaceStringShouldContinueReadingFrames() throws Exception {
         when(connection.isServer()).thenReturn(true);
         handler = newHandler();
-        handler.channelRead(ctx, connectionPrefaceBuf());
-        verify(decoder).decodeFrame(any(ChannelHandlerContext.class),
+        ByteBuf prefacePlusSome = addSettingsHeader(Unpooled.buffer().writeBytes(connectionPrefaceBuf()));
+        handler.channelRead(ctx, prefacePlusSome);
+        verify(decoder, atLeastOnce()).decodeFrame(any(ChannelHandlerContext.class),
                 any(ByteBuf.class), Matchers.<List<Object>>any());
     }
 
@@ -228,6 +234,7 @@ public void verifyChannelHandlerCanBeReusedInPipeline() throws Exception {
         // Only read the connection preface...after preface is read internal state of Http2ConnectionHandler
         // is expected to change relative to the pipeline.
         ByteBuf preface = connectionPrefaceBuf();
+        handler.channelRead(ctx, preface);
         verify(decoder, never()).decodeFrame(any(ChannelHandlerContext.class),
                 any(ByteBuf.class), Matchers.<List<Object>>any());
 
@@ -236,10 +243,9 @@ public void verifyChannelHandlerCanBeReusedInPipeline() throws Exception {
         handler.handlerAdded(ctx);
 
         // Now verify we can continue as normal, reading connection preface plus more.
-        ByteBuf prefacePlusSome = Unpooled.wrappedBuffer(new byte[preface.readableBytes() + 1]);
-        prefacePlusSome.resetWriterIndex().writeBytes(preface).writeByte(0);
+        ByteBuf prefacePlusSome = addSettingsHeader(Unpooled.buffer().writeBytes(connectionPrefaceBuf()));
         handler.channelRead(ctx, prefacePlusSome);
-        verify(decoder, times(2)).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.<List<Object>>any());
+        verify(decoder, atLeastOnce()).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.<List<Object>>any());
     }
 
     @Test
@@ -276,7 +282,8 @@ public void writeRstOnNonExistantStreamShouldSucceed() throws Exception {
         handler = newHandler();
         handler.resetStream(ctx, NON_EXISTANT_STREAM_ID, STREAM_CLOSED.code(), promise);
         verify(frameWriter, never())
-            .writeRstStream(any(ChannelHandlerContext.class), anyInt(), anyLong(), any(ChannelPromise.class));
+            .writeRstStream(any(ChannelHandlerContext.class), anyInt(), anyLong(),
+                    any(ChannelPromise.class));
         assertTrue(promise.isDone());
         assertTrue(promise.isSuccess());
         assertNull(promise.cause());
@@ -291,7 +298,8 @@ public void writeRstOnClosedStreamShouldSucceed() throws Exception {
         // The stream is "closed" but is still known about by the connection (connection().stream(..)
         // will return the stream). We should still write a RST_STREAM frame in this scenario.
         handler.resetStream(ctx, STREAM_ID, STREAM_CLOSED.code(), promise);
-        verify(frameWriter).writeRstStream(eq(ctx), eq(STREAM_ID), anyLong(), any(ChannelPromise.class));
+        verify(frameWriter).writeRstStream(eq(ctx), eq(STREAM_ID), anyLong(),
+                any(ChannelPromise.class));
     }
 
     @SuppressWarnings("unchecked")
@@ -346,7 +354,8 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
         handler.goAway(ctx, STREAM_ID, errorCode, data, promise);
 
         verify(connection).goAwaySent(eq(STREAM_ID), eq(errorCode), eq(data));
-        verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID), eq(errorCode), eq(data), eq(promise));
+        verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID), eq(errorCode), eq(data),
+                eq(promise));
         verify(ctx).close();
         assertEquals(0, data.refCnt());
     }
@@ -358,7 +367,8 @@ public void canSendGoAwayFramesWithDecreasingLastStreamIds() throws Exception {
         long errorCode = Http2Error.INTERNAL_ERROR.code();
 
         handler.goAway(ctx, STREAM_ID + 2, errorCode, data.retain(), promise);
-        verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID + 2), eq(errorCode), eq(data), eq(promise));
+        verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID + 2), eq(errorCode), eq(data),
+                eq(promise));
         verify(connection).goAwaySent(eq(STREAM_ID + 2), eq(errorCode), eq(data));
         promise = new DefaultChannelPromise(channel);
         handler.goAway(ctx, STREAM_ID, errorCode, data, promise);
@@ -398,4 +408,12 @@ public void channelReadCompleteTriggersFlush() throws Exception {
     private ByteBuf dummyData() {
         return Unpooled.buffer().writeBytes("abcdefgh".getBytes(CharsetUtil.UTF_8));
     }
+
+    private ByteBuf addSettingsHeader(ByteBuf buf) {
+        buf.writeMedium(Http2CodecUtil.SETTING_ENTRY_LENGTH);
+        buf.writeByte(Http2FrameTypes.SETTINGS);
+        buf.writeByte(0);
+        buf.writeInt(0);
+        return buf;
+    }
 }