Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Streamline SocketHttpHandler's ParseStatusLine validation #27163

Merged
merged 2 commits into from
Feb 15, 2018
Merged

Streamline SocketHttpHandler's ParseStatusLine validation #27163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59e373a
Streamline SocketHttpHandler's ParseStatusLine validation
stephentoub Feb 15, 2018
2dca128
Address PR feedback
stephentoub Feb 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ public Version Version
}
}

internal void SetVersionWithoutValidation(Version value) => _version = value;

public HttpContent Content
{
get { return _content; }
Expand Down Expand Up @@ -74,6 +76,8 @@ public HttpStatusCode StatusCode
}
}

internal void SetStatusCodeWithoutValidation(HttpStatusCode value) => _statusCode = value;

public string ReasonPhrase
{
get
Expand All @@ -97,6 +101,8 @@ public string ReasonPhrase
}
}

internal void SetReasonPhraseWithoutValidation(string value) => _reasonPhrase = value;

public HttpResponseHeaders Headers
{
get
Expand Down
86 changes: 53 additions & 33 deletions src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -635,73 +638,90 @@ 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<byte> line, HttpResponseMessage response) =>
private static void ParseStatusLine(ArraySegment<byte> line, HttpResponseMessage response) =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're in this code anyway :), can we remove this overload per comment above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, no, the C# compiler still prevents span locals in an async method. @VSadov, is there an open issue tracking that I can link to? The overall issue I linked to above is closed.

ParseStatusLine((Span<byte>)line, response);

private void ParseStatusLine(Span<byte> line, HttpResponseMessage response)
private static void ParseStatusLine(Span<byte> 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];
if (!IsDigit(status1) || !IsDigit(status2) || !IsDigit(status3))
{
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<byte> 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<byte> 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<byte> line, HttpResponseMessage response) =>
private static void ParseHeaderNameValue(ArraySegment<byte> line, HttpResponseMessage response) =>
ParseHeaderNameValue((Span<byte>)line, response);

private void ParseHeaderNameValue(Span<byte> line, HttpResponseMessage response)
private static void ParseHeaderNameValue(Span<byte> line, HttpResponseMessage response)
{
Debug.Assert(line.Length > 0);

Expand Down
7 changes: 3 additions & 4 deletions src/System.Net.Http/tests/FunctionalTests/HttpProtocolTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,13 +403,14 @@ public static IEnumerable<string> 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.
Expand Down Expand Up @@ -442,8 +443,6 @@ public static IEnumerable<string> 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";
}
}
Expand Down Expand Up @@ -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);
}
}
Expand Down