From d6829bd98b35b77d13d73e7920c8de76620d616e Mon Sep 17 00:00:00 2001 From: Geoff Kizer Date: Wed, 26 Jun 2019 01:47:41 -0700 Subject: [PATCH] ignore invalid frames on closed streams Commit migrated from https://github.com/dotnet/corefx/commit/10ae10869a86e0ad9dc2d789b2da0c9b9acec2c1 --- .../System/Net/Http/Http2LoopbackConnection.cs | 6 +- .../Net/Http/SocketsHttpHandler/Http2Connection.cs | 44 +++++------ .../FunctionalTests/HttpClientHandlerTest.Http2.cs | 85 ++++++++++++++++------ 3 files changed, 88 insertions(+), 47 deletions(-) diff --git a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs index 2d22351..d2cafd0 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs @@ -565,7 +565,11 @@ namespace System.Net.Test.Common { PingFrame ping = new PingFrame(new byte[8] { 1, 2, 3, 4, 50, 60, 70, 80 }, FrameFlags.None, 0); await WriteFrameAsync(ping).ConfigureAwait(false); - await ReadFrameAsync(Timeout).ConfigureAwait(false); + Frame pingAck = await ReadFrameAsync(Timeout).ConfigureAwait(false); + if (pingAck.Type != FrameType.Ping || !pingAck.AckFlag) + { + throw new Exception("Expected PING ACK"); + } } public async Task SendDefaultResponseHeadersAsync(int streamId) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs index 3fc6ab1..2b5f530 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs @@ -296,7 +296,7 @@ namespace System.Net.Http } private static readonly HPackDecoder.HeaderCallback s_http2StreamOnResponseHeader = - (state, name, value) => ((Http2Stream)state).OnResponseHeader(name, value); + (state, name, value) => ((Http2Stream)state)?.OnResponseHeader(name, value); private async Task ProcessHeadersFrame(FrameHeader frameHeader) { @@ -306,14 +306,12 @@ namespace System.Net.Http int streamId = frameHeader.StreamId; Http2Stream http2Stream = GetStream(streamId); - if (http2Stream == null) - { - _incomingBuffer.Discard(frameHeader.Length); - throw new Http2ProtocolException(Http2ProtocolErrorCode.StreamClosed); - } + // Note, http2Stream will be null if this is a closed stream. + // We will still process the headers, to ensure the header decoding state is up-to-date, + // but we will discard the decoded headers. - http2Stream.OnResponseHeadersStart(); + http2Stream?.OnResponseHeadersStart(); _hpackDecoder.Decode( GetFrameData(_incomingBuffer.ActiveSpan.Slice(0, frameHeader.Length), frameHeader.PaddedFlag, frameHeader.PriorityFlag), @@ -341,9 +339,9 @@ namespace System.Net.Http _hpackDecoder.CompleteDecode(); - http2Stream.OnResponseHeadersComplete(endStream); + http2Stream?.OnResponseHeadersComplete(endStream); - if (endStream) + if (endStream && http2Stream != null) { RemoveStream(http2Stream); } @@ -388,25 +386,25 @@ namespace System.Net.Http Debug.Assert(frameHeader.Type == FrameType.Data); Http2Stream http2Stream = GetStream(frameHeader.StreamId); - if (http2Stream == null) - { - _incomingBuffer.Discard(frameHeader.Length); - throw new Http2ProtocolException(Http2ProtocolErrorCode.StreamClosed); - } + // Note, http2Stream will be null if this is a closed stream. + // Just ignore the frame in this case. - ReadOnlySpan frameData = GetFrameData(_incomingBuffer.ActiveSpan.Slice(0, frameHeader.Length), hasPad: frameHeader.PaddedFlag, hasPriority: false); - - bool endStream = frameHeader.EndStreamFlag; + if (http2Stream != null) + { + ReadOnlySpan frameData = GetFrameData(_incomingBuffer.ActiveSpan.Slice(0, frameHeader.Length), hasPad: frameHeader.PaddedFlag, hasPriority: false); - http2Stream.OnResponseData(frameData, endStream); + bool endStream = frameHeader.EndStreamFlag; - _incomingBuffer.Discard(frameHeader.Length); + http2Stream.OnResponseData(frameData, endStream); - if (endStream) - { - RemoveStream(http2Stream); + if (endStream) + { + RemoveStream(http2Stream); + } } + + _incomingBuffer.Discard(frameHeader.Length); } private void ProcessSettingsFrame(FrameHeader frameHeader) @@ -1498,6 +1496,8 @@ namespace System.Net.Http private void RemoveStream(Http2Stream http2Stream) { + Debug.Assert(http2Stream != null); + lock (SyncObject) { if (!_httpStreams.Remove(http2Stream.StreamId, out Http2Stream removed)) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 7b7199a..a05d28f 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -601,8 +601,10 @@ namespace System.Net.Http.Functional.Tests } } - [ConditionalFact(nameof(SupportsAlpn))] - public async Task CompletedResponse_FrameReceived_ConnectionError() + [ConditionalTheory(nameof(SupportsAlpn))] + [InlineData(true)] + [InlineData(false)] + public async Task CompletedResponse_FrameReceived_Ignored(bool sendDataFrame) { using (var server = Http2LoopbackServer.CreateServer()) using (HttpClient client = CreateHttpClient()) @@ -622,19 +624,20 @@ namespace System.Net.Http.Functional.Tests Assert.Equal(HttpStatusCode.OK, response.StatusCode); // Send a frame on the now-closed stream. - DataFrame invalidFrame = new DataFrame(new byte[10], FrameFlags.None, 0, streamId); + Frame invalidFrame = ConstructInvalidFrameForClosedStream(streamId, sendDataFrame); await connection.WriteFrameAsync(invalidFrame); - if (!IsWinHttpHandler) - { - // The client should close the connection as this is a fatal connection level error. - Assert.Null(await connection.ReadFrameAsync(TimeSpan.FromSeconds(30))); - } + // Pingpong to ensure the frame is processed and ignored + await connection.PingPong(); + + await ValidateConnection(client, server.Address, connection); } } - [ConditionalFact(nameof(SupportsAlpn))] - public async Task EmptyResponse_FrameReceived_ConnectionError() + [ConditionalTheory(nameof(SupportsAlpn))] + [InlineData(true)] + [InlineData(false)] + public async Task EmptyResponse_FrameReceived_Ignored(bool sendDataFrame) { using (var server = Http2LoopbackServer.CreateServer()) using (HttpClient client = CreateHttpClient()) @@ -651,14 +654,13 @@ namespace System.Net.Http.Functional.Tests Assert.Equal(HttpStatusCode.OK, response.StatusCode); // Send a frame on the now-closed stream. - DataFrame invalidFrame = new DataFrame(new byte[10], FrameFlags.None, 0, streamId); + Frame invalidFrame = ConstructInvalidFrameForClosedStream(streamId, sendDataFrame); await connection.WriteFrameAsync(invalidFrame); - if (!IsWinHttpHandler) - { - // The client should close the connection as this is a fatal connection level error. - Assert.Null(await connection.ReadFrameAsync(TimeSpan.FromSeconds(30))); - } + // Pingpong to ensure the frame is processed and ignored + await connection.PingPong(); + + await ValidateConnection(client, server.Address, connection); } } @@ -688,15 +690,51 @@ namespace System.Net.Http.Functional.Tests } } + private static Frame ConstructInvalidFrameForClosedStream(int streamId, bool dataFrame) + { + if (dataFrame) + { + return new DataFrame(new byte[10], FrameFlags.None, 0, streamId); + } + else + { + byte[] headers = new byte[] { 0x88 }; // Encoding for ":status: 200" + return new HeadersFrame(headers, FrameFlags.EndHeaders, 0, 0, 0, streamId); + } + } + + // Validate that connection is still usable, by sending a request and receiving a response + private static async Task ValidateConnection(HttpClient client, Uri serverAddress, Http2LoopbackConnection connection) + { + Task sendTask = client.GetAsync(serverAddress); + + int streamId = await connection.ReadRequestHeaderAsync(); + await connection.SendDefaultResponseAsync(streamId); + + HttpResponseMessage response = await sendTask; + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + return streamId; + } + public static IEnumerable ValidAndInvalidProtocolErrors() => Enum.GetValues(typeof(ProtocolErrors)) .Cast() .Concat(new[] { (ProtocolErrors)12345 }) .Select(p => new object[] { p }); + public static IEnumerable ValidAndInvalidProtocolErrorsAndBool() + { + foreach (object[] args in ValidAndInvalidProtocolErrors()) + { + yield return args.Append(true).ToArray(); + yield return args.Append(false).ToArray(); + } + } + [ConditionalTheory(nameof(SupportsAlpn))] - [MemberData(nameof(ValidAndInvalidProtocolErrors))] - public async Task ResetResponseStream_FrameReceived_ConnectionError(ProtocolErrors error) + [MemberData(nameof(ValidAndInvalidProtocolErrorsAndBool))] + public async Task ResetResponseStream_FrameReceived_Ignored(ProtocolErrors error, bool dataFrame) { using (var server = Http2LoopbackServer.CreateServer()) using (HttpClient client = CreateHttpClient()) @@ -714,14 +752,13 @@ namespace System.Net.Http.Functional.Tests await AssertProtocolErrorAsync(sendTask, error); // Send a frame on the now-closed stream. - DataFrame invalidFrame = new DataFrame(new byte[10], FrameFlags.None, 0, streamId); + Frame invalidFrame = ConstructInvalidFrameForClosedStream(streamId, dataFrame); await connection.WriteFrameAsync(invalidFrame); - if (!IsWinHttpHandler) - { - // The client should close the connection as this is a fatal connection level error. - Assert.Null(await connection.ReadFrameAsync(TimeSpan.FromSeconds(30))); - } + // Pingpong to ensure the frame is processed and ignored + await connection.PingPong(); + + await ValidateConnection(client, server.Address, connection); } } -- 2.7.4