Why is HMACSHA256 ComputeHash not thread safe

Question

Since the class is Disposable I somehow thought it should probably be used as a singleton. How should I use it safely?

        // Code uses Nuget package FluentAssertions
        var key = "supersecret";
        var keybytes = Encoding.UTF8.GetBytes(key);
        var hmac = new HMACSHA256(keybytes);

        var tokenBytes = Encoding.UTF8.GetBytes("tokentocompute");
        var expected = hmac.ComputeHash(tokenBytes);

        await Task.WhenAll(
            Enumerable.Range(0, 100).Select(i => Task.Run(() =>
            {
                var hash = hmac.ComputeHash(tokenBytes);
                // This throws most of the time
                hash.ShouldBeEquivalentTo(expected, $"{i}");
            }))
        );

I did not think it was a duplicate of HMACSHA1.ComputeHash() thread-safety question since there it talks specifically about different threads setting the key, whereas I am using the same key throughout every call. After rereading it, it may yet be a duplicate. Will wait upon the opinion of you guys.

Possible duplicate of [HMACSHA1.ComputeHash() thread-safety question](https://stackoverflow.com/questions/569683/hmacsha1-computehash-thread-safety-question) — mjwills, Dec 15 '17 at 12:28
"Since the class is Disposable I somehow thought it should probably be used as a singleton" - eh? I don't know where you picked up this "pattern". Most disposables should be disposed of as soon as its naturally possible to do - disposal allows for early release of resources, whereas singletons unnaturally extend lifetimes. — Damien_The_Unbeliever, Dec 15 '17 at 12:35
Yeah definitely, was using too many long living singletons lately, like stackexchange redis connection multiplexer. Need to take that as a reminder that those aren't the norm. — Alex AIT, Dec 15 '17 at 12:50

score 9 · Accepted Answer · answered Dec 15 '17 at 12:19

From MSDN:

Any public static ( Shared in Visual Basic) members of this type are thread safe. Any instance members are not guaranteed to be thread safe.

Even though this paragraph is present on what feels like every class in MSDN, you need to take that to heart.

Looking at the decompiled code, it seems it uses a couple of private variables here and there. Since it has no locks, errors can occur pretty quickly.

[HashAlgorithm.cs]
/// <summary>Represents the size, in bits, of the computed hash code.</summary>
protected int HashSizeValue;
/// <summary>Represents the value of the computed hash code.</summary>
protected internal byte[] HashValue;
/// <summary>Represents the state of the hash computation.</summary>
protected int State;

[...]

[HashAlgorithm.cs]
public byte[] ComputeHash(byte[] buffer)
{
  if (this.m_bDisposed)
    throw new ObjectDisposedException((string) null);
  if (buffer == null)
    throw new ArgumentNullException(nameof (buffer));
  this.HashCore(buffer, 0, buffer.Length);
  this.HashValue = this.HashFinal();
  byte[] numArray = (byte[]) this.HashValue.Clone();
  this.Initialize();
  return numArray;
}

We ended up putting a using-block in our code which recreated the hmac instance every time. Performance was similar to putting a global lock around it in our cursory tests. We wanted to avoid overengineering something with e.g. threadstatic since performance was pretty good.

    await Task.WhenAll(
        Enumerable.Range(0, 100).Select(i => Task.Run(() =>
        {
            byte[] hash;
            using (var hma = new HMACSHA256(keybytes))
            {
                hash = hma.ComputeHash(tokenBytes);
            }
            //lock (this)
            //{
            //    hash = hmac.ComputeHash(tokenBytes); 
            //}       

            // Both ways achieved the desired results and performance was similar         
            hash.ShouldBeEquivalentTo(expected, $"{i}");
        }))
    );

Why is HMACSHA256 ComputeHash not thread safe

1 Answers1