From 9e15b2bca41ab1dcba50cbfe921bb03c104646b1 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 31 May 2018 11:27:38 -0700 Subject: [PATCH] Fix PipeReader consumption pattern [2.1] --- .../PlatformBenchmarks/HttpApplication.cs | 8 +-- .../Adapter/Internal/RawStream.cs | 3 +- .../Internal/Http/Http1MessageBody.cs | 4 +- src/Kestrel.Core/Internal/Http/MessageBody.cs | 6 ++- .../Internal/Http2/Http2Connection.cs | 19 +++---- .../Internal/LibuvOutputConsumer.cs | 3 +- .../ChunkedRequestTests.cs | 51 +++++++++++++++++++ 7 files changed, 73 insertions(+), 21 deletions(-) diff --git a/benchmarkapps/PlatformBenchmarks/HttpApplication.cs b/benchmarkapps/PlatformBenchmarks/HttpApplication.cs index e9b20f5aaa..698e8952b0 100644 --- a/benchmarkapps/PlatformBenchmarks/HttpApplication.cs +++ b/benchmarkapps/PlatformBenchmarks/HttpApplication.cs @@ -107,10 +107,6 @@ namespace PlatformBenchmarks ThrowUnexpectedEndOfData(); } } - else if (result.IsCompleted) - { - break; - } Reader.AdvanceTo(consumed, examined); @@ -120,6 +116,10 @@ namespace PlatformBenchmarks _state = State.StartLine; } + else if (result.IsCompleted) + { + break; + } } } diff --git a/src/Kestrel.Core/Adapter/Internal/RawStream.cs b/src/Kestrel.Core/Adapter/Internal/RawStream.cs index ea381c22b2..084eed2418 100644 --- a/src/Kestrel.Core/Adapter/Internal/RawStream.cs +++ b/src/Kestrel.Core/Adapter/Internal/RawStream.cs @@ -125,7 +125,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal readableBuffer.CopyTo(destination.Span); return count; } - else if (result.IsCompleted) + + if (result.IsCompleted) { return 0; } diff --git a/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs b/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs index d65805cc83..f11619d7f3 100644 --- a/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs @@ -87,7 +87,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http break; } } - else if (result.IsCompleted) + + // Read() will have already have greedily consumed the entire request body if able. + if (result.IsCompleted) { // Treat any FIN from an upgraded request as expected. // It's up to higher-level consumer (i.e. WebSocket middleware) to determine diff --git a/src/Kestrel.Core/Internal/Http/MessageBody.cs b/src/Kestrel.Core/Internal/Http/MessageBody.cs index a406e64ffa..33bd8ebfb5 100644 --- a/src/Kestrel.Core/Internal/Http/MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http/MessageBody.cs @@ -57,7 +57,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http slice.CopyTo(buffer.Span); return actual; } - else if (result.IsCompleted) + + if (result.IsCompleted) { return 0; } @@ -96,7 +97,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http #endif } } - else if (result.IsCompleted) + + if (result.IsCompleted) { return; } diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index 549e804d33..c3c0a53672 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -117,14 +117,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 try { - if (!readableBuffer.IsEmpty) + if (!readableBuffer.IsEmpty && ParsePreface(readableBuffer, out consumed, out examined)) { - if (ParsePreface(readableBuffer, out consumed, out examined)) - { - break; - } + break; } - else if (result.IsCompleted) + + if (result.IsCompleted) { return; } @@ -149,13 +147,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 try { - if (!readableBuffer.IsEmpty) + if (!readableBuffer.IsEmpty && Http2FrameReader.ReadFrame(readableBuffer, _incomingFrame, out consumed, out examined)) { - if (Http2FrameReader.ReadFrame(readableBuffer, _incomingFrame, out consumed, out examined)) - { - Log.LogTrace($"Connection id {ConnectionId} received {_incomingFrame.Type} frame with flags 0x{_incomingFrame.Flags:x} and length {_incomingFrame.Length} for stream ID {_incomingFrame.StreamId}"); - await ProcessFrameAsync(application); - } + Log.LogTrace($"Connection id {ConnectionId} received {_incomingFrame.Type} frame with flags 0x{_incomingFrame.Flags:x} and length {_incomingFrame.Length} for stream ID {_incomingFrame.StreamId}"); + await ProcessFrameAsync(application); } else if (result.IsCompleted) { diff --git a/src/Kestrel.Transport.Libuv/Internal/LibuvOutputConsumer.cs b/src/Kestrel.Transport.Libuv/Internal/LibuvOutputConsumer.cs index 94195a3ffc..e2ba7a09e3 100644 --- a/src/Kestrel.Transport.Libuv/Internal/LibuvOutputConsumer.cs +++ b/src/Kestrel.Transport.Libuv/Internal/LibuvOutputConsumer.cs @@ -99,7 +99,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal writeReq = null; } } - else if (result.IsCompleted) + + if (result.IsCompleted) { break; } diff --git a/test/Kestrel.FunctionalTests/ChunkedRequestTests.cs b/test/Kestrel.FunctionalTests/ChunkedRequestTests.cs index 8dba8331e1..10b68fdf45 100644 --- a/test/Kestrel.FunctionalTests/ChunkedRequestTests.cs +++ b/test/Kestrel.FunctionalTests/ChunkedRequestTests.cs @@ -1,14 +1,17 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; using System.IO; using System.Linq; using System.Net; +using System.Net.Sockets; using System.Text; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Server.Kestrel.Core; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.Logging.Testing; using Xunit; @@ -634,6 +637,54 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests } } } + + [Theory] + [MemberData(nameof(ConnectionAdapterData))] + public async Task ClosingConnectionMidChunkPrefixThrows(ListenOptions listenOptions) + { + var testContext = new TestServiceContext(LoggerFactory); + var readStartedTcs = new TaskCompletionSource(); + var exTcs = new TaskCompletionSource(); + + using (var server = new TestServer(async httpContext => + { + var readTask = httpContext.Request.Body.CopyToAsync(Stream.Null); + readStartedTcs.SetResult(null); + + try + { + await readTask; + } + catch (BadHttpRequestException badRequestEx) + { + exTcs.TrySetResult(badRequestEx); + } + catch (Exception ex) + { + exTcs.SetException(ex); + } + }, testContext, listenOptions)) + { + using (var connection = server.CreateConnection()) + { + await connection.SendAll( + "POST / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "", + "1"); + + await readStartedTcs.Task.TimeoutAfter(TestConstants.DefaultTimeout); + + connection.Socket.Shutdown(SocketShutdown.Send); + + await connection.ReceiveEnd(); + + var badReqEx = await exTcs.Task.TimeoutAfter(TestConstants.DefaultTimeout); + Assert.Equal(RequestRejectionReason.UnexpectedEndOfRequestContent, badReqEx.Reason); + } + } + } } }