From 02a434290824a1d9ef4496f6bb6a742d156be465 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 7 Mar 2017 08:58:55 -0800 Subject: [PATCH] Made changes to TakeSingleHeader (#1453) * Made changes to TakeSingleHeader - Remove state machine and just parse in place - Inline OnHeader into TakeSingleHeader - Use IndexOfVectorized instead of custom indexof - Normalize header whitespace error - Combine IndexOf and IndexOfAny into a single IndexOfNameEnd call --- .../BadHttpRequestException.cs | 3 - .../Internal/Http/KestrelHttpParser.cs | 273 ++++++++---------- .../Internal/Http/RequestRejectionReason.cs | 1 - .../FrameTests.cs | 2 +- test/shared/HttpParsingData.cs | 4 +- 5 files changed, 131 insertions(+), 152 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs b/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs index a19fd6db35..54533f05e1 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs @@ -25,9 +25,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel case RequestRejectionReason.HeadersCorruptedInvalidHeaderSequence: ex = new BadHttpRequestException("Headers corrupted, invalid header sequence.", StatusCodes.Status400BadRequest); break; - case RequestRejectionReason.HeaderLineMustNotStartWithWhitespace: - ex = new BadHttpRequestException("Header line must not start with whitespace.", StatusCodes.Status400BadRequest); - break; case RequestRejectionReason.NoColonCharacterFoundInHeaderLine: ex = new BadHttpRequestException("No ':' character found in header line.", StatusCodes.Status400BadRequest); break; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs index 998aa6f46a..f85af592fb 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs @@ -4,6 +4,7 @@ using System; using System.Buffers; using System.IO.Pipelines; +using System.Runtime.CompilerServices; using Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure; using Microsoft.Extensions.Logging; @@ -316,9 +317,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http // Headers don't end in CRLF line. RejectRequest(RequestRejectionReason.HeadersCorruptedInvalidHeaderSequence); } - else if (ch1 == ByteSpace || ch1 == ByteTab) + else if(ch1 == ByteSpace || ch1 == ByteTab) { - RejectRequest(RequestRejectionReason.HeaderLineMustNotStartWithWhitespace); + RejectRequest(RequestRejectionReason.WhitespaceIsNotAllowedInHeaderName); } // We moved the reader so look ahead 2 bytes so reset both the reader @@ -329,13 +330,15 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http index = reader.Index; } - Span headerSpan; - - var endIndex = IndexOf(pBuffer, index, remaining, ByteLF); + var endIndex = new Span(pBuffer + index, remaining).IndexOfVectorized(ByteLF); + var length = 0; if (endIndex != -1) { - headerSpan = new Span(pBuffer + index, (endIndex - index + 1)); + length = endIndex + 1; + var pHeader = pBuffer + index; + + TakeSingleHeader(pHeader, length, handler); } else { @@ -350,28 +353,23 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http // Make sure LF is included in lineEnd lineEnd = buffer.Move(lineEnd, 1); - headerSpan = buffer.Slice(current, lineEnd).ToSpan(); + var headerSpan = buffer.Slice(current, lineEnd).ToSpan(); + length = headerSpan.Length; + + fixed (byte* pHeader = &headerSpan.DangerousGetPinnableReference()) + { + TakeSingleHeader(pHeader, length, handler); + } // We're going to the next span after this since we know we crossed spans here // so mark the remaining as equal to the headerSpan so that we end up at 0 // on the next iteration - remaining = headerSpan.Length; - } - - if (!TakeSingleHeader(headerSpan, out var nameStart, out var nameEnd, out var valueStart, out var valueEnd)) - { - return false; + remaining = length; } // Skip the reader forward past the header line - reader.Skip(headerSpan.Length); - - remaining -= headerSpan.Length; - - var nameBuffer = headerSpan.Slice(nameStart, nameEnd - nameStart); - var valueBuffer = headerSpan.Slice(valueStart, valueEnd - valueStart); - - handler.OnHeader(nameBuffer, valueBuffer); + reader.Skip(length); + remaining -= length; } } } @@ -390,142 +388,127 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } } - private unsafe int IndexOf(byte* pBuffer, int index, int length, byte value) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe int IndexOfNameEnd(byte* pBuffer, int index, int length) { - for (int i = 0; i < length; i++, index++) + var pCurrent = pBuffer + index; + var pEnd = pBuffer + index + length; + var result = -1; + var sawWhitespace = false; + + while (pCurrent < pEnd) { - if (pBuffer[index] == value) + var ch = *pCurrent; + if (ch == ByteColon) { - return index; + if (sawWhitespace) + { + RejectRequest(RequestRejectionReason.WhitespaceIsNotAllowedInHeaderName); + } + + result = index; + break; } + + if (ch == ByteTab || ch == ByteSpace) + { + sawWhitespace = true; + } + + index++; + pCurrent++; } - return -1; + return result; } - private unsafe bool TakeSingleHeader(Span span, out int nameStart, out int nameEnd, out int valueStart, out int valueEnd) + private unsafe void TakeSingleHeader(byte* pHeader, int headerLineLength, T handler) where T : IHttpHeadersHandler { - nameStart = 0; - nameEnd = -1; - valueStart = -1; - valueEnd = -1; - var headerLineLength = span.Length; - var nameHasWhitespace = false; - var previouslyWhitespace = false; + var nameEnd = -1; + var valueStart = -1; + var valueEnd = -1; + var index = 0; + var pCurrent = pHeader + index; + var pEnd = pHeader + headerLineLength; - int i = 0; - var done = false; - fixed (byte* pHeader = &span.DangerousGetPinnableReference()) + nameEnd = IndexOfNameEnd(pHeader, index, headerLineLength); + + if (nameEnd == -1) { - switch (HeaderState.Name) - { - case HeaderState.Name: - for (; i < headerLineLength; i++) - { - var ch = pHeader[i]; - if (ch == ByteColon) - { - if (nameHasWhitespace) - { - RejectRequest(RequestRejectionReason.WhitespaceIsNotAllowedInHeaderName); - } - nameEnd = i; - - // Consume space - i++; - - goto case HeaderState.Whitespace; - } - - if (ch == ByteSpace || ch == ByteTab) - { - nameHasWhitespace = true; - } - } - RejectRequest(RequestRejectionReason.NoColonCharacterFoundInHeaderLine); - - break; - case HeaderState.Whitespace: - for (; i < headerLineLength; i++) - { - var ch = pHeader[i]; - var whitespace = ch == ByteTab || ch == ByteSpace || ch == ByteCR; - - if (!whitespace) - { - // Mark the first non whitespace char as the start of the - // header value and change the state to expect to the header value - valueStart = i; - - goto case HeaderState.ExpectValue; - } - // If we see a CR then jump to the next state directly - else if (ch == ByteCR) - { - goto case HeaderState.ExpectValue; - } - } - - RejectRequest(RequestRejectionReason.MissingCRInHeaderLine); - - break; - case HeaderState.ExpectValue: - for (; i < headerLineLength; i++) - { - var ch = pHeader[i]; - var whitespace = ch == ByteTab || ch == ByteSpace; - - if (whitespace) - { - if (!previouslyWhitespace) - { - // If we see a whitespace char then maybe it's end of the - // header value - valueEnd = i; - } - } - else if (ch == ByteCR) - { - // If we see a CR and we haven't ever seen whitespace then - // this is the end of the header value - if (valueEnd == -1) - { - valueEnd = i; - } - - // We never saw a non whitespace character before the CR - if (valueStart == -1) - { - valueStart = valueEnd; - } - - // Consume space - i++; - - goto case HeaderState.ExpectNewLine; - } - else - { - // If we find a non whitespace char that isn't CR then reset the end index - valueEnd = -1; - } - - previouslyWhitespace = whitespace; - } - RejectRequest(RequestRejectionReason.MissingCRInHeaderLine); - break; - case HeaderState.ExpectNewLine: - if (pHeader[i] != ByteLF) - { - RejectRequest(RequestRejectionReason.HeaderValueMustNotContainCR); - } - goto case HeaderState.Complete; - case HeaderState.Complete: - done = true; - break; - } + RejectRequest(RequestRejectionReason.NoColonCharacterFoundInHeaderLine); } - return done; + // Skip colon + index += nameEnd + 1; + pCurrent += index; + valueStart = index; + var pValueStart = pCurrent; + + while (pCurrent < pEnd) + { + var ch = *pCurrent; + if (ch != ByteTab && ch != ByteSpace && ch != ByteCR) + { + valueStart = index; + pValueStart = pCurrent; + break; + } + else if (ch == ByteCR) + { + break; + } + pCurrent++; + index++; + } + + var endIndex = headerLineLength - 1; + pEnd--; + + if (*pEnd != ByteLF) + { + RejectRequest(RequestRejectionReason.HeaderValueMustNotContainCR); + } + + endIndex--; + pEnd--; + + if (*pEnd != ByteCR) + { + RejectRequest(RequestRejectionReason.MissingCRInHeaderLine); + } + + endIndex--; + pEnd--; + + while (pEnd >= pValueStart) + { + var ch = *pEnd; + if (ch != ByteTab && ch != ByteSpace && ch != ByteCR && valueEnd == -1) + { + valueEnd = endIndex; + } + else if (ch == ByteCR) + { + RejectRequest(RequestRejectionReason.HeaderValueMustNotContainCR); + } + + pEnd--; + endIndex--; + } + + if (valueEnd == -1) + { + valueEnd = valueStart; + } + else + { + valueEnd++; + } + + + var nameBuffer = new Span(pHeader, nameEnd); + var valueBuffer = new Span(pHeader + valueStart, valueEnd - valueStart); + + handler.OnHeader(nameBuffer, valueBuffer); } private static bool IsValidTokenChar(char c) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs index 042a25d99c..b0aa5a1372 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/RequestRejectionReason.cs @@ -7,7 +7,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { UnrecognizedHTTPVersion, HeadersCorruptedInvalidHeaderSequence, - HeaderLineMustNotStartWithWhitespace, NoColonCharacterFoundInHeaderLine, WhitespaceIsNotAllowedInHeaderName, HeaderValueMustNotContainCR, diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs index ebfd64fea4..46d54d1b40 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs @@ -200,7 +200,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests var exception = Assert.Throws(() => _frame.TakeMessageHeaders(readableBuffer, out _consumed, out _examined)); _socketInput.Reader.Advance(_consumed, _examined); - Assert.Equal("Header line must not start with whitespace.", exception.Message); + Assert.Equal("Whitespace is not allowed in header name.", exception.Message); Assert.Equal(StatusCodes.Status400BadRequest, exception.StatusCode); } diff --git a/test/shared/HttpParsingData.cs b/test/shared/HttpParsingData.cs index 50a5633c9c..56688b67f0 100644 --- a/test/shared/HttpParsingData.cs +++ b/test/shared/HttpParsingData.cs @@ -254,10 +254,10 @@ namespace Microsoft.AspNetCore.Testing return new[] { - Tuple.Create(headersWithLineFolding,"Header line must not start with whitespace."), + Tuple.Create(headersWithLineFolding,"Whitespace is not allowed in header name."), Tuple.Create(headersWithCRInValue,"Header value must not contain CR characters."), Tuple.Create(headersWithMissingColon,"No ':' character found in header line."), - Tuple.Create(headersStartingWithWhitespace, "Header line must not start with whitespace."), + Tuple.Create(headersStartingWithWhitespace, "Whitespace is not allowed in header name."), Tuple.Create(headersWithWithspaceInName,"Whitespace is not allowed in header name."), Tuple.Create(headersNotEndingInCrLfLine, "Headers corrupted, invalid header sequence.") }