From a37fa83aee400f621b1f19fca6a777bd3d804f6f Mon Sep 17 00:00:00 2001
From: David Fowler <davidfowl@gmail.com>
Date: Sun, 1 Apr 2018 00:04:02 -0700
Subject: [PATCH]  Fixed a parser bug found when trying out the array pool
 (#2450)

- When using the array pool, we get terrible block density and as a result
the header parser was failing.
- This fixes the case where the parser needed to skip 2 blocks at the end
(which is unrealistic). Comparing the current index to the reader index is incorrect
since we may end up at the same index in another segment.
---
 src/Kestrel.Core/Internal/Http/HttpParser.cs |  7 ++-
 test/Kestrel.Core.Tests/HttpParserTests.cs   | 51 +++++++++++++++++++-
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/src/Kestrel.Core/Internal/Http/HttpParser.cs b/src/Kestrel.Core/Internal/Http/HttpParser.cs
index 594757eb4c..b3d8d90d9c 100644
--- a/src/Kestrel.Core/Internal/Http/HttpParser.cs
+++ b/src/Kestrel.Core/Internal/Http/HttpParser.cs
@@ -213,6 +213,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
                             var index = reader.CurrentSegmentIndex;
                             int ch1;
                             int ch2;
+                            var readAhead = false;
 
                             // Fast path, we're still looking at the same span
                             if (remaining >= 2)
@@ -229,6 +230,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
                                 // Possibly split across spans
                                 ch1 = reader.Read();
                                 ch2 = reader.Read();
+
+                                readAhead = true;
                             }
 
                             if (ch1 == ByteCR)
@@ -244,7 +247,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
                                 {
                                     // If we got 2 bytes from the span directly so skip ahead 2 so that
                                     // the reader's state matches what we expect
-                                    if (index == reader.CurrentSegmentIndex)
+                                    if (!readAhead)
                                     {
                                         reader.Advance(2);
                                     }
@@ -259,7 +262,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
 
                             // We moved the reader so look ahead 2 bytes so reset both the reader
                             // and the index
-                            if (index != reader.CurrentSegmentIndex)
+                            if (readAhead)
                             {
                                 reader = start;
                                 index = reader.CurrentSegmentIndex;
diff --git a/test/Kestrel.Core.Tests/HttpParserTests.cs b/test/Kestrel.Core.Tests/HttpParserTests.cs
index dec3c68681..f40c25669b 100644
--- a/test/Kestrel.Core.Tests/HttpParserTests.cs
+++ b/test/Kestrel.Core.Tests/HttpParserTests.cs
@@ -27,8 +27,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
             string expectedMethod,
             string expectedRawTarget,
             string expectedRawPath,
-// This warns that theory methods should use all of their parameters,
-// but this method is using a shared data collection with Http1ConnectionTests.TakeStartLineSetsHttpProtocolProperties and others.
+            // This warns that theory methods should use all of their parameters,
+            // but this method is using a shared data collection with Http1ConnectionTests.TakeStartLineSetsHttpProtocolProperties and others.
 #pragma warning disable xUnit1026
             string expectedDecodedPath,
             string expectedQueryString,
@@ -386,6 +386,30 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
             Assert.Equal(buffer.End, examined);
         }
 
+        [Fact]
+        public void ParseHeadersWithGratuitouslySplitBuffers()
+        {
+            var parser = CreateParser(_nullTrace);
+            var buffer = BytePerSegmentTestSequenceFactory.Instance.CreateWithContent("Host:\r\nConnection: keep-alive\r\n\r\n");
+
+            var requestHandler = new RequestHandler();
+            var result = parser.ParseHeaders(requestHandler, buffer, out var consumed, out var examined, out _);
+
+            Assert.True(result);
+        }
+
+        [Fact]
+        public void ParseHeadersWithGratuitouslySplitBuffers2()
+        {
+            var parser = CreateParser(_nullTrace);
+            var buffer = BytePerSegmentTestSequenceFactory.Instance.CreateWithContent("A:B\r\nB: C\r\n\r\n");
+
+            var requestHandler = new RequestHandler();
+            var result = parser.ParseHeaders(requestHandler, buffer, out var consumed, out var examined, out _);
+
+            Assert.True(result);
+        }
+
         private void VerifyHeader(
             string headerName,
             string rawHeaderValue,
@@ -469,5 +493,28 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
                 PathEncoded = pathEncoded;
             }
         }
+
+        // Doesn't put empty blocks inbetween every byte
+        internal class BytePerSegmentTestSequenceFactory : ReadOnlySequenceFactory
+        {
+            public static ReadOnlySequenceFactory Instance { get; } = new HttpParserTests.BytePerSegmentTestSequenceFactory();
+
+            public override ReadOnlySequence<byte> CreateOfSize(int size)
+            {
+                return CreateWithContent(new byte[size]);
+            }
+
+            public override ReadOnlySequence<byte> CreateWithContent(byte[] data)
+            {
+                var segments = new List<byte[]>();
+
+                foreach (var b in data)
+                {
+                    segments.Add(new[] { b });
+                }
+
+                return CreateSegments(segments.ToArray());
+            }
+        }
     }
 }