From fe2ae36d2154b4e98bb976db5a45873c0e81ba08 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 15 Feb 2018 16:04:40 -0500 Subject: [PATCH] Streamline SocketHttpHandler's ParseStatusLine validation (#27163) * Streamline SocketHttpHandler's ParseStatusLine validation For a typical status line like "HTTP/1.1 200 OK", cuts the time of ParseStatusLine almost in half. * Address PR feedback --- .../System/Net/Http/HttpResponseMessage.cs | 6 ++ .../Http/SocketsHttpHandler/HttpConnection.cs | 86 ++++++++++++------- .../FunctionalTests/HttpProtocolTests.cs | 7 +- 3 files changed, 62 insertions(+), 37 deletions(-) diff --git a/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs b/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs index 3e368add2ac3..9df1d1c98988 100644 --- a/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs +++ b/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs @@ -36,6 +36,8 @@ public Version Version } } + internal void SetVersionWithoutValidation(Version value) => _version = value; + public HttpContent Content { get { return _content; } @@ -74,6 +76,8 @@ public HttpStatusCode StatusCode } } + internal void SetStatusCodeWithoutValidation(HttpStatusCode value) => _statusCode = value; + public string ReasonPhrase { get @@ -97,6 +101,8 @@ public string ReasonPhrase } } + internal void SetReasonPhraseWithoutValidation(string value) => _reasonPhrase = value; + public HttpResponseHeaders Headers { get diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs index ed3567219e59..f84afdf8557c 100644 --- a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs +++ b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs @@ -43,6 +43,9 @@ internal partial class HttpConnection : IDisposable private static readonly byte[] s_spaceHttp11NewlineAsciiBytes = Encoding.ASCII.GetBytes(" HTTP/1.1\r\n"); private static readonly byte[] s_hostKeyAndSeparator = Encoding.ASCII.GetBytes(HttpKnownHeaderNames.Host + ": "); private static readonly byte[] s_httpSchemeAndDelimiter = Encoding.ASCII.GetBytes(Uri.UriSchemeHttp + Uri.SchemeDelimiter); + private static readonly byte[] s_http1DotBytes = Encoding.ASCII.GetBytes("HTTP/1."); + private static readonly ulong s_http10Bytes = BitConverter.ToUInt64(Encoding.ASCII.GetBytes("HTTP/1.0")); + private static readonly ulong s_http11Bytes = BitConverter.ToUInt64(Encoding.ASCII.GetBytes("HTTP/1.1")); private static readonly string s_cancellationMessage = new OperationCanceledException().Message; // use same message as the default ctor private readonly HttpConnectionPool _pool; @@ -635,36 +638,43 @@ private async Task SendRequestContentWithExpect100ContinueAsync( // TODO: Remove this overload once https://github.com/dotnet/roslyn/issues/17287 is addressed // and the compiler doesn't lift the span temporary from the call site into the async state // machine in debug builds. - private void ParseStatusLine(ArraySegment line, HttpResponseMessage response) => + private static void ParseStatusLine(ArraySegment line, HttpResponseMessage response) => ParseStatusLine((Span)line, response); - private void ParseStatusLine(Span line, HttpResponseMessage response) + private static void ParseStatusLine(Span line, HttpResponseMessage response) { // We sent the request version as either 1.0 or 1.1. // We expect a response version of the form 1.X, where X is a single digit as per RFC. - const int MinStatusLineLength = 12; // "HTTP/1.1 123" - - if (line.Length < MinStatusLineLength || - line[0] != 'H' || - line[1] != 'T' || - line[2] != 'T' || - line[3] != 'P' || - line[4] != '/' || - line[5] != '1' || - line[6] != '.' || - line[8] != ' ') + // Validate the beginning of the status line and set the response version. + const int MinStatusLineLength = 12; // "HTTP/1.x 123" + if (line.Length < MinStatusLineLength || line[8] != ' ') { ThrowInvalidHttpResponse(); } - // Set the response HttpVersion - byte minorVersion = line[7]; - response.Version = - minorVersion == '1' ? HttpVersion.Version11 : - minorVersion == '0' ? HttpVersion.Version10 : - !IsDigit(minorVersion) ? throw new HttpRequestException(SR.net_http_invalid_response) : - new Version(1, minorVersion - '0'); + ulong first8Bytes = BitConverter.ToUInt64(line); + if (first8Bytes == s_http11Bytes) + { + response.SetVersionWithoutValidation(HttpVersion.Version11); + } + else if (first8Bytes == s_http10Bytes) + { + response.SetVersionWithoutValidation(HttpVersion.Version10); + } + else + { + byte minorVersion = line[7]; + if (IsDigit(minorVersion) && + line.Slice(0, 7).SequenceEqual(s_http1DotBytes)) + { + response.SetVersionWithoutValidation(new Version(1, minorVersion - '0')); + } + else + { + ThrowInvalidHttpResponse(); + } + } // Set the status code byte status1 = line[9], status2 = line[10], status3 = line[11]; @@ -672,36 +682,46 @@ private void ParseStatusLine(Span line, HttpResponseMessage response) { ThrowInvalidHttpResponse(); } - - response.StatusCode = - (HttpStatusCode)(100 * (status1 - '0') + 10 * (status2 - '0') + (status3 - '0')); + response.SetStatusCodeWithoutValidation((HttpStatusCode)(100 * (status1 - '0') + 10 * (status2 - '0') + (status3 - '0'))); // Parse (optional) reason phrase if (line.Length == MinStatusLineLength) { - response.ReasonPhrase = string.Empty; + response.SetReasonPhraseWithoutValidation(string.Empty); } - else if (line[MinStatusLineLength] != ' ') + else if (line[MinStatusLineLength] == ' ') { - ThrowInvalidHttpResponse(); + Span reasonBytes = line.Slice(MinStatusLineLength + 1); + string knownReasonPhrase = HttpStatusDescription.Get(response.StatusCode); + if (knownReasonPhrase != null && EqualsOrdinal(knownReasonPhrase, reasonBytes)) + { + response.SetReasonPhraseWithoutValidation(knownReasonPhrase); + } + else + { + try + { + response.ReasonPhrase = HttpRuleParser.DefaultHttpEncoding.GetString(reasonBytes); + } + catch (FormatException error) + { + ThrowInvalidHttpResponse(error); + } + } } else { - Span reasonBytes = line.Slice(MinStatusLineLength + 1); - string knownReasonPhrase = HttpStatusDescription.Get(response.StatusCode); - response.ReasonPhrase = knownReasonPhrase != null && EqualsOrdinal(knownReasonPhrase, reasonBytes) ? - knownReasonPhrase : - HttpRuleParser.DefaultHttpEncoding.GetString(reasonBytes); + ThrowInvalidHttpResponse(); } } // TODO: Remove this overload once https://github.com/dotnet/roslyn/issues/17287 is addressed // and the compiler doesn't lift the span temporary from the call site into the async state // machine in debug builds. - private void ParseHeaderNameValue(ArraySegment line, HttpResponseMessage response) => + private static void ParseHeaderNameValue(ArraySegment line, HttpResponseMessage response) => ParseHeaderNameValue((Span)line, response); - private void ParseHeaderNameValue(Span line, HttpResponseMessage response) + private static void ParseHeaderNameValue(Span line, HttpResponseMessage response) { Debug.Assert(line.Length > 0); diff --git a/src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs b/src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs index 3a37d77319d6..2ee1847eb37e 100644 --- a/src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs +++ b/src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs @@ -403,13 +403,14 @@ public static IEnumerable GetInvalidStatusLine() yield return "HTTP/A.1 200 OK"; yield return "HTTP/X.Y.Z 200 OK"; - // Only pass on .NET Core Windows & ManagedHandler. + // Only pass on .NET Core Windows & SocketsHttpHandler. if (PlatformDetection.IsNetCore && PlatformDetection.IsWindows) { yield return "HTTP/0.1 200 OK"; yield return "HTTP/3.5 200 OK"; yield return "HTTP/1.12 200 OK"; yield return "HTTP/12.1 200 OK"; + yield return "HTTP/1.1 200 O\rK"; } // Skip these test cases on CurlHandler since the behavior is different. @@ -442,8 +443,6 @@ public static IEnumerable GetInvalidStatusLine() yield return "ABCD/1.1 200 OK"; yield return "HTTP/1.1"; yield return "HTTP\\1.1 200 OK"; - // ActiveIssue: #26946 - // yield return "HTTP/1.1 200 O\rK"; yield return "NOTHTTP/1.1 200 OK"; } } @@ -471,7 +470,7 @@ public async Task GetAsync_ReasonPhraseHasLF_BehaviorDifference() } else { - // UAP and ManagedHandler will allow LF ending. + // UAP and SocketsHttpHandler will allow LF ending. await GetAsyncSuccessHelper(responseString, expectedStatusCode, expectedReason); } }