From ad3c257ef56eb944fc9b5d34ead2650c152b54d5 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Mon, 5 Oct 2015 09:45:23 -0700 Subject: [PATCH] Fix for #3252 - Issues with pooled buffer + unicode - Dispose buffers when Flush throws inside the Dispose method - Compute size of buffers correctly - Throw earlier when handed invalid-sized buffers --- .../HttpResponseStreamWriter.cs | 46 ++++++++++++------ ...moryPoolHttpResponseStreamWriterFactory.cs | 12 +++-- .../Properties/Resources.Designer.cs | 16 +++++++ src/Microsoft.AspNet.Mvc.Core/Resources.resx | 3 ++ .../HttpResponseStreamWriterTest.cs | 48 +++++++------------ ...PoolHttpResponseStreamWriterFactoryTest.cs | 4 +- 6 files changed, 76 insertions(+), 53 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs b/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs index b2db867f48..c56f2ec288 100644 --- a/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs @@ -5,6 +5,7 @@ using System; using System.IO; using System.Text; using System.Threading.Tasks; +using Microsoft.AspNet.Mvc.Core; using Microsoft.Extensions.MemoryPool; namespace Microsoft.AspNet.Mvc @@ -58,6 +59,7 @@ namespace Microsoft.AspNet.Mvc public HttpResponseStreamWriter( Stream stream, Encoding encoding, + int bufferSize, LeasedArraySegment leasedByteBuffer, LeasedArraySegment leasedCharBuffer) { @@ -81,21 +83,27 @@ namespace Microsoft.AspNet.Mvc throw new ArgumentNullException(nameof(leasedCharBuffer)); } + var requiredLength = encoding.GetMaxByteCount(bufferSize); + if (requiredLength > leasedByteBuffer.Data.Count) + { + var message = Resources.FormatHttpResponseStreamWriter_InvalidBufferSize( + requiredLength, + bufferSize, + encoding.EncodingName, + typeof(Encoding).FullName, + nameof(Encoding.GetMaxByteCount)); + throw new ArgumentException(message, nameof(leasedByteBuffer)); + } + _stream = stream; Encoding = encoding; + _charBufferSize = bufferSize; _leasedByteBuffer = leasedByteBuffer; _leasedCharBuffer = leasedCharBuffer; _encoder = encoding.GetEncoder(); _byteBuffer = leasedByteBuffer.Data; _charBuffer = leasedCharBuffer.Data; - - // We need to compute the usable size of the char buffer based on the size of the byte buffer. - // Encoder.GetBytes assumes that the entirety of the byte[] passed in can be used, and that's not the - // case with ArraySegments. - _charBufferSize = Math.Min( - leasedCharBuffer.Data.Count, - encoding.GetMaxCharCount(leasedByteBuffer.Data.Count)); } public override Encoding Encoding { get; } @@ -215,16 +223,24 @@ namespace Microsoft.AspNet.Mvc // sent in chunked encoding in case of Helios. protected override void Dispose(bool disposing) { - FlushInternal(flushStream: false, flushEncoder: true); - - if (_leasedByteBuffer != null) + if (disposing) { - _leasedByteBuffer.Owner.Return(_leasedByteBuffer); - } + try + { + FlushInternal(flushStream: false, flushEncoder: true); + } + finally + { + if (_leasedByteBuffer != null) + { + _leasedByteBuffer.Owner.Return(_leasedByteBuffer); + } - if (_leasedCharBuffer != null) - { - _leasedCharBuffer.Owner.Return(_leasedCharBuffer); + if (_leasedCharBuffer != null) + { + _leasedCharBuffer.Owner.Return(_leasedCharBuffer); + } + } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MemoryPoolHttpResponseStreamWriterFactory.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MemoryPoolHttpResponseStreamWriterFactory.cs index 83825356bc..7686c781c5 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MemoryPoolHttpResponseStreamWriterFactory.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MemoryPoolHttpResponseStreamWriterFactory.cs @@ -14,9 +14,9 @@ namespace Microsoft.AspNet.Mvc.Infrastructure public class MemoryPoolHttpResponseStreamWriterFactory : IHttpResponseStreamWriterFactory { /// - /// The default size of created buffers. + /// The default size of created char buffers. /// - public static readonly int DefaultBufferSize = 4 * 1024; // 4KB + public static readonly int DefaultBufferSize = 1024; // 1KB - results in a 4KB byte array for UTF8. private readonly IArraySegmentPool _bytePool; private readonly IArraySegmentPool _charPool; @@ -66,10 +66,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure try { - bytes = _bytePool.Lease(DefaultBufferSize); chars = _charPool.Lease(DefaultBufferSize); - return new HttpResponseStreamWriter(stream, encoding, bytes, chars); + // We need to compute the minimum size of the byte buffer based on the size of the char buffer, + // so that we have enough room to encode the buffer in one shot. + var minimumSize = encoding.GetMaxByteCount(DefaultBufferSize); + bytes = _bytePool.Lease(minimumSize); + + return new HttpResponseStreamWriter(stream, encoding, DefaultBufferSize, bytes, chars); } catch { diff --git a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs index f866e339dd..714b435aca 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs @@ -1002,6 +1002,22 @@ namespace Microsoft.AspNet.Mvc.Core return string.Format(CultureInfo.CurrentCulture, GetString("ValueProviderResult_NoConverterExists"), p0, p1); } + /// + /// The byte buffer must have a length of at least '{0}' to be used with a char buffer of size '{1}' and encoding '{2}'. Use '{3}.{4}' to compute the correct size for the byte buffer. + /// + internal static string HttpResponseStreamWriter_InvalidBufferSize + { + get { return GetString("HttpResponseStreamWriter_InvalidBufferSize"); } + } + + /// + /// The byte buffer must have a length of at least '{0}' to be used with a char buffer of size '{1}' and encoding '{2}'. Use '{3}.{4}' to compute the correct size for the byte buffer. + /// + internal static string FormatHttpResponseStreamWriter_InvalidBufferSize(object p0, object p1, object p2, object p3, object p4) + { + return string.Format(CultureInfo.CurrentCulture, GetString("HttpResponseStreamWriter_InvalidBufferSize"), p0, p1, p2, p3, p4); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNet.Mvc.Core/Resources.resx b/src/Microsoft.AspNet.Mvc.Core/Resources.resx index bedebefd24..95a2ff975a 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNet.Mvc.Core/Resources.resx @@ -312,4 +312,7 @@ The parameter conversion from type '{0}' to type '{1}' failed because no type converter can convert between these types. + + The byte buffer must have a length of at least '{0}' to be used with a char buffer of size '{1}' and encoding '{2}'. Use '{3}.{4}' to compute the correct size for the byte buffer. + \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.cs index 1a199766d7..4918426050 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.cs @@ -6,6 +6,7 @@ using System.IO; using System.Text; using System.Threading; using System.Threading.Tasks; +using Microsoft.AspNet.Testing; using Microsoft.Extensions.MemoryPool; using Xunit; @@ -381,9 +382,9 @@ namespace Microsoft.AspNet.Mvc try { bytes = bytePool.Lease(4096); - chars = charPool.Lease(4096); + chars = charPool.Lease(1024); - writer = new HttpResponseStreamWriter(stream, encoding, bytes, chars); + writer = new HttpResponseStreamWriter(stream, encoding, 1024, bytes, chars); } catch { @@ -412,8 +413,8 @@ namespace Microsoft.AspNet.Mvc Assert.Equal(expectedBytes, stream.ToArray()); } - // This covers the case where we need to limit the usable region of the char buffer - // based on the size of the byte buffer. See comments in the constructor. + // This covers the error case where the byte buffer is too small. This is a safeguard, and shouldn't happen + // if we're using the writer factory. [Fact] public void HttpResponseStreamWriter_UsingPooledBuffers_SmallByteBuffer() { @@ -421,12 +422,10 @@ namespace Microsoft.AspNet.Mvc var encoding = Encoding.UTF8; var stream = new MemoryStream(); - var charBufferSize = encoding.GetMaxCharCount(1024); - - // This content is bigger than the byte buffer can hold, so it will need to be split - // into two separate encoding operations. - var content = new string('a', charBufferSize + 1); - var expectedBytes = encoding.GetBytes(content); + var message = + "The byte buffer must have a length of at least '12291' to be used with a char buffer of " + + "size '4096' and encoding 'Unicode (UTF-8)'. Use 'System.Text.Encoding.GetMaxByteCount' " + + "to compute the correct size for the byte buffer."; using (var bytePool = new DefaultArraySegmentPool()) { @@ -434,14 +433,19 @@ namespace Microsoft.AspNet.Mvc { LeasedArraySegment bytes = null; LeasedArraySegment chars = null; - HttpResponseStreamWriter writer; + HttpResponseStreamWriter writer = null; try { bytes = bytePool.Lease(1024); chars = charPool.Lease(4096); - writer = new HttpResponseStreamWriter(stream, encoding, bytes, chars); + // Act & Assert + ExceptionAssert.ThrowsArgument( + () => writer = new HttpResponseStreamWriter(stream, encoding, chars.Data.Count, bytes, chars), + "byteBuffer", + message); + writer.Dispose(); } catch { @@ -454,29 +458,9 @@ namespace Microsoft.AspNet.Mvc { chars.Owner.Return(chars); } - - throw; - } - - // Zero the byte buffer because we're going to examine it. - Array.Clear(bytes.Data.Array, 0, bytes.Data.Array.Length); - - // Act - using (writer) - { - writer.Write(content); - } - - // Verify that we didn't buffer overflow 'our' region of the underlying array. - if (bytes.Data.Array.Length > bytes.Data.Count) - { - Assert.Equal((byte)0, bytes.Data.Array[bytes.Data.Count]); } } } - - // Assert - Assert.Equal(expectedBytes, stream.ToArray()); } private class TestMemoryStream : MemoryStream diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MemoryPoolHttpResponseStreamWriterFactoryTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MemoryPoolHttpResponseStreamWriterFactoryTest.cs index 1cb9870e7b..26b8e3d170 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MemoryPoolHttpResponseStreamWriterFactoryTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MemoryPoolHttpResponseStreamWriterFactoryTest.cs @@ -18,7 +18,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure // Arrange var bytePool = new Mock>(MockBehavior.Strict); bytePool - .Setup(p => p.Lease(MemoryPoolHttpResponseStreamWriterFactory.DefaultBufferSize)) + .Setup(p => p.Lease(It.IsAny())) .Returns(new LeasedArraySegment(new ArraySegment(new byte[0]), bytePool.Object)); bytePool .Setup(p => p.Return(It.IsAny>())) @@ -32,7 +32,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure .Setup(p => p.Return(It.IsAny>())) .Verifiable(); - var encoding = new Mock(MockBehavior.Strict); + var encoding = new Mock(); encoding .Setup(e => e.GetEncoder()) .Throws(new InvalidOperationException());