Skip to content

Don't mutate strings in Kestrel #17556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
7 commits merged into from
Feb 10, 2020
Merged

Don't mutate strings in Kestrel #17556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e0584f4
Removed mutating of string-contents in BCryptHandle
gfoidl Dec 3, 2019
67458a3
Revert "Removed mutating of string-contents in BCryptHandle"
gfoidl Dec 3, 2019
d51a8f1
Better perf for StringUtilities.TryGetAsciiString
gfoidl Dec 2, 2019
64e58ae
Removed mutating of created string from HttpUtilities
gfoidl Dec 2, 2019
80a77ff
Use static readonly span-actions as this gives a boost due to not hav…
gfoidl Dec 3, 2019
ca25431
Debug Asserts
gfoidl Jan 22, 2020
b1be648
PR Feedback
gfoidl Feb 10, 2020
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
72 changes: 50 additions & 22 deletions src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Buffers;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
Expand All @@ -30,6 +32,8 @@ internal static partial class HttpUtilities
private const ulong _http11VersionLong = 3543824036068086856; // GetAsciiStringAsLong("HTTP/1.1"); const results in better codegen

private static readonly UTF8EncodingSealed HeaderValueEncoding = new UTF8EncodingSealed();
private static readonly SpanAction<char, IntPtr> _getHeaderName = GetHeaderName;
private static readonly SpanAction<char, IntPtr> _getAsciiStringNonNullCharacters = GetAsciiStringNonNullCharacters;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void SetKnownMethod(ulong mask, ulong knownMethodUlong, HttpMethod knownMethod, int length)
Expand Down Expand Up @@ -86,52 +90,61 @@ private static unsafe ulong GetMaskAsLong(byte[] bytes)
}

// The same as GetAsciiStringNonNullCharacters but throws BadRequest
[MethodImpl(MethodImplOptions.AggressiveInlining)]
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's OK to inline this methods, as in effect it's just the delegate invocation.

public static unsafe string GetHeaderName(this ReadOnlySpan<byte> span)
{
if (span.IsEmpty)
{
return string.Empty;
}

var asciiString = new string('\0', span.Length);
fixed (byte* source = &MemoryMarshal.GetReference(span))
Copy link
Member

Choose a reason for hiding this comment

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

Does this perform better than fixed (byte* buffer = span) used previously?

Copy link
Member Author

@gfoidl gfoidl Feb 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes (it safes one length == 0 check).
GetPinnableReference needs to check for empty span, this check is already done above, so we are sure that we have a non-empty span, so can skip the check for empty by using MemoryMarshal.GetReference.

{
return string.Create(span.Length, new IntPtr(source), _getHeaderName);
}
}

fixed (char* output = asciiString)
fixed (byte* buffer = span)
private static unsafe void GetHeaderName(Span<char> buffer, IntPtr state)
{
fixed (char* output = &MemoryMarshal.GetReference(buffer))
{
// This version if AsciiUtilities returns null if there are any null (0 byte) characters
// in the string
if (!StringUtilities.TryGetAsciiString(buffer, output, span.Length))
if (!StringUtilities.TryGetAsciiString((byte*)state.ToPointer(), output, buffer.Length))
{
BadHttpRequestException.Throw(RequestRejectionReason.InvalidCharactersInHeaderName);
}
}

return asciiString;
}

public static string GetAsciiStringNonNullCharacters(this Span<byte> span)
=> GetAsciiStringNonNullCharacters((ReadOnlySpan<byte>)span);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static unsafe string GetAsciiStringNonNullCharacters(this ReadOnlySpan<byte> span)
{
if (span.IsEmpty)
{
return string.Empty;
}

var asciiString = new string('\0', span.Length);
fixed (byte* source = &MemoryMarshal.GetReference(span))
{
return string.Create(span.Length, new IntPtr(source), _getAsciiStringNonNullCharacters);
}
}

fixed (char* output = asciiString)
fixed (byte* buffer = span)
private static unsafe void GetAsciiStringNonNullCharacters(Span<char> buffer, IntPtr state)
{
fixed (char* output = &MemoryMarshal.GetReference(buffer))
{
// This version if AsciiUtilities returns null if there are any null (0 byte) characters
// in the string
if (!StringUtilities.TryGetAsciiString(buffer, output, span.Length))
if (!StringUtilities.TryGetAsciiString((byte*)state.ToPointer(), output, buffer.Length))
{
throw new InvalidOperationException();
}
}
return asciiString;
}

