From 95a1708004150515285afe94e7127bc4dbb94e6c Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Tue, 17 Oct 2023 10:50:22 -0700 Subject: [PATCH] Avoid using IncrementalHash on .NET Framework On .NET Framework, IncrementalHash uses the OS's implementation of SHA-256, which results in several safe handles being created. These handles are finalizable and can cause lock contention in the CLR if created rapidly, which Razor does. This change adjusts the Checksum.Builder to use the SHA256 class directly on .NET Framework (and netstandard2.0), which causes the managed implementation of SHA256 to be used (if FIPS isn't enabled). That implementation avoids the creation of finalizable safe handles. See https://github.com/dotnet/roslyn/issues/67995 for more detail. --- .../IncrementalHashPool.Policy.cs | 29 ---- .../PooledObjects/IncrementalHashPool.cs | 26 ---- .../Utilities/Checksum.Builder.Policy.cs | 55 +++++++ .../Utilities/Checksum.Builder.cs | 147 ++++++++++++------ .../Utilities/Checksum.cs | 3 +- 5 files changed, 154 insertions(+), 106 deletions(-) delete mode 100644 src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.Policy.cs delete mode 100644 src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.cs create mode 100644 src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.Policy.cs diff --git a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.Policy.cs b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.Policy.cs deleted file mode 100644 index 2674cf699a6..00000000000 --- a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.Policy.cs +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the MIT license. See License.txt in the project root for license information. - -using System.Diagnostics; -using System.Security.Cryptography; -using Microsoft.Extensions.ObjectPool; - -namespace Microsoft.AspNetCore.Razor.PooledObjects; - -internal static partial class IncrementalHashPool -{ - private class Policy : IPooledObjectPolicy - { - public static readonly Policy Instance = new(); - - private Policy() - { - } - - public IncrementalHash Create() => IncrementalHash.CreateHash(HashAlgorithmName.SHA256); - - public bool Return(IncrementalHash hash) - { - Debug.Assert(hash.AlgorithmName == HashAlgorithmName.SHA256); - - return true; - } - } -} diff --git a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.cs b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.cs deleted file mode 100644 index 726ab78833b..00000000000 --- a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/IncrementalHashPool.cs +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the MIT license. See License.txt in the project root for license information. - -using System.Security.Cryptography; -using Microsoft.Extensions.ObjectPool; - -namespace Microsoft.AspNetCore.Razor.PooledObjects; - -/// -/// A pool of instances. -/// -/// -/// -/// Instances originating from this pool are intended to be short-lived and are suitable -/// for temporary work. Do not return them as the results of methods or store them in fields. -/// -internal static partial class IncrementalHashPool -{ - public static readonly ObjectPool Default = DefaultPool.Create(Policy.Instance); - - public static PooledObject GetPooledObject() - => Default.GetPooledObject(); - - public static PooledObject GetPooledObject(out IncrementalHash hash) - => Default.GetPooledObject(out hash); -} diff --git a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.Policy.cs b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.Policy.cs new file mode 100644 index 00000000000..37460e95b2c --- /dev/null +++ b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.Policy.cs @@ -0,0 +1,55 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the MIT license. See License.txt in the project root for license information. + +using Microsoft.Extensions.ObjectPool; +#if NET +using System.Diagnostics; +#endif +using System.Security.Cryptography; + +namespace Microsoft.AspNetCore.Razor.Utilities; + +internal sealed partial record Checksum +{ + internal readonly ref partial struct Builder + { + +#if NET + private sealed class Policy : IPooledObjectPolicy + { + public static readonly Policy Instance = new(); + + private Policy() + { + } + + public IncrementalHash Create() + => IncrementalHash.CreateHash(HashAlgorithmName.SHA256); + + public bool Return(IncrementalHash hash) + { + Debug.Assert(hash.AlgorithmName == HashAlgorithmName.SHA256); + + return true; + } + } +#else + private sealed class Policy : IPooledObjectPolicy + { + public static readonly Policy Instance = new(); + + private Policy() + { + } + + public SHA256 Create() + => SHA256.Create(); + + public bool Return(SHA256 hash) + { + return true; + } + } +#endif + } +} diff --git a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.cs b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.cs index c41cefed5fe..f3e581ff9ca 100644 --- a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.cs +++ b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.Builder.cs @@ -2,19 +2,36 @@ // Licensed under the MIT license. See License.txt in the project root for license information. using System; +#if !NET using System.Buffers; +#endif using System.Buffers.Binary; using System.Diagnostics; using System.Runtime.InteropServices; using System.Security.Cryptography; using Microsoft.AspNetCore.Razor.PooledObjects; +using Microsoft.Extensions.ObjectPool; namespace Microsoft.AspNetCore.Razor.Utilities; internal sealed partial record Checksum { - internal readonly ref struct Builder + // PERFORMANCE: Care has been taken to avoid using IncrementalHash on .NET Framework, which can cause + // threadpool starvation. Essentially, on .NET Framework, IncrementalHash ends up using the OS implementation + // of SHA-256, which creates several finalizable objects in the form of safe handles. By creating instances + // of SHA256 for .NET Framework (and netstandard2.0), we get the managed code version of SHA-256, which + // doesn't have the overhead of using the OS implementations. + // + // See https://github.com/dotnet/roslyn/issues/67995 for more detail. + + internal readonly ref partial struct Builder { +#if NET + private static readonly ObjectPool s_incrementalHashPool = DefaultPool.Create(Policy.Instance); +#else + private static readonly ObjectPool s_incrementalHashPool = DefaultPool.Create(Policy.Instance); +#endif + private enum TypeKind : byte { Null, @@ -31,11 +48,19 @@ private enum TypeKind : byte [ThreadStatic] private static byte[]? s_buffer; +#if NET5_0_OR_GREATER private readonly IncrementalHash _hash; +#else + private readonly SHA256 _hash; +#endif public Builder() { - _hash = IncrementalHashPool.Default.Get(); + _hash = s_incrementalHashPool.Get(); + +#if !NET5_0_OR_GREATER + _hash.Initialize(); +#endif } static byte[] GetBuffer() @@ -43,119 +68,141 @@ static byte[] GetBuffer() public Checksum FreeAndGetChecksum() { - var result = From(_hash.GetHashAndReset()); - IncrementalHashPool.Default.Return(_hash); +#if NET + Span hash = stackalloc byte[HashSize]; + _hash.GetHashAndReset(hash); + var result = From(hash); +#else + _hash.TransformFinalBlock(inputBuffer: Array.Empty(), inputOffset: 0, inputCount: 0); + var result = From(_hash.Hash); +#endif + + s_incrementalHashPool.Return(_hash); return result; } - private static void AppendTypeKind(IncrementalHash hash, TypeKind kind) + private void AppendBuffer(int count) + { + Debug.Assert(s_buffer is not null); + +#if NET + _hash.AppendData(s_buffer, offset: 0, count); +#else + _hash.TransformBlock(s_buffer, inputOffset: 0, inputCount: count, outputBuffer: null, outputOffset: 0); +#endif + } + + private void AppendTypeKind(TypeKind kind) { var buffer = GetBuffer(); buffer[0] = (byte)kind; - hash.AppendData(buffer, offset: 0, count: 1); + AppendBuffer(count: 1); } - private static void AppendBoolValue(IncrementalHash hash, bool value) + private void AppendBoolValue(bool value) { - var buffer = GetBuffer(); - buffer[0] = (byte)(value ? 1 : 0); - hash.AppendData(buffer, offset: 0, count: sizeof(bool)); + AppendByteValue((byte)(value ? 1 : 0)); } - private static void AppendByteValue(IncrementalHash hash, byte value) + private void AppendByteValue(byte value) { var buffer = GetBuffer(); buffer[0] = value; - hash.AppendData(buffer, offset: 0, count: sizeof(byte)); + AppendBuffer(count: sizeof(byte)); } - private static void AppendCharValue(IncrementalHash hash, char value) + private void AppendCharValue(char value) { var buffer = GetBuffer(); BinaryPrimitives.WriteUInt16LittleEndian(buffer.AsSpan(0, sizeof(char)), value); - hash.AppendData(buffer, offset: 0, count: sizeof(char)); + AppendBuffer(count: sizeof(char)); } - private static void AppendInt32Value(IncrementalHash hash, int value) + private void AppendInt32Value(int value) { var buffer = GetBuffer(); BinaryPrimitives.WriteInt32LittleEndian(buffer.AsSpan(0, sizeof(int)), value); - hash.AppendData(buffer, offset: 0, count: sizeof(int)); + AppendBuffer(count: sizeof(int)); } - private static void AppendInt64Value(IncrementalHash hash, long value) + private void AppendInt64Value(long value) { var buffer = GetBuffer(); BinaryPrimitives.WriteInt64LittleEndian(buffer.AsSpan(0, sizeof(long)), value); - hash.AppendData(buffer, offset: 0, count: sizeof(long)); + AppendBuffer(count: sizeof(long)); } - private static void AppendStringValue(IncrementalHash hash, string value) + private void AppendStringValue(string value) { - var stringBytes = MemoryMarshal.AsBytes(value.AsSpan()); - Debug.Assert(stringBytes.Length == value.Length * 2); +#if NET + _hash.AppendData(MemoryMarshal.AsBytes(value.AsSpan())); + _hash.AppendData(MemoryMarshal.AsBytes("\0".AsSpan())); +#else + using var _ = ArrayPool.Shared.GetPooledArray(4 * 1024, out var buffer); - var buffer = ArrayPool.Shared.Rent(4 * 1024); - try + AppendData(_hash, buffer, value); + AppendData(_hash, buffer, "\0"); + + static void AppendData(SHA256 hash, byte[] buffer, string value) { + var stringBytes = MemoryMarshal.AsBytes(value.AsSpan()); + Debug.Assert(stringBytes.Length == value.Length * 2); + var index = 0; while (index < stringBytes.Length) { - var remaining = stringBytes.Length - index; + var remaining = stringBytes.Length; var toCopy = Math.Min(remaining, buffer.Length); + stringBytes.Slice(index, toCopy).CopyTo(buffer); - hash.AppendData(buffer, 0, toCopy); + hash.TransformBlock(buffer, inputOffset: 0, inputCount: toCopy, outputBuffer: null, outputOffset: 0); index += toCopy; } } - finally - { - ArrayPool.Shared.Return(buffer); - } +#endif } - - private static void AppendHashDataValue(IncrementalHash hash, HashData value) + private void AppendHashDataValue(HashData value) { - AppendInt64Value(hash, value.Data1); - AppendInt64Value(hash, value.Data2); - AppendInt64Value(hash, value.Data3); - AppendInt64Value(hash, value.Data4); + AppendInt64Value(value.Data1); + AppendInt64Value(value.Data2); + AppendInt64Value(value.Data3); + AppendInt64Value(value.Data4); } public void AppendNull() { - AppendTypeKind(_hash, TypeKind.Null); + AppendTypeKind(TypeKind.Null); } public void AppendData(bool value) { - AppendTypeKind(_hash, TypeKind.Bool); - AppendBoolValue(_hash, value); + AppendTypeKind(TypeKind.Bool); + AppendBoolValue(value); } public void AppendData(byte value) { - AppendTypeKind(_hash, TypeKind.Byte); - AppendByteValue(_hash, value); + AppendTypeKind(TypeKind.Byte); + AppendByteValue(value); } public void AppendData(char value) { - AppendTypeKind(_hash, TypeKind.Char); - AppendCharValue(_hash, value); + AppendTypeKind(TypeKind.Char); + AppendCharValue(value); } public void AppendData(int value) { - AppendTypeKind(_hash, TypeKind.Int32); - AppendInt32Value(_hash, value); + AppendTypeKind(TypeKind.Int32); + AppendInt32Value(value); } public void AppendData(long value) { - AppendTypeKind(_hash, TypeKind.Int64); - AppendInt64Value(_hash, value); + AppendTypeKind(TypeKind.Int64); + AppendInt64Value(value); } public void AppendData(string? value) @@ -166,14 +213,14 @@ public void AppendData(string? value) return; } - AppendTypeKind(_hash, TypeKind.String); - AppendStringValue(_hash, value); + AppendTypeKind(TypeKind.String); + AppendStringValue(value); } public void AppendData(Checksum value) { - AppendTypeKind(_hash, TypeKind.Checksum); - AppendHashDataValue(_hash, value.Data); + AppendTypeKind(TypeKind.Checksum); + AppendHashDataValue(value.Data); } public void AppendData(object? value) diff --git a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.cs b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.cs index 9490efc5ee4..ef7c03dd0ec 100644 --- a/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.cs +++ b/src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/Utilities/Checksum.cs @@ -11,7 +11,8 @@ namespace Microsoft.AspNetCore.Razor.Utilities; internal sealed partial record Checksum { - private const int HashSize = 32; + // Size of SHA-256 + private const int HashSize = 256 / 8; public static readonly Checksum Null = new(default(HashData));