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 c10cc4742d..c99af7cbff 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.NO_ERROR; 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; @@ -219,10 +222,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http @Override public void decode(ChannelHandlerContext ctx, ByteBuf in, List 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); @@ -271,12 +274,15 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http } 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); @@ -290,6 +296,28 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http 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. */ 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 7f44126aad..4dc3feb978 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.anyBoolean; 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.ChannelPromise; 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.MockitoAnnotations; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; +import java.util.List; + /** * Tests for {@link Http2ConnectionHandler} */ @@ -202,22 +203,27 @@ public class Http2ConnectionHandlerTest { } @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.>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 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.>any()); } @@ -228,6 +234,7 @@ public class Http2ConnectionHandlerTest { // 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.>any()); @@ -236,10 +243,9 @@ public class Http2ConnectionHandlerTest { 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.>any()); + verify(decoder, atLeastOnce()).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.>any()); } @Test @@ -276,7 +282,8 @@ public class Http2ConnectionHandlerTest { 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 class Http2ConnectionHandlerTest { // 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 class Http2ConnectionHandlerTest { 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 class Http2ConnectionHandlerTest { 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 class Http2ConnectionHandlerTest { 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; + } }