Fix form pipe parser #12381 (#12749)

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[] _firstUtf8 = Encoding.UTF8.GetBytes("foo=bar&baz=bo");
-        private byte[] _secondUtf8 = Encoding.UTF8.GetBytes("&haha=hehe&lol=temp");
+        private byte[] _secondUtf8 = Encoding.UTF8.GetBytes("o&haha=hehe&lol=temp");
         private FormPipeReader _formPipeReader;
 
         [IterationSetup]

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<byte> key = default;
+            ReadOnlySpan<byte> key;
-            ReadOnlySpan<byte> value = default;
+            ReadOnlySpan<byte> value;
             consumed = 0;
             var equalsDelimiter = GetEqualsForEncoding();
             var andDelimiter = GetAndForEncoding();
 
             while (span.Length > 0)
             {
-                var equals = span.IndexOf(equalsDelimiter);
+                // Find the end of the key=value pair.
-
-                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;
-
                 var ampersand = span.IndexOf(andDelimiter);
+                ReadOnlySpan<byte> keyValuePair;
+                int equals;
+                var foundAmpersand = ampersand != -1;
 
-                if (ampersand == -1)
+                if (foundAmpersand)
                 {
-                    if (span.Length > ValueLengthLimit)
+                    keyValuePair = span.Slice(0, ampersand);
-                    {
+                    span = span.Slice(keyValuePair.Length + andDelimiter.Length);
-                        ThrowValueTooLargeException();
+                    consumed += keyValuePair.Length + andDelimiter.Length;
-                        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<byte>.Empty;
                 }
                 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<byte>(buffer);
+            ReadOnlySequence<byte> 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 keyValuePair, andDelimiter))
-                if (!sequenceReader.TryReadTo(out ReadOnlySequence<byte> 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<byte> value, andDelimiter, false) ||
-                    !sequenceReader.IsNext(andDelimiter, true))
                 {
                     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);
+                    // This must be the final key=value pair
-
+                    keyValuePair = buffer.Slice(sequenceReader.Position);
-                    sequenceReader.Advance(value.Length);
+                    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<byte>(keyValuePair);
+                ReadOnlySequence<byte> 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.");

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<string, StringValues> 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<string, StringValues> 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<ReadOnlySequence<byte>> IncompleteFormKeys =>