From 010139ac8a3ad9f4cebdf7013baba2c844f155f1 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 29 Mar 2019 16:34:26 -0700 Subject: [PATCH] Limit chunk size when writing chunked responses (#8837) --- .../HttpResponseWritingExtensions.cs | 21 ++- .../Core/src/Internal/Http/ChunkWriter.cs | 92 +++++++++++++ .../src/Internal/Http/Http1OutputProducer.cs | 64 +++++---- .../Kestrel/Core/test/ChunkWriterTests.cs | 39 ++++++ .../ChunkedResponseTests.cs | 126 +++++++++++++++++- .../InMemory.FunctionalTests/ResponseTests.cs | 31 +++++ 6 files changed, 338 insertions(+), 35 deletions(-) diff --git a/src/Http/Http.Abstractions/src/Extensions/HttpResponseWritingExtensions.cs b/src/Http/Http.Abstractions/src/Extensions/HttpResponseWritingExtensions.cs index 6857eee189..7d4532c4c8 100644 --- a/src/Http/Http.Abstractions/src/Extensions/HttpResponseWritingExtensions.cs +++ b/src/Http/Http.Abstractions/src/Extensions/HttpResponseWritingExtensions.cs @@ -14,6 +14,8 @@ namespace Microsoft.AspNetCore.Http /// public static class HttpResponseWritingExtensions { + private const int UTF8MaxByteLength = 6; + /// /// Writes the given text to the response body. UTF-8 encoding will be used. /// @@ -93,9 +95,10 @@ namespace Microsoft.AspNetCore.Http private static void Write(this HttpResponse response, string text, Encoding encoding) { + var minimumByteSize = GetEncodingMaxByteSize(encoding); var pipeWriter = response.BodyWriter; var encodedLength = encoding.GetByteCount(text); - var destination = pipeWriter.GetSpan(encodedLength); + var destination = pipeWriter.GetSpan(minimumByteSize); if (encodedLength <= destination.Length) { @@ -105,11 +108,21 @@ namespace Microsoft.AspNetCore.Http } else { - WriteMultiSegmentEncoded(pipeWriter, text, encoding, destination, encodedLength); + WriteMultiSegmentEncoded(pipeWriter, text, encoding, destination, encodedLength, minimumByteSize); } } - private static void WriteMultiSegmentEncoded(PipeWriter writer, string text, Encoding encoding, Span destination, int encodedLength) + private static int GetEncodingMaxByteSize(Encoding encoding) + { + if (encoding == Encoding.UTF8) + { + return UTF8MaxByteLength; + } + + return encoding.GetMaxByteCount(1); + } + + private static void WriteMultiSegmentEncoded(PipeWriter writer, string text, Encoding encoding, Span destination, int encodedLength, int minimumByteSize) { var encoder = encoding.GetEncoder(); var source = text.AsSpan(); @@ -126,7 +139,7 @@ namespace Microsoft.AspNetCore.Http writer.Advance(bytesUsed); source = source.Slice(charsUsed); - destination = writer.GetSpan(encodedLength - totalBytesUsed); + destination = writer.GetSpan(minimumByteSize); } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/ChunkWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http/ChunkWriter.cs index 500d5124ca..75186e8ad6 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/ChunkWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/ChunkWriter.cs @@ -44,6 +44,98 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return count; } + internal static int GetPrefixBytesForChunk(int length, out bool sliceOneByte) + { + sliceOneByte = false; + // If GetMemory returns one of the following values, there is no way to set the prefix/body lengths + // such that we either wouldn't have an invalid chunk or would need to copy if the entire memory chunk is used. + // For example, if GetMemory returned 21, we would guess that the chunked prefix is 4 bytes initially + // and the suffix is 2 bytes, meaning there is 15 bytes remaining to write into. However, 15 bytes only need 3 + // bytes for the chunked prefix, so we would have to copy once we call advance. Therefore, to avoid this scenario, + // we slice the memory by one byte. + + // See https://gist.github.com/halter73/af2b9f78978f83813b19e187c4e5309e if you would like to tweek the algorithm at all. + + if (length <= 65544) + { + if (length <= 262) + { + if (length <= 21) + { + if (length == 21) + { + sliceOneByte = true; + } + return 3; + } + else + { + if (length == 262) + { + sliceOneByte = true; + } + return 4; + } + } + else + { + if (length <= 4103) + { + if (length == 4103) + { + sliceOneByte = true; + } + return 5; + } + else + { + if (length == 65544) + { + sliceOneByte = true; + } + return 6; + } + } + } + else + { + if (length <= 16777226) + { + if (length <= 1048585) + { + if (length == 1048585) + { + sliceOneByte = true; + } + return 7; + } + else + { + if (length == 16777226) + { + sliceOneByte = true; + } + return 8; + } + } + else + { + if (length <= 268435467) + { + if (length == 268435467) + { + sliceOneByte = true; + } + return 9; + } + else + { + return 10; + } + } + } + } + internal static int WriteBeginChunkBytes(this ref BufferWriter start, int dataCount) { // 10 bytes is max length + \r\n diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index 17e32d8206..7e1aca76e1 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -27,7 +27,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http // "0\r\n\r\n" private static ReadOnlySpan EndChunkedResponseBytes => new byte[] { (byte)'0', (byte)'\r', (byte)'\n', (byte)'\r', (byte)'\n' }; - private const int BeginChunkLengthMax = 5; + private const int MaxBeginChunkLength = 10; private const int EndChunkLength = 2; private readonly string _connectionId; @@ -44,6 +44,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http private bool _completed; private bool _aborted; private long _unflushedBytes; + private int _currentMemoryPrefixBytes; private readonly PipeWriter _pipeWriter; private IMemoryOwner _fakeMemoryOwner; @@ -148,7 +149,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http Debug.Assert(producer._autoChunk && producer._advancedBytesForChunk > 0); var writer = new BufferWriter(producer._pipeWriter); - producer.WriteCurrentMemoryToPipeWriter(ref writer); + producer.WriteCurrentChunkMemoryToPipeWriter(ref writer); writer.Commit(); var bytesWritten = producer._unflushedBytes + writer.BytesCommitted; @@ -230,7 +231,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } else if (_autoChunk) { - if (_advancedBytesForChunk > _currentChunkMemory.Length - BeginChunkLengthMax - EndChunkLength - bytes) + if (_advancedBytesForChunk > _currentChunkMemory.Length - _currentMemoryPrefixBytes - EndChunkLength - bytes) { throw new ArgumentOutOfRangeException("Can't advance past buffer size."); } @@ -275,7 +276,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { if (_advancedBytesForChunk > 0) { - WriteCurrentMemoryToPipeWriter(ref writer); + WriteCurrentChunkMemoryToPipeWriter(ref writer); } if (buffer.Length > 0) @@ -483,6 +484,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http _autoChunk = false; _startCalled = false; _currentChunkMemoryUpdated = false; + _currentMemoryPrefixBytes = 0; } private ValueTask WriteAsync( @@ -512,7 +514,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { // If there is data that was chunked before writing (ex someone did GetMemory->Advance->WriteAsync) // make sure to write whatever was advanced first - WriteCurrentMemoryToPipeWriter(ref writer); + WriteCurrentChunkMemoryToPipeWriter(ref writer); } else { @@ -538,57 +540,61 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http cancellationToken); } - // These methods are for chunked http responses that use GetMemory/Advance private Memory GetChunkedMemory(int sizeHint) { - // The max size of a chunk will be the size of memory returned from the PipeWriter (today 4096) - // minus 5 for the max chunked prefix size and minus 2 for the chunked ending, leaving a total of - // 4089. - if (!_currentChunkMemoryUpdated) { - // First time calling GetMemory - _currentChunkMemory = _pipeWriter.GetMemory(sizeHint); - _currentChunkMemoryUpdated = true; + // Calculating ChunkWriter.GetBeginChunkByteCount isn't free, so instead, we can add + // the max length for the prefix and suffix and add it to the sizeHint. + // This still guarantees that the memory passed in will be larger than the sizeHint. + sizeHint += MaxBeginChunkLength + EndChunkLength; + UpdateCurrentChunkMemory(sizeHint); } - - var memoryMaxLength = _currentChunkMemory.Length - BeginChunkLengthMax - EndChunkLength; - if (_advancedBytesForChunk >= memoryMaxLength - sizeHint && _advancedBytesForChunk > 0) + // Check if we need to allocate a new memory. + else if (_advancedBytesForChunk >= _currentChunkMemory.Length - _currentMemoryPrefixBytes - EndChunkLength - sizeHint && _advancedBytesForChunk > 0) { - // Chunk is completely written, commit it to the pipe so GetMemory will return a new chunk of memory. + sizeHint += MaxBeginChunkLength + EndChunkLength; var writer = new BufferWriter(_pipeWriter); - WriteCurrentMemoryToPipeWriter(ref writer); + WriteCurrentChunkMemoryToPipeWriter(ref writer); writer.Commit(); - _unflushedBytes += writer.BytesCommitted; - _currentChunkMemory = _pipeWriter.GetMemory(sizeHint); - _currentChunkMemoryUpdated = true; + + UpdateCurrentChunkMemory(sizeHint); } var actualMemory = _currentChunkMemory.Slice( - BeginChunkLengthMax + _advancedBytesForChunk, - memoryMaxLength - _advancedBytesForChunk); - - Debug.Assert(actualMemory.Length <= 4089); + _currentMemoryPrefixBytes + _advancedBytesForChunk, + _currentChunkMemory.Length - _currentMemoryPrefixBytes - EndChunkLength - _advancedBytesForChunk); return actualMemory; } - private void WriteCurrentMemoryToPipeWriter(ref BufferWriter writer) + private void UpdateCurrentChunkMemory(int sizeHint) + { + _currentChunkMemory = _pipeWriter.GetMemory(sizeHint); + _currentMemoryPrefixBytes = ChunkWriter.GetPrefixBytesForChunk(_currentChunkMemory.Length, out var sliceOne); + if (sliceOne) + { + _currentChunkMemory = _currentChunkMemory.Slice(0, _currentChunkMemory.Length - 1); + } + _currentChunkMemoryUpdated = true; + } + + private void WriteCurrentChunkMemoryToPipeWriter(ref BufferWriter writer) { Debug.Assert(_advancedBytesForChunk <= _currentChunkMemory.Length); Debug.Assert(_advancedBytesForChunk > 0); var bytesWritten = writer.WriteBeginChunkBytes(_advancedBytesForChunk); - Debug.Assert(bytesWritten <= BeginChunkLengthMax); + Debug.Assert(bytesWritten <= _currentMemoryPrefixBytes); - if (bytesWritten < BeginChunkLengthMax) + if (bytesWritten < _currentMemoryPrefixBytes) { // If the current chunk of memory isn't completely utilized, we need to copy the contents forwards. // This occurs if someone uses less than 255 bytes of the current Memory segment. // Therefore, we need to copy it forwards by either 1 or 2 bytes (depending on number of bytes) - _currentChunkMemory.Slice(BeginChunkLengthMax, _advancedBytesForChunk).CopyTo(_currentChunkMemory.Slice(bytesWritten)); + _currentChunkMemory.Slice(_currentMemoryPrefixBytes, _advancedBytesForChunk).CopyTo(_currentChunkMemory.Slice(bytesWritten)); } writer.Advance(_advancedBytesForChunk); diff --git a/src/Servers/Kestrel/Core/test/ChunkWriterTests.cs b/src/Servers/Kestrel/Core/test/ChunkWriterTests.cs index 2b6f3a3c65..42bb412485 100644 --- a/src/Servers/Kestrel/Core/test/ChunkWriterTests.cs +++ b/src/Servers/Kestrel/Core/test/ChunkWriterTests.cs @@ -43,5 +43,44 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests Assert.Equal(Encoding.ASCII.GetBytes(expected), span.Slice(0, count).ToArray()); } + + [Theory] + [InlineData(20, false)] + [InlineData(21, true)] + [InlineData(22, false)] + [InlineData(261, false)] + [InlineData(262, true)] + [InlineData(263, false)] + [InlineData(4102, false)] + [InlineData(4103, true)] + [InlineData(4104, false)] + [InlineData(65543, false)] + [InlineData(65544, true)] + [InlineData(65545, false)] + [InlineData(1048584, false)] + [InlineData(1048585, true)] + [InlineData(1048586, false)] + [InlineData(16777225, false)] + [InlineData(16777226, true)] + [InlineData(16777227, false)] + [InlineData(268435466, false)] + [InlineData(268435467, true)] + [InlineData(268435468, false)] + public void ChunkedPrefixReturnsSegmentThatDoesNotNeedToMove(int dataCount, bool expectSlice) + { + // Will call GetMemory on at least 5 bytes from the Http1OutputProducer + var prefixLength = ChunkWriter.GetPrefixBytesForChunk(dataCount, out var actualSliceOneByte); + if (actualSliceOneByte) + { + dataCount--; + } + + var fakeMemory = new Memory(new byte[16]); + + var actualLength = ChunkWriter.BeginChunkBytes(dataCount - prefixLength - 2, fakeMemory.Span); + + Assert.Equal(prefixLength, actualLength); + Assert.Equal(expectSlice, actualSliceOneByte); + } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedResponseTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedResponseTests.cs index 6c36b041bf..9bdf8785e9 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedResponseTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedResponseTests.cs @@ -117,6 +117,128 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests } } + [Fact] + public async Task ResponsesAreChunkedAutomaticallyLargeResponseWithOverloadedWriteAsync() + { + var testContext = new TestServiceContext(LoggerFactory); + var expectedString = new string('a', 10000); + using (var server = new TestServer(async httpContext => + { + await httpContext.Response.WriteAsync(expectedString); + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "GET / HTTP/1.1", + "Host: ", + "Connection: close", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Transfer-Encoding: chunked", + "", + "f92", + new string('a', 3986), + "ff9", + new string('a', 4089), + "785", + new string('a', 1925), + "0", + "", + ""); + } + await server.StopAsync(); + } + } + + [Theory] + [InlineData(4096)] + [InlineData(10000)] + [InlineData(100000)] + public async Task ResponsesAreChunkedAutomaticallyLargeChunksLargeResponseWithOverloadedWriteAsync(int length) + { + var testContext = new TestServiceContext(LoggerFactory); + var expectedString = new string('a', length); + using (var server = new TestServer(async httpContext => + { + await httpContext.Response.StartAsync(); + var memory = httpContext.Response.BodyWriter.GetMemory(length); + Assert.True(length <= memory.Length); + Encoding.ASCII.GetBytes(expectedString).CopyTo(memory); + httpContext.Response.BodyWriter.Advance(length); + await httpContext.Response.BodyWriter.FlushAsync(); + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "GET / HTTP/1.1", + "Host: ", + "Connection: close", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Transfer-Encoding: chunked", + "", + length.ToString("x"), + new string('a', length), + "0", + "", + ""); + } + await server.StopAsync(); + } + } + + [Theory] + [InlineData(1)] + [InlineData(16)] + [InlineData(256)] + [InlineData(4096)] + public async Task ResponsesAreChunkedAutomaticallyPartialWrite(int partialLength) + { + var testContext = new TestServiceContext(LoggerFactory); + var expectedString = new string('a', partialLength); + using (var server = new TestServer(async httpContext => + { + await httpContext.Response.StartAsync(); + var memory = httpContext.Response.BodyWriter.GetMemory(100000); + Encoding.ASCII.GetBytes(expectedString).CopyTo(memory); + httpContext.Response.BodyWriter.Advance(partialLength); + await httpContext.Response.BodyWriter.FlushAsync(); + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "GET / HTTP/1.1", + "Host: ", + "Connection: close", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Transfer-Encoding: chunked", + "", + partialLength.ToString("x"), + new string('a', partialLength), + "0", + "", + ""); + } + await server.StopAsync(); + } + } + [Fact] public async Task SettingConnectionCloseHeaderInAppDoesNotDisableChunking() { @@ -523,7 +645,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests { var response = httpContext.Response; - var memory = response.BodyWriter.GetMemory(); + var memory = response.BodyWriter.GetMemory(5000); length.Value = memory.Length; semaphore.Release(); @@ -581,7 +703,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests await response.BodyWriter.FlushAsync(); - var memory = response.BodyWriter.GetMemory(); + var memory = response.BodyWriter.GetMemory(5000); length.Value = memory.Length; semaphore.Release(); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ResponseTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ResponseTests.cs index 3abfea307c..c9705fefb8 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ResponseTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ResponseTests.cs @@ -2906,6 +2906,37 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests } } + [Fact] + public async Task LargeWriteWithContentLengthWritingWorks() + { + var testContext = new TestServiceContext(LoggerFactory); + var expectedLength = 100000; + var expectedString = new string('a', expectedLength); + using (var server = new TestServer(async httpContext => + { + httpContext.Response.Headers["Content-Length"] = new[] { expectedLength.ToString() }; + await httpContext.Response.WriteAsync(expectedString); + Assert.True(httpContext.Response.HasStarted); + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "GET / HTTP/1.1", + "Host:", + "", + ""); + await connection.Receive( + "HTTP/1.1 200 OK", + $"Date: {testContext.DateHeaderValue}", + $"Content-Length: {expectedLength}", + "", + expectedString); + } + await server.StopAsync(); + } + } + [Fact] public async Task StartAsyncAndFlushWorks() {