diff --git a/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/FormPipeReaderInternalsBenchmark.cs b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/FormPipeReaderInternalsBenchmark.cs index 64019bf0d9..02691d1dff 100644 --- a/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/FormPipeReaderInternalsBenchmark.cs +++ b/src/Http/WebUtilities/perf/Microsoft.AspNetCore.WebUtilities.Performance/FormPipeReaderInternalsBenchmark.cs @@ -13,8 +13,8 @@ namespace Microsoft.AspNetCore.WebUtilities.Performance public class FormPipeReaderInternalsBenchmark { private byte[] _singleUtf8 = Encoding.UTF8.GetBytes("foo=bar&baz=boo&haha=hehe&lol=temp"); - private byte[] _firstUtf8 = Encoding.UTF8.GetBytes("foo=bar&baz=boo"); - private byte[] _secondUtf8 = Encoding.UTF8.GetBytes("&haha=hehe&lol=temp"); + private byte[] _firstUtf8 = Encoding.UTF8.GetBytes("foo=bar&baz=bo"); + private byte[] _secondUtf8 = Encoding.UTF8.GetBytes("o&haha=hehe&lol=temp"); private FormPipeReader _formPipeReader; [IterationSetup] diff --git a/src/Http/WebUtilities/src/FormPipeReader.cs b/src/Http/WebUtilities/src/FormPipeReader.cs index 70e85ccfcf..e82a0bf9c4 100644 --- a/src/Http/WebUtilities/src/FormPipeReader.cs +++ b/src/Http/WebUtilities/src/FormPipeReader.cs @@ -4,6 +4,7 @@ using System; using System.Buffers; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.IO.Pipelines; using System.Runtime.CompilerServices; @@ -139,74 +140,78 @@ namespace Microsoft.AspNetCore.WebUtilities bool isFinalBlock, out int consumed) { - ReadOnlySpan key = default; - ReadOnlySpan value = default; + ReadOnlySpan key; + ReadOnlySpan value; consumed = 0; var equalsDelimiter = GetEqualsForEncoding(); var andDelimiter = GetAndForEncoding(); while (span.Length > 0) { - var equals = span.IndexOf(equalsDelimiter); - - if (equals == -1) - { - if (span.Length > KeyLengthLimit) - { - ThrowKeyTooLargeException(); - } - break; - } - - if (equals > KeyLengthLimit) - { - ThrowKeyTooLargeException(); - } - - key = span.Slice(0, equals); - - span = span.Slice(key.Length + equalsDelimiter.Length); - value = span; - + // Find the end of the key=value pair. var ampersand = span.IndexOf(andDelimiter); + ReadOnlySpan keyValuePair; + int equals; + var foundAmpersand = ampersand != -1; - if (ampersand == -1) + if (foundAmpersand) { - if (span.Length > ValueLengthLimit) - { - ThrowValueTooLargeException(); - return; - } - - if (!isFinalBlock) - { - // We can't know that what is currently read is the end of the form value, that's only the case if this is the final block - // If we're not in the final block, then consume nothing - break; - } - - // If we are on the final block, the remaining content in value is what we want to add to the KVAccumulator. - // Clear out the remaining span such that the loop will exit. - span = Span.Empty; + keyValuePair = span.Slice(0, ampersand); + span = span.Slice(keyValuePair.Length + andDelimiter.Length); + consumed += keyValuePair.Length + andDelimiter.Length; } else { - if (ampersand > ValueLengthLimit) + // We can't know that what is currently read is the end of the form value, that's only the case if this is the final block + // If we're not in the final block, then consume nothing + if (!isFinalBlock) + { + // Don't buffer indefinately + if (span.Length > KeyLengthLimit + ValueLengthLimit) + { + ThrowKeyOrValueTooLargeException(); + } + return; + } + + keyValuePair = span; + span = default; + consumed += keyValuePair.Length; + } + + equals = keyValuePair.IndexOf(equalsDelimiter); + + if (equals == -1) + { + // Too long for the whole segment to be a key. + if (keyValuePair.Length > KeyLengthLimit) + { + ThrowKeyTooLargeException(); + } + + // There is no more data, this segment must be "key" with no equals or value. + key = keyValuePair; + value = default; + } + else + { + key = keyValuePair.Slice(0, equals); + if (key.Length > KeyLengthLimit) + { + ThrowKeyTooLargeException(); + } + + value = keyValuePair.Slice(equals + equalsDelimiter.Length); + if (value.Length > ValueLengthLimit) { ThrowValueTooLargeException(); } - - value = span.Slice(0, ampersand); - span = span.Slice(ampersand + andDelimiter.Length); } var decodedKey = GetDecodedString(key); var decodedValue = GetDecodedString(value); AppendAndVerify(ref accumulator, decodedKey, decodedValue); - - // Cover case where we don't have an ampersand at the end. - consumed += key.Length + value.Length + (ampersand == -1 ? equalsDelimiter.Length : equalsDelimiter.Length + andDelimiter.Length); } } @@ -217,6 +222,8 @@ namespace Microsoft.AspNetCore.WebUtilities bool isFinalBlock) { var sequenceReader = new SequenceReader(buffer); + ReadOnlySequence keyValuePair; + var consumed = sequenceReader.Position; var consumedBytes = default(long); var equalsDelimiter = GetEqualsForEncoding(); @@ -224,46 +231,59 @@ namespace Microsoft.AspNetCore.WebUtilities while (!sequenceReader.End) { - // TODO seems there is a bug with TryReadTo (advancePastDelimiter: true). It isn't advancing past the delimiter on second read. - if (!sequenceReader.TryReadTo(out ReadOnlySequence key, equalsDelimiter, advancePastDelimiter: false) || - !sequenceReader.IsNext(equalsDelimiter, true)) - { - if ((sequenceReader.Consumed - consumedBytes) > KeyLengthLimit) - { - ThrowKeyTooLargeException(); - } - - break; - } - - if (key.Length > KeyLengthLimit) - { - ThrowKeyTooLargeException(); - } - - if (!sequenceReader.TryReadTo(out ReadOnlySequence value, andDelimiter, false) || - !sequenceReader.IsNext(andDelimiter, true)) + if (!sequenceReader.TryReadTo(out keyValuePair, andDelimiter)) { if (!isFinalBlock) { - if ((sequenceReader.Consumed - consumedBytes - key.Length) > ValueLengthLimit) + // Don't buffer indefinately + if ((sequenceReader.Consumed - consumedBytes) > KeyLengthLimit + ValueLengthLimit) { - ThrowValueTooLargeException(); + ThrowKeyOrValueTooLargeException(); } break; } - value = buffer.Slice(sequenceReader.Position); - - sequenceReader.Advance(value.Length); + // This must be the final key=value pair + keyValuePair = buffer.Slice(sequenceReader.Position); + sequenceReader.Advance(keyValuePair.Length); } - if (value.Length > ValueLengthLimit) + if (keyValuePair.IsSingleSegment) { - ThrowValueTooLargeException(); + ParseFormValuesFast(keyValuePair.FirstSpan, ref accumulator, isFinalBlock: true, out var segmentConsumed); + Debug.Assert(segmentConsumed == keyValuePair.FirstSpan.Length); + continue; + } + + var keyValueReader = new SequenceReader(keyValuePair); + ReadOnlySequence value; + + if (keyValueReader.TryReadTo(out var key, equalsDelimiter)) + { + if (key.Length > KeyLengthLimit) + { + ThrowKeyTooLargeException(); + } + + value = keyValuePair.Slice(keyValueReader.Position); + if (value.Length > ValueLengthLimit) + { + ThrowValueTooLargeException(); + } + } + else + { + // Too long for the whole segment to be a key. + if (keyValuePair.Length > KeyLengthLimit) + { + ThrowKeyTooLargeException(); + } + + // There is no more data, this segment must be "key" with no equals or value. + key = keyValuePair; + value = default; } - // Need to call ToArray if the key/value spans multiple segments var decodedKey = GetDecodedStringFromReadOnlySequence(key); var decodedValue = GetDecodedStringFromReadOnlySequence(value); @@ -276,6 +296,11 @@ namespace Microsoft.AspNetCore.WebUtilities buffer = buffer.Slice(consumed); } + private void ThrowKeyOrValueTooLargeException() + { + throw new InvalidDataException($"Form key length limit {KeyLengthLimit} or value length limit {ValueLengthLimit} exceeded."); + } + private void ThrowKeyTooLargeException() { throw new InvalidDataException($"Form key length limit {KeyLengthLimit} exceeded."); diff --git a/src/Http/WebUtilities/test/FormPipeReaderTests.cs b/src/Http/WebUtilities/test/FormPipeReaderTests.cs index 192911562f..6c6a6a42bc 100644 --- a/src/Http/WebUtilities/test/FormPipeReaderTests.cs +++ b/src/Http/WebUtilities/test/FormPipeReaderTests.cs @@ -37,7 +37,7 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public async Task ReadFormAsync_EmptyValuedAtEndAllowed() + public async Task ReadFormAsync_EmptyValueAtEndAllowed() { var bodyPipe = await MakePipeReader("foo="); @@ -47,7 +47,17 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public async Task ReadFormAsync_EmptyValuedWithAdditionalEntryAllowed() + public async Task ReadFormAsync_EmptyValueWithoutEqualsAtEndAllowed() + { + var bodyPipe = await MakePipeReader("foo"); + + var formCollection = await ReadFormAsync(new FormPipeReader(bodyPipe)); + + Assert.Equal("", formCollection["foo"].ToString()); + } + + [Fact] + public async Task ReadFormAsync_EmptyValueWithAdditionalEntryAllowed() { var bodyPipe = await MakePipeReader("foo=&baz=2"); @@ -57,6 +67,17 @@ namespace Microsoft.AspNetCore.WebUtilities Assert.Equal("2", formCollection["baz"].ToString()); } + [Fact] + public async Task ReadFormAsync_EmptyValueWithoutEqualsWithAdditionalEntryAllowed() + { + var bodyPipe = await MakePipeReader("foo&baz=2"); + + var formCollection = await ReadFormAsync(new FormPipeReader(bodyPipe)); + + Assert.Equal("", formCollection["foo"].ToString()); + Assert.Equal("2", formCollection["baz"].ToString()); + } + [Fact] public async Task ReadFormAsync_ValueCountLimitMet_Success() { @@ -172,7 +193,7 @@ namespace Microsoft.AspNetCore.WebUtilities [Theory] [MemberData(nameof(Encodings))] - public void TryParseFormValues_MultiSegmentWorks(Encoding encoding) + public void TryParseFormValues_Works(Encoding encoding) { var readOnlySequence = ReadOnlySequenceFactory.SingleSegmentFactory.CreateWithContent(encoding.GetBytes("foo=bar&baz=boo&t=")); @@ -190,9 +211,9 @@ namespace Microsoft.AspNetCore.WebUtilities [Theory] [MemberData(nameof(Encodings))] - public void TryParseFormValues_MultiSegmentSplitAcrossSegmentsWorks(Encoding encoding) + public void TryParseFormValues_SplitAcrossSegmentsWorks(Encoding encoding) { - var readOnlySequence = ReadOnlySequenceFactory.SingleSegmentFactory.CreateWithContent(encoding.GetBytes("foo=bar&baz=boo&t=")); + var readOnlySequence = ReadOnlySequenceFactory.SegmentPerByteFactory.CreateWithContent(encoding.GetBytes("foo=bar&baz=boo&t=")); KeyValueAccumulator accumulator = default; @@ -210,7 +231,7 @@ namespace Microsoft.AspNetCore.WebUtilities [MemberData(nameof(Encodings))] public void TryParseFormValues_MultiSegmentWithArrayPoolAcrossSegmentsWorks(Encoding encoding) { - var readOnlySequence = ReadOnlySequenceFactory.SingleSegmentFactory.CreateWithContent(encoding.GetBytes("foo=bar&baz=bo" + new string('a', 128))); + var readOnlySequence = ReadOnlySequenceFactory.SegmentPerByteFactory.CreateWithContent(encoding.GetBytes("foo=bar&baz=bo" + new string('a', 128))); KeyValueAccumulator accumulator = default; @@ -227,7 +248,7 @@ namespace Microsoft.AspNetCore.WebUtilities [MemberData(nameof(Encodings))] public void TryParseFormValues_MultiSegmentSplitAcrossSegmentsWithPlusesWorks(Encoding encoding) { - var readOnlySequence = ReadOnlySequenceFactory.SingleSegmentFactory.CreateWithContent(encoding.GetBytes("+++=+++&++++=++++&+=")); + var readOnlySequence = ReadOnlySequenceFactory.SegmentPerByteFactory.CreateWithContent(encoding.GetBytes("+++=+++&++++=++++&+=")); KeyValueAccumulator accumulator = default; @@ -261,9 +282,9 @@ namespace Microsoft.AspNetCore.WebUtilities [Theory] [MemberData(nameof(Encodings))] - public void TryParseFormValues_MultiSegmentSplitAcrossSegmentsThatNeedDecodingWorks(Encoding encoding) + public void TryParseFormValues_SplitAcrossSegmentsThatNeedDecodingWorks(Encoding encoding) { - var readOnlySequence = ReadOnlySequenceFactory.SingleSegmentFactory.CreateWithContent(encoding.GetBytes("\"%-.<>\\^_`{|}~=\"%-.<>\\^_`{|}~&\"%-.<>\\^_`{|}=wow")); + var readOnlySequence = ReadOnlySequenceFactory.SegmentPerByteFactory.CreateWithContent(encoding.GetBytes("\"%-.<>\\^_`{|}~=\"%-.<>\\^_`{|}~&\"%-.<>\\^_`{|}=wow")); KeyValueAccumulator accumulator = default; @@ -277,7 +298,7 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public void TryParseFormValues_MultiSegmentExceedKeyLengthThrows() + public void TryParseFormValues_ExceedKeyLengthThrows() { var readOnlySequence = ReadOnlySequenceFactory.SingleSegmentFactory.CreateWithContent(Encoding.UTF8.GetBytes("foo=bar&baz=boo&t=")); @@ -291,7 +312,7 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public void TryParseFormValues_MultiSegmentExceedKeyLengthThrowsInSplitSegment() + public void TryParseFormValues_ExceedKeyLengthThrowsInSplitSegment() { var readOnlySequence = ReadOnlySequenceFactory.CreateSegments(Encoding.UTF8.GetBytes("fo=bar&ba"), Encoding.UTF8.GetBytes("z=boo&t=")); @@ -305,9 +326,9 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public void TryParseFormValues_MultiSegmentExceedValueLengthThrows() + public void TryParseFormValues_ExceedValueLengthThrows() { - var readOnlySequence = ReadOnlySequenceFactory.CreateSegments(Encoding.UTF8.GetBytes("foo=bar&baz=bo"), Encoding.UTF8.GetBytes("o&t=")); + var readOnlySequence = ReadOnlySequenceFactory.CreateSegments(Encoding.UTF8.GetBytes("foo=bar&baz=boo&t=")); KeyValueAccumulator accumulator = default; @@ -319,7 +340,7 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public void TryParseFormValues_MultiSegmentExceedValueLengthThrowsInSplitSegment() + public void TryParseFormValues_ExceedValueLengthThrowsInSplitSegment() { var readOnlySequence = ReadOnlySequenceFactory.CreateSegments(Encoding.UTF8.GetBytes("foo=ba&baz=bo"), Encoding.UTF8.GetBytes("o&t=")); @@ -333,7 +354,7 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public void TryParseFormValues_MultiSegmentExceedKeLengthThrowsInSplitSegmentEnd() + public void TryParseFormValues_ExceedKeyLengthThrowsInSplitSegmentEnd() { var readOnlySequence = ReadOnlySequenceFactory.CreateSegments(Encoding.UTF8.GetBytes("foo=ba&baz=bo"), Encoding.UTF8.GetBytes("o&asdfasdfasd=")); @@ -347,7 +368,7 @@ namespace Microsoft.AspNetCore.WebUtilities } [Fact] - public void TryParseFormValues_MultiSegmentExceedValueLengthThrowsInSplitSegmentEnd() + public void TryParseFormValues_ExceedValueLengthThrowsInSplitSegmentEnd() { var readOnlySequence = ReadOnlySequenceFactory.CreateSegments(Encoding.UTF8.GetBytes("foo=ba&baz=bo"), Encoding.UTF8.GetBytes("o&t=asdfasdfasd")); @@ -389,12 +410,13 @@ namespace Microsoft.AspNetCore.WebUtilities KeyLengthLimit = 3 }; - formReader.ParseFormValues(ref readOnlySequence, ref accumulator, isFinalBlock: false); + formReader.ParseFormValues(ref readOnlySequence, ref accumulator, isFinalBlock: true); IDictionary values = accumulator.GetResults(); - Assert.Single(values); Assert.Contains("fo", values); Assert.Equal("bar", values["fo"]); + Assert.Contains("ba", values); + Assert.Equal("", values["ba"]); } [Theory] @@ -408,12 +430,13 @@ namespace Microsoft.AspNetCore.WebUtilities ValueLengthLimit = 3 }; - formReader.ParseFormValues(ref readOnlySequence, ref accumulator, isFinalBlock: false); + formReader.ParseFormValues(ref readOnlySequence, ref accumulator, isFinalBlock: true); IDictionary values = accumulator.GetResults(); - Assert.Single(values); Assert.Contains("fo", values); Assert.Equal("bar", values["fo"]); + Assert.Contains("b", values); + Assert.Equal("", values["b"]); } public static TheoryData> IncompleteFormKeys =>