diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java index 4d866eb41a..70596cc533 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java @@ -771,7 +771,9 @@ public class DefaultHttp2Connection implements Http2Connection { @Override public DefaultStream reservePushStream(int streamId, Http2Stream parent) throws Http2Exception { if (parent == null) { - throw connectionError(PROTOCOL_ERROR, "Parent stream missing"); + incrementExpectedStreamId(streamId); + throw streamError(streamId, Http2Error.REFUSED_STREAM, "PUSH_PROMISE sent on unknown stream, " + + "likely discarded due to a RST_STREAM."); } if (isLocal() ? !parent.state().localSideOpen() : !parent.state().remoteSideOpen()) { throw connectionError(PROTOCOL_ERROR, "Stream %d is not open for sending push promise", parent.id()); 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 718312f02b..b1417e856d 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 @@ -501,26 +501,24 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { throw connectionError(PROTOCOL_ERROR, "A client cannot push."); } - Http2Stream parentStream = connection.stream(streamId); - - if (shouldIgnoreHeadersOrDataFrame(ctx, streamId, parentStream, "PUSH_PROMISE")) { + if (streamCreatedAfterGoAwaySent(promisedStreamId)) { return; } - if (parentStream == null) { - throw connectionError(PROTOCOL_ERROR, "Stream %d does not exist", streamId); - } + Http2Stream parentStream = connection.stream(streamId); - switch (parentStream.state()) { - case OPEN: - case HALF_CLOSED_LOCAL: - // Allowed to receive push promise in these states. - break; - default: - // Connection error. - throw connectionError(PROTOCOL_ERROR, - "Stream %d in unexpected state for receiving push promise: %s", - parentStream.id(), parentStream.state()); + if (parentStream != null) { + switch (parentStream.state()) { + case OPEN: + case HALF_CLOSED_LOCAL: + // Allowed to receive push promise in these states. + break; + default: + // Connection error. + throw connectionError(PROTOCOL_ERROR, + "Stream %d in unexpected state for receiving push promise: %s", + parentStream.id(), parentStream.state()); + } } if (!requestVerifier.isAuthoritative(ctx, headers)) { 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 17903db860..b1b1d69a2d 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 @@ -26,6 +26,7 @@ import junit.framework.AssertionFailedError; import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; +import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.mockito.invocation.InvocationOnMock; @@ -586,7 +587,7 @@ public class DefaultHttp2ConnectionDecoderTest { @Test public void pushPromiseReadAfterGoAwaySentShouldBeIgnored() throws Exception { - mockGoAwaySent(); + mockGoAwaySent(PUSH_STREAM_ID); decode().onPushPromiseRead(ctx, STREAM_ID, PUSH_STREAM_ID, EmptyHttp2Headers.INSTANCE, 0); verify(remote, never()).reservePushStream(anyInt(), any(Http2Stream.class)); verify(listener, never()).onPushPromiseRead(eq(ctx), anyInt(), anyInt(), any(Http2Headers.class), anyInt()); @@ -600,21 +601,11 @@ 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 pushPromiseReadForUnknownStreamThatNeverExistedShouldThrow() throws Exception { + @Test(expected = Http2Exception.StreamException.class) + public void pushPromiseReadForUnknownStreamShouldThrowStreamException() throws Exception { when(connection.streamMayHaveExisted(STREAM_ID)).thenReturn(false); + when(connection.remote().reservePushStream(anyInt(), ArgumentMatchers.isNull())) + .thenThrow(new Http2Exception.StreamException(PUSH_STREAM_ID, Http2Error.REFUSED_STREAM, "no stream")); when(connection.stream(STREAM_ID)).thenReturn(null); decode().onPushPromiseRead(ctx, STREAM_ID, PUSH_STREAM_ID, EmptyHttp2Headers.INSTANCE, 0); } @@ -795,8 +786,12 @@ public class DefaultHttp2ConnectionDecoderTest { } private void mockGoAwaySent() { + mockGoAwaySent(STREAM_ID); + } + + private void mockGoAwaySent(int streamId) { when(connection.goAwaySent()).thenReturn(true); - when(remote.isValidStreamId(STREAM_ID)).thenReturn(true); + when(remote.isValidStreamId(streamId)).thenReturn(true); when(remote.lastStreamKnownByPeer()).thenReturn(0); } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java index b43c000206..1b335ddec6 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java @@ -320,6 +320,18 @@ public class DefaultHttp2ConnectionTest { assertEquals(2, server.local().lastStreamCreated()); } + @Test + public void reservePushStreamForUnknownParentStreamIncrementsExpectedStreamIdAndThrows() throws Http2Exception { + try { + assertEquals(0, server.local().lastStreamCreated()); + server.local().reservePushStream(2, null); + fail("Expected StreamException"); + } catch (Http2Exception.StreamException expected) { + // Next expected stream id should still be incremented. + assertEquals(2, server.local().lastStreamCreated()); + } + } + @Test public void clientReservePushStreamShouldSucceed() throws Http2Exception { Http2Stream stream = server.remote().createStream(3, true);