public static unsafe string GetAsciiOrUTF8StringNonNullCharacters(this Span<byte> span)
Expand All @@ -144,14 +157,12 @@ public static unsafe string GetAsciiOrUTF8StringNonNullCharacters(this ReadOnlyS
return string.Empty;
}

var resultString = new string('\0', span.Length);

fixed (char* output = resultString)
fixed (byte* buffer = span)
fixed (byte* source = &MemoryMarshal.GetReference(span))
{
// This version if AsciiUtilities returns null if there are any null (0 byte) characters
// in the string
if (!StringUtilities.TryGetAsciiString(buffer, output, span.Length))
var resultString = string.Create(span.Length, new IntPtr(source), s_getAsciiOrUtf8StringNonNullCharacters);

// If resultString is marked, perform UTF-8 encoding
if (resultString[0] == '\0')
Copy link
Member Author

Choose a reason for hiding this comment

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

See comment below.

{
// null characters are considered invalid
if (span.IndexOf((byte)0) != -1)
Expand All @@ -161,15 +172,32 @@ public static unsafe string GetAsciiOrUTF8StringNonNullCharacters(this ReadOnlyS

try
{
resultString = HeaderValueEncoding.GetString(buffer, span.Length);
resultString = HeaderValueEncoding.GetString(span);
}
catch (DecoderFallbackException)
{
throw new InvalidOperationException();
}
}

return resultString;
}
}

private static readonly SpanAction<char, IntPtr> s_getAsciiOrUtf8StringNonNullCharacters = GetAsciiOrUTF8StringNonNullCharacters;

private static unsafe void GetAsciiOrUTF8StringNonNullCharacters(Span<char> buffer, IntPtr state)
{
fixed (char* output = &MemoryMarshal.GetReference(buffer))
{
// This version if AsciiUtilities returns null if there are any null (0 byte) characters
// in the string
if (!StringUtilities.TryGetAsciiString((byte*)state.ToPointer(), output, buffer.Length))
{
// Mark resultString for UTF-8 encoding
output[0] = '\0';
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't find a better solution on how to communicate back that non-acii data is available. Except using exceptions, but it's not exceptional and slow / miuse of exceptions.

That's why is marked with a "flag", which is checked above at L167.

}
}
return resultString;
}

public static string GetAsciiStringEscaped(this Span<byte> span, int maxChars)
Expand Down Expand Up @@ -288,7 +316,7 @@ public static HttpMethod GetKnownMethod(string value)
{
method = HttpMethod.Head;
}
else if(firstChar == 'P' && string.Equals(value, HttpMethods.Post, StringComparison.Ordinal))
else if (firstChar == 'P' && string.Equals(value, HttpMethods.Post, StringComparison.Ordinal))
{
method = HttpMethod.Post;
}
Expand All @@ -299,7 +327,7 @@ public static HttpMethod GetKnownMethod(string value)
{
method = HttpMethod.Trace;
}
else if(firstChar == 'P' && string.Equals(value, HttpMethods.Patch, StringComparison.Ordinal))
else if (firstChar == 'P' && string.Equals(value, HttpMethods.Patch, StringComparison.Ordinal))
{
method = HttpMethod.Patch;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Shared/Http2cat/Http2Utilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace Microsoft.AspNetCore.Http2Cat
internal class Http2Utilities : IHttpHeadersHandler
{
public static ReadOnlySpan<byte> ClientPreface => new byte[24] { (byte)'P', (byte)'R', (byte)'I', (byte)' ', (byte)'*', (byte)' ', (byte)'H', (byte)'T', (byte)'T', (byte)'P', (byte)'/', (byte)'2', (byte)'.', (byte)'0', (byte)'\r', (byte)'\n', (byte)'\r', (byte)'\n', (byte)'S', (byte)'M', (byte)'\r', (byte)'\n', (byte)'\r', (byte)'\n' };
public static readonly int MaxRequestHeaderFieldSize = 16 * 1024;
public const int MaxRequestHeaderFieldSize = 16 * 1024;
public static readonly string FourKHeaderValue = new string('a', 4096);
private static readonly Encoding HeaderValueEncoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true);

Expand Down
Loading