From cd9761b2fbee741a42f8cb205adfed3587195c4e Mon Sep 17 00:00:00 2001 From: shorea Date: Fri, 15 Jun 2018 23:05:42 -0700 Subject: [PATCH] Issue #8025. Ignoring HEADER and DATA frames for streams that may have existed in the past. --- .../http2/DefaultHttp2ConnectionDecoder.java | 10 ++-- .../DefaultHttp2ConnectionDecoderTest.java | 56 ++++++++++++++----- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java index 09f16c0a4b..718312f02b 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java @@ -585,11 +585,11 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { ctx.channel(), frameName, streamId); return true; } - // Its possible that this frame would result in stream ID out of order creation (PROTOCOL ERROR) and its - // also possible that this frame is received on a CLOSED stream (STREAM_CLOSED after a RST_STREAM is - // sent). We don't have enough information to know for sure, so we choose the lesser of the two errors. - throw streamError(streamId, STREAM_CLOSED, "Received %s frame for an unknown stream %d", - frameName, streamId); + // If the stream could have existed in the past we assume this is a frame sent by the remote + // after a RST_STREAM has been sent. Since we don't retain metadata about streams that have been + // reset we can't know this for sure. + verifyStreamMayHaveExisted(streamId); + return true; } else if (stream.isResetSent() || streamCreatedAfterGoAwaySent(streamId)) { // If we have sent a reset stream it is assumed the stream will be closed after the write completes. // If we have not sent a reset, but the stream was created after a GoAway this is not supported by diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java index 7e87d52893..17903db860 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java @@ -272,6 +272,18 @@ public class DefaultHttp2ConnectionDecoderTest { } } + @Test(expected = Http2Exception.class) + public void dataReadForUnknownStreamThatNeverExistedShouldThrow() throws Exception { + when(connection.streamMayHaveExisted(STREAM_ID)).thenReturn(false); + when(connection.stream(STREAM_ID)).thenReturn(null); + final ByteBuf data = dummyData(); + try { + decode().onDataRead(ctx, STREAM_ID, data, 0, true); + } finally { + data.release(); + } + } + @Test public void dataReadForUnknownStreamShouldApplyFlowControl() throws Exception { when(connection.stream(STREAM_ID)).thenReturn(null); @@ -279,19 +291,15 @@ public class DefaultHttp2ConnectionDecoderTest { int padding = 10; int processedBytes = data.readableBytes() + padding; try { - try { - decode().onDataRead(ctx, STREAM_ID, data, padding, true); - fail(); - } catch (Http2Exception e) { - verify(localFlow) - .receiveFlowControlledFrame(eq((Http2Stream) null), eq(data), eq(padding), eq(true)); - verify(localFlow).consumeBytes(eq((Http2Stream) null), eq(processedBytes)); - verify(localFlow).frameWriter(any(Http2FrameWriter.class)); - verifyNoMoreInteractions(localFlow); + decode().onDataRead(ctx, STREAM_ID, data, padding, true); + verify(localFlow) + .receiveFlowControlledFrame(eq((Http2Stream) null), eq(data), eq(padding), eq(true)); + verify(localFlow).consumeBytes(eq((Http2Stream) null), eq(processedBytes)); + verify(localFlow).frameWriter(any(Http2FrameWriter.class)); + verifyNoMoreInteractions(localFlow); - // Verify that the event was absorbed and not propagated to the observer. - verify(listener, never()).onDataRead(eq(ctx), anyInt(), any(ByteBuf.class), anyInt(), anyBoolean()); - } + // Verify that the event was absorbed and not propagated to the observer. + verify(listener, never()).onDataRead(eq(ctx), anyInt(), any(ByteBuf.class), anyInt(), anyBoolean()); } finally { data.release(); } @@ -430,10 +438,15 @@ public class DefaultHttp2ConnectionDecoderTest { } } - @Test(expected = Http2Exception.class) - public void headersReadForUnknownStreamShouldThrow() throws Exception { + @Test + public void headersReadForUnknownStreamThatMayHaveExistedShouldBeIgnored() throws Exception { when(connection.stream(STREAM_ID)).thenReturn(null); decode().onHeadersRead(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, 0, false); + + // Verify that the event was absorbed and not propagated to the observer. + verify(listener, never()).onHeadersRead(eq(ctx), anyInt(), any(Http2Headers.class), anyInt(), anyBoolean()); + verify(remote, never()).createStream(anyInt(), anyBoolean()); + verify(stream, never()).open(anyBoolean()); } @Test @@ -587,8 +600,21 @@ public class DefaultHttp2ConnectionDecoderTest { verify(listener).onPushPromiseRead(eq(ctx), anyInt(), anyInt(), any(Http2Headers.class), anyInt()); } + @Test + public void pushPromiseReadForUnknownStreamThatMayHaveExistedShouldBeIgnored() throws Exception { + when(connection.stream(STREAM_ID)).thenReturn(null); + decode().onPushPromiseRead(ctx, STREAM_ID, PUSH_STREAM_ID, EmptyHttp2Headers.INSTANCE, 0); + + // Verify that the event was absorbed and not propagated to the observer. + verify(listener, never()).onPushPromiseRead(eq(ctx), anyInt(), anyInt(), + any(Http2Headers.class), anyInt()); + verify(remote, never()).createStream(anyInt(), anyBoolean()); + verify(stream, never()).open(anyBoolean()); + } + @Test(expected = Http2Exception.class) - public void pushPromiseReadForUnknownStreamShouldThrow() throws Exception { + public void pushPromiseReadForUnknownStreamThatNeverExistedShouldThrow() throws Exception { + when(connection.streamMayHaveExisted(STREAM_ID)).thenReturn(false); when(connection.stream(STREAM_ID)).thenReturn(null); decode().onPushPromiseRead(ctx, STREAM_ID, PUSH_STREAM_ID, EmptyHttp2Headers.INSTANCE, 0); }