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
This commit is contained in:
Eric Anderson 2020-11-05 00:07:28 -08:00 committed by Norman Maurer
parent f053870f38
commit 0754dac14d
3 changed files with 20 additions and 8 deletions

View File

@ -856,7 +856,16 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
*/ */
private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause, ChannelPromise promise) { private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause, ChannelPromise promise) {
long errorCode = cause != null ? cause.error().code() : NO_ERROR.code(); 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); return goAway(ctx, lastKnownStream, errorCode, Http2CodecUtil.toByteBuf(ctx, cause), promise);
} }

View File

@ -290,8 +290,8 @@ public class Http2ConnectionHandlerTest {
handler = newHandler(); handler = newHandler();
handler.channelRead(ctx, copiedBuffer("BAD_PREFACE", UTF_8)); handler.channelRead(ctx, copiedBuffer("BAD_PREFACE", UTF_8));
ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class); ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class), eq(0), eq(PROTOCOL_ERROR.code()), verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class),
captor.capture(), eq(promise)); eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()), captor.capture(), eq(promise));
assertEquals(0, captor.getValue().refCnt()); assertEquals(0, captor.getValue().refCnt());
} }
@ -301,8 +301,8 @@ public class Http2ConnectionHandlerTest {
handler = newHandler(); handler = newHandler();
handler.channelRead(ctx, copiedBuffer("GET /path HTTP/1.1", US_ASCII)); handler.channelRead(ctx, copiedBuffer("GET /path HTTP/1.1", US_ASCII));
ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class); ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class), eq(0), eq(PROTOCOL_ERROR.code()), verify(frameWriter).writeGoAway(any(ChannelHandlerContext.class), eq(Integer.MAX_VALUE),
captor.capture(), eq(promise)); eq(PROTOCOL_ERROR.code()), captor.capture(), eq(promise));
assertEquals(0, captor.getValue().refCnt()); assertEquals(0, captor.getValue().refCnt());
assertTrue(goAwayDebugCap.contains("/path")); assertTrue(goAwayDebugCap.contains("/path"));
} }
@ -318,7 +318,7 @@ public class Http2ConnectionHandlerTest {
handler.channelRead(ctx, buf); handler.channelRead(ctx, buf);
ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class); ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
verify(frameWriter, atLeastOnce()).writeGoAway(any(ChannelHandlerContext.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()); assertEquals(0, captor.getValue().refCnt());
} }
@ -365,10 +365,13 @@ public class Http2ConnectionHandlerTest {
public void connectionErrorShouldStartShutdown() throws Exception { public void connectionErrorShouldStartShutdown() throws Exception {
handler = newHandler(); handler = newHandler();
Http2Exception e = new Http2Exception(PROTOCOL_ERROR); 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); when(remote.lastStreamCreated()).thenReturn(STREAM_ID);
handler.exceptionCaught(ctx, e); handler.exceptionCaught(ctx, e);
ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class); ArgumentCaptor<ByteBuf> 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.capture(), eq(promise));
captor.getValue().release(); captor.getValue().release();
} }

View File

@ -244,7 +244,7 @@ public class Http2ControlFrameLimitEncoderTest {
verify(ctx, atLeast(invocations)).flush(); verify(ctx, atLeast(invocations)).flush();
verify(ctx, times(invocations)).close(); verify(ctx, times(invocations)).close();
if (failed) { 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)); any(ByteBuf.class), any(ChannelPromise.class));
} }
} }