Analyzer / fixer proposal: Prefer static HashData methods over ComputeHash

[GrabYourPitchforks]

Category: performance, improved linker trimming efficiency (is that even a category?)

We should consider an analyzer that can detect this pattern:

byte[] buffer = GetSomeBuffer();
using (var sha256 = SHA256.Create())
{
    byte[] digest = sha256.ComputeHash(buffer);
    /* use 'digest' here */
}

And suggest the code instead use this pattern:

byte[] buffer = GetSomeBuffer();
byte[] digest = SHA256.HashData(buffer);
/* use 'digest' here */

Using one-shot hashing APIs like this is a bit more foolproof than using the normal stateful instance members on these types.

The analyzer should detect the pattern where a HashAlgorithm instance is created (either via SHA256.Create, new SHA256Managed, or new SHA256CryptoServiceProvider; or via the MD5 / SHA* equivalents), there is a single call made to HashAlgorithm.ComputeHash(byte[]), then there is an optional call made to Dispose.

The analyzer should only trigger for projects targeting net5.0+, as that's when the new static APIs were introduced.

See also: #17590, dotnet/aspnetcore#24696, dotnet/wpf#3318

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
See info in area-owners.md if you want to be subscribed.

[terrajobst]

Video

This makes sense.

[wzchua]

Which severity should this be?

[GrabYourPitchforks]

Probably suggestion / informational / etc.? We tend to keep perf-focused analyzers low severity unless the caller is doing something egregiously wrong.

[wzchua]

I see.
I would like to be assigned this issue.

[Ponant]

Is the bottom line that the using can be skipped by using the static HashData?

[bartonjs]

Is the bottom line that the using can be skipped by using the static HashData?

• The using
• The object creation
• The pooling that people often write to avoid the object creation
• The questions about thread safety that some of the people who write pooling often have

[stephentoub]

What Jeremy said... it's simpler and faster.

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
WithInstance	1,185.3 ns	10.21 ns	9.55 ns	1.00	0.0381	-	-	240 B
WithStatic	972.3 ns	12.58 ns	11.15 ns	0.82	0.0076	-	-	56 B

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Security.Cryptography;

[MemoryDiagnoser]
public class Program
{
    public static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private readonly byte[] _data = RandomNumberGenerator.GetBytes(128);

    [Benchmark(Baseline = true)]
    public byte[] WithInstance()
    {
        using (var sha256 = SHA256.Create())
        {
            return sha256.ComputeHash(_data);
        }
    }

    [Benchmark]
    public byte[] WithStatic()
    {
        return SHA256.HashData(_data);
    }
}

[Ponant]

Well that is pretty great guys. I came accross this in a pretty random way. I saw methods using using ReadOnlySpan, should we favor them, too?
Also, how disposal work now, I guess in the background but how?

[GrabYourPitchforks]

I saw methods using using ReadOnlySpan, should we favor them, too?

It depends on your needs. If you already have a destination span you want the results written to, call the overload that takes a destination span. Otherwise call the overload that creates the result array on your behalf.

Also, how disposal work now, I guess in the background but how?

Depends on the OS. For example, we may keep a shared handle around and keep reusing it. Or we may call a specialty OS API under the covers. This is just an implementation detail and callers shouldn't need to worry about it.

[Ponant]

That is great thank you. I think the docs should explain this somehow, I was updating a library I had from .Net Standard and somehow came to this thread by chance. The compute hash method is not labelled as obsolete.

[Ponant]

@stephentoub , the allocation ratio is huge! (240/56)

[stephentoub]

That's because it's only allocating the resulting byte[] now rather than also allocating the SHA256 instance and associated state.

[GrabYourPitchforks]

I think the docs should explain this somehow, ... The compute hash method is not labelled as obsolete.

The docs should explain what, specifically? The existing ComputeHash instance method is not obsolete, nor are there current plans to make it obsolete.

[Ponant]

Explain the difference in usage, and also provide examples as done above. If not to be obsoleted, then explain in what scenario the old method would be better, e.g. for projects targeting .Net5+ the static version seems better.
E.g. in these docs the using is used https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.sha256?view=net-5.0.

[Ponant]

Does the skipping of the using statement also applies to RandomNumberGenerator?
Example

 var random= RandomNumberGenerator.GetBytes(16);
 var encode = WebEncoders.Base64UrlEncode(random);

Or should we use

using (var rng = RandomNumberGenerator.Create())
{
...
}

?

[vcsjones]

Does the skipping of the using statement also applies to RandomNumberGenerator?

You should use the static members when possible on RandomNumberGenerator. (and in the case of GetInt32, it is only available as a static member). The benefits are similar, there is no object to allocate and no need to reason about the lifetime of the object.

[vcsjones]

Speaking more broadly, there has been a lot of "staticification" of the crypto stuff in .NET:

1. RandomNumberGenerator had static members added back in the .NET Core 2 and 3 timeframe. In .NET 6, GetBytes(int) was added as a convenience of doing new byte[n] and calling Fill on it.

Similarly, GetInt32 was added as a static-only member, out of a desire to use statics going forward.

2. .NET 5 added static HashData to the hash APIs (which is the topic of this issue)

3. .NET 6 added static Pbkdf2 to Rfc2898DerivedBytes.

4. .NET 6 added static HashData to the HMAC APIs (eg HMACSHA256.HashData).

[Ponant]

Thank you for the detailed info @vcsjones and what you have done sounds great and surely welcomed.

[danmoseley]

@jeffhandley should this be in 6.0 at this point?