From 0754dac14d0755850772132bb4487617d79683e3 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 5 Nov 2020 00:07:28 -0800 Subject: [PATCH] codec-http2: Correct last-stream-id for HEADERS-triggered connection error (#10775) Motivation: When parsing HEADERS, connection errors can occur (e.g., too large of headers, such that we don't want to HPACK decode them). These trigger a GOAWAY with a last-stream-id telling the client which streams haven't been processed. Unfortunately that last-stream-id didn't include the stream for the HEADERS that triggered the error. Since clients are free to silently retry streams not included in last-stream-id, the client is free to retransmit the request on a new connection, which will fail the connection with the wrong last-stream-id, and the client is still free to retransmit the request. Modifications: Have fatal connection errors (those that hard-cut the connection) include all streams in last-stream-id, which guarantees the HEADERS' stream is included and thus should not be silently retried by the HTTP/2 client. This modification is heavy-handed, as it will cause racing streams to also fail, but alternatives that provide precise last-stream-id tracking are much more invasive. Hard-cutting the connection is already heavy-handed and so is rare. Result: Fixes #10670 --- .../codec/http2/Http2ConnectionHandler.java | 11 ++++++++++- .../codec/http2/Http2ConnectionHandlerTest.java | 15 +++++++++------ .../http2/Http2ControlFrameLimitEncoderTest.java | 2 +- 3 files changed, 20 insertions(+), 8 deletions(-) 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 47d511e12c..af4c507266 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 @@ -856,7 +856,16 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http */ private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause, ChannelPromise promise) { long errorCode = cause != null ? cause.error().code() : NO_ERROR.code(); - int lastKnownStream = connection().remote().lastStreamCreated(); + int lastKnownStream; + if (cause != null && cause.shutdownHint() == Http2Exception.ShutdownHint.HARD_SHUTDOWN) { + // The hard shutdown could have been triggered during header processing, before updating + // lastStreamCreated(). Specifically, any connection errors encountered by Http2FrameReader or HPACK + // decoding will fail to update the last known stream. So we must be pessimistic. + // https://github.com/netty/netty/issues/10670 + lastKnownStream = Integer.MAX_VALUE; + } else { + lastKnownStream = connection().remote().lastStreamCreated(); + } return goAway(ctx, lastKnownStream, errorCode, Http2CodecUtil.toByteBuf(ctx, cause), promise); } 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 57c639e28e..058814f95e 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 @@ -290,8 +290,8 @@ public class Http2ConnectionHandlerTest { handler = newHandler(); handler.channelRead(ctx, copiedBuffer("BAD_PREFACE", UTF_8)); ArgumentCaptor captor = ArgumentCaptor.forClass(ByteBuf.class); - verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class), eq(0), eq(PROTOCOL_ERROR.code()), - captor.capture(), eq(promise)); + verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class), + eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()), captor.capture(), eq(promise)); assertEquals(0, captor.getValue().refCnt()); } @@ -301,8 +301,8 @@ public class Http2ConnectionHandlerTest { handler = newHandler(); handler.channelRead(ctx, copiedBuffer("GET /path HTTP/1.1", US_ASCII)); ArgumentCaptor captor = ArgumentCaptor.forClass(ByteBuf.class); - verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class), eq(0), eq(PROTOCOL_ERROR.code()), - captor.capture(), eq(promise)); + verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class), eq(Integer.MAX_VALUE), + eq(PROTOCOL_ERROR.code()), captor.capture(), eq(promise)); assertEquals(0, captor.getValue().refCnt()); assertTrue(goAwayDebugCap.contains("/path")); } @@ -318,7 +318,7 @@ public class Http2ConnectionHandlerTest { handler.channelRead(ctx, buf); ArgumentCaptor captor = ArgumentCaptor.forClass(ByteBuf.class); verify(frameWriter, atLeastOnce()).writeGoAway(any(ChannelHandlerContext.class), - eq(0), eq(PROTOCOL_ERROR.code()), captor.capture(), eq(promise)); + eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()), captor.capture(), eq(promise)); assertEquals(0, captor.getValue().refCnt()); } @@ -365,10 +365,13 @@ public class Http2ConnectionHandlerTest { public void connectionErrorShouldStartShutdown() throws Exception { handler = newHandler(); Http2Exception e = new Http2Exception(PROTOCOL_ERROR); + // There's no guarantee that lastStreamCreated in correct, as the error could have occurred during header + // processing before it was updated. Thus, it should _not_ be used for the GOAWAY. + // https://github.com/netty/netty/issues/10670 when(remote.lastStreamCreated()).thenReturn(STREAM_ID); handler.exceptionCaught(ctx, e); ArgumentCaptor captor = ArgumentCaptor.forClass(ByteBuf.class); - verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID), eq(PROTOCOL_ERROR.code()), + verify(frameWriter).writeGoAway(eq(ctx), eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()), captor.capture(), eq(promise)); captor.getValue().release(); } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java index cb019ac02b..a68739e896 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java @@ -244,7 +244,7 @@ public class Http2ControlFrameLimitEncoderTest { verify(ctx, atLeast(invocations)).flush(); verify(ctx, times(invocations)).close(); if (failed) { - verify(writer, times(1)).writeGoAway(eq(ctx), eq(0), eq(ENHANCE_YOUR_CALM.code()), + verify(writer, times(1)).writeGoAway(eq(ctx), eq(Integer.MAX_VALUE), eq(ENHANCE_YOUR_CALM.code()), any(ByteBuf.class), any(ChannelPromise.class)); } }