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.
This commit is contained in:
David Fowler 2018-04-01 00:04:02 -07:00 committed by GitHub
parent 7382198356
commit a37fa83aee
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 54 additions and 4 deletions

View File

@ -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;

View File

@ -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());
}
}
}
}