From e2f8c226efbb8008a342e1b3a12badeb77b7a86d Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Tue, 7 Mar 2017 21:30:57 +0000 Subject: [PATCH] Simplify TakeSingleHeader and Vectorize (#1457) * Sanitize unsafe code * Vectorize * Extract Contains and add more tests --- .../Internal/Http/KestrelHttpParser.cs | 175 ++++++++++-------- test/shared/HttpParsingData.cs | 19 ++ 2 files changed, 113 insertions(+), 81 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs index f85af592fb..d8da5a89bc 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.Numerics; using System.Runtime.CompilerServices; using Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure; using Microsoft.Extensions.Logging; @@ -389,128 +390,126 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private unsafe int IndexOfNameEnd(byte* pBuffer, int index, int length) + private static unsafe int FindEndOfName(byte* headerLine, int length) { - var pCurrent = pBuffer + index; - var pEnd = pBuffer + index + length; - var result = -1; + var index = 0; var sawWhitespace = false; - - while (pCurrent < pEnd) + for (; index < length; index++) { - var ch = *pCurrent; + var ch = headerLine[index]; if (ch == ByteColon) { - if (sawWhitespace) - { - RejectRequest(RequestRejectionReason.WhitespaceIsNotAllowedInHeaderName); - } - - result = index; break; } - - if (ch == ByteTab || ch == ByteSpace) + if (ch == ByteTab || ch == ByteSpace || ch == ByteCR) { sawWhitespace = true; } - - index++; - pCurrent++; } - return result; - } - private unsafe void TakeSingleHeader(byte* pHeader, int headerLineLength, T handler) where T : IHttpHeadersHandler - { - var nameEnd = -1; - var valueStart = -1; - var valueEnd = -1; - var index = 0; - var pCurrent = pHeader + index; - var pEnd = pHeader + headerLineLength; - - nameEnd = IndexOfNameEnd(pHeader, index, headerLineLength); - - if (nameEnd == -1) + if (index == length) { RejectRequest(RequestRejectionReason.NoColonCharacterFoundInHeaderLine); } - - // Skip colon - index += nameEnd + 1; - pCurrent += index; - valueStart = index; - var pValueStart = pCurrent; - - while (pCurrent < pEnd) + if (sawWhitespace) { - var ch = *pCurrent; - if (ch != ByteTab && ch != ByteSpace && ch != ByteCR) - { - valueStart = index; - pValueStart = pCurrent; - break; - } - else if (ch == ByteCR) - { - break; - } - pCurrent++; - index++; + RejectRequest(RequestRejectionReason.WhitespaceIsNotAllowedInHeaderName); } + return index; + } - var endIndex = headerLineLength - 1; - pEnd--; + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe void TakeSingleHeader(byte* headerLine, int length, T handler) where T : IHttpHeadersHandler + { + // Skip CR, LF from end position + var valueEnd = length - 3; + var nameEnd = FindEndOfName(headerLine, length); - if (*pEnd != ByteLF) + if (headerLine[valueEnd + 2] != ByteLF) { RejectRequest(RequestRejectionReason.HeaderValueMustNotContainCR); } - - endIndex--; - pEnd--; - - if (*pEnd != ByteCR) + if (headerLine[valueEnd + 1] != ByteCR) { RejectRequest(RequestRejectionReason.MissingCRInHeaderLine); } - endIndex--; - pEnd--; - - while (pEnd >= pValueStart) + // Skip colon from value start + var valueStart = nameEnd + 1; + // Ignore start whitespace + for(; valueStart < valueEnd; valueStart++) { - var ch = *pEnd; - if (ch != ByteTab && ch != ByteSpace && ch != ByteCR && valueEnd == -1) + var ch = headerLine[valueStart]; + if (ch != ByteTab && ch != ByteSpace && ch != ByteCR) { - valueEnd = endIndex; + break; } else if (ch == ByteCR) { RejectRequest(RequestRejectionReason.HeaderValueMustNotContainCR); } - - pEnd--; - endIndex--; } - if (valueEnd == -1) + + // Check for CR in value + var i = valueStart + 1; + if (Contains(headerLine + i, valueEnd - i, ByteCR)) { - valueEnd = valueStart; + RejectRequest(RequestRejectionReason.HeaderValueMustNotContainCR); } - else + + // Ignore end whitespace + for (; valueEnd > valueStart; valueEnd--) { - valueEnd++; + var ch = headerLine[valueEnd]; + if (ch != ByteTab && ch != ByteSpace) + { + break; + } } - - var nameBuffer = new Span(pHeader, nameEnd); - var valueBuffer = new Span(pHeader + valueStart, valueEnd - valueStart); + var nameBuffer = new Span(headerLine, nameEnd); + var valueBuffer = new Span(headerLine + valueStart, valueEnd - valueStart + 1); handler.OnHeader(nameBuffer, valueBuffer); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static unsafe bool Contains(byte* searchSpace, int length, byte value) + { + var i = 0; + if (Vector.IsHardwareAccelerated) + { + // Check Vector lengths + if (length - Vector.Count >= i) + { + var vValue = GetVector(value); + do + { + if (!Vector.Zero.Equals(Vector.Equals(vValue, Unsafe.Read>(searchSpace + i)))) + { + goto found; + } + + i += Vector.Count; + } while (length - Vector.Count >= i); + } + } + + // Check remaining for CR + for (; i <= length; i++) + { + var ch = searchSpace[i]; + if (ch == value) + { + goto found; + } + } + return false; + found: + return true; + } + private static bool IsValidTokenChar(char c) { // Determines if a character is valid as a 'token' as defined in the @@ -536,20 +535,25 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http c == '~'; } - public void RejectRequest(RequestRejectionReason reason) + public static void RejectRequest(RequestRejectionReason reason) { throw BadHttpRequestException.GetException(reason); } - public void RejectRequest(RequestRejectionReason reason, string value) + public static void RejectRequest(RequestRejectionReason reason, string value) { throw BadHttpRequestException.GetException(reason, value); } private void RejectRequestLine(Span span) + { + throw GetRejectRequestLineException(span); + } + + private BadHttpRequestException GetRejectRequestLineException(Span span) { const int MaxRequestLineError = 32; - RejectRequest(RequestRejectionReason.InvalidRequestLine, + return BadHttpRequestException.GetException(RequestRejectionReason.InvalidRequestLine, Log.IsEnabled(LogLevel.Information) ? span.GetAsciiStringEscaped(MaxRequestLineError) : string.Empty); } @@ -558,6 +562,15 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static Vector GetVector(byte vectorByte) + { + // Vector .ctor doesn't become an intrinsic due to detection issue + // However this does cause it to become an intrinsic (with additional multiply and reg->reg copy) + // https://github.com/dotnet/coreclr/issues/7459#issuecomment-253965670 + return Vector.AsVectorByte(new Vector(vectorByte * 0x01010101u)); + } + private enum HeaderState { Name, diff --git a/test/shared/HttpParsingData.cs b/test/shared/HttpParsingData.cs index 56688b67f0..72a579feb0 100644 --- a/test/shared/HttpParsingData.cs +++ b/test/shared/HttpParsingData.cs @@ -212,6 +212,14 @@ namespace Microsoft.AspNetCore.Testing "Header-1: value1\rHeader-2: value2\r\n\r\n", "Header-1: value1\r\nHeader-2: value2\r\r\n", "Header-1: value1\r\nHeader-2: v\ralue2\r\n", + "Header-1: Value__\rVector16________Vector32\r\n", + "Header-1: Value___Vector16\r________Vector32\r\n", + "Header-1: Value___Vector16_______\rVector32\r\n", + "Header-1: Value___Vector16________Vector32\r\r\n", + "Header-1: Value___Vector16________Vector32_\r\r\n", + "Header-1: Value___Vector16________Vector32Value___Vector16_______\rVector32\r\n", + "Header-1: Value___Vector16________Vector32Value___Vector16________Vector32\r\r\n", + "Header-1: Value___Vector16________Vector32Value___Vector16________Vector32_\r\r\n", }; // Missing colon @@ -236,9 +244,20 @@ namespace Microsoft.AspNetCore.Testing { "Header : value\r\n\r\n", "Header\t: value\r\n\r\n", + "Header\r: value\r\n\r\n", + "Header_\rVector16: value\r\n\r\n", + "Header__Vector16\r: value\r\n\r\n", + "Header__Vector16_\r: value\r\n\r\n", + "Header_\rVector16________Vector32: value\r\n\r\n", + "Header__Vector16________Vector32\r: value\r\n\r\n", + "Header__Vector16________Vector32_\r: value\r\n\r\n", + "Header__Vector16________Vector32Header_\rVector16________Vector32: value\r\n\r\n", + "Header__Vector16________Vector32Header__Vector16________Vector32\r: value\r\n\r\n", + "Header__Vector16________Vector32Header__Vector16________Vector32_\r: value\r\n\r\n", "Header 1: value1\r\nHeader-2: value2\r\n\r\n", "Header 1 : value1\r\nHeader-2: value2\r\n\r\n", "Header 1\t: value1\r\nHeader-2: value2\r\n\r\n", + "Header 1\r: value1\r\nHeader-2: value2\r\n\r\n", "Header-1: value1\r\nHeader 2: value2\r\n\r\n", "Header-1: value1\r\nHeader-2 : value2\r\n\r\n", "Header-1: value1\r\nHeader-2\t: value2\r\n\r\n",