Why does SHA1.ComputeHash fail under high load with many threads?

Question

I'm seeing an issue with some code I maintain. The code below has a private static SHA1 member (which is an IDisposable but since it's static, it should never get finalized). However, under stress this code throws an exception that suggests it has been closed:

Caught exception.  Safe handle has been closed" 
Stack trace: Call stack where exception was thrown
at System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean& success)
at System.Security.Cryptography.Utils.HashData(SafeHashHandle hHash, Byte[] data, Int32 cbData, Int32 ibStart, Int32 cbSize)
at System.Security.Cryptography.Utils.HashData(SafeHashHandle hHash, Byte[] data, Int32 ibStart, Int32 cbSize)
at System.Security.Cryptography.HashAlgorithm.ComputeHash(Byte[] buffer)

The code in question is:

internal class TokenCache
{
    private static SHA1 _sha1 = SHA1.Create();

    private string ComputeHash(string password)
    {
        byte[] passwordBytes = UTF8Encoding.UTF8.GetBytes(password);
        return UTF8Encoding.UTF8.GetString(_sha1.ComputeHash(passwordBytes));
    }

My question is obviously what could cause this issue. Can the call to SHA1.Create fail silently (how many cryptographic resources are available)? Could this be caused by the appdomain going down?

Any other theories?

Bowrah

What does this have to do with Dispose? Also, which "SHA1" class is that? – Quadragesimal

Are you sure the class SHA1 is threadsafe? Are you able to grab the password being hashed when it fails? – Building

@John Saunders, sorry you're correct. This has nothing to do with Dispose. I thought the finalizer on the System.Security.Cryptography.SHA1CryptoServiceProvider might have been triggered somehow. msdn.microsoft.com/en-us/library/e7hyyd4e(v=vs.110).aspx – Bowrah

@Rob, The MSDN help page does not say anything about thread safety. However, the WCF services this is used in all use instancecontextmode == perCall and ConcurrencyMode == Single – Bowrah

@user18044 I just ran a stress test locally (10,000) random hashes. It worked fine. Changing it to being parallel caused the exact same error you received (Safe handle has been closed). I'm very sure it is because somewhere, your application is threaded. – Building

@user18044 The following reproduces your error: var strings = Enumerable.Range(1,10000).Select(r => Guid.NewGuid().ToString()).ToList(); Parallel.ForEach(strings, s => { ComputeHash(s).Dump(); }); using foreach, however, does not – Building

@Rob, thanks, that is very interesting! I'm still confused about why the handle would be closed though. There should never be more than one instance of that class. I'll start looking for threading in our application. – Bowrah

Answer 1

As per the documentation for the HashAlgorithm base class

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

You should not share these classes between threads where different threads try and call ComputeHash on the same instance at the same time.

EDIT This is what is causing your error. The stress test below yields a variety of errors due to multiple threads calling ComputeHash on the same hash algorithm instance. Your error is one of them.

Specifically, I have seen the following errors with this stress test:

• System.Security.Cryptography.CryptographicException: Hash not valid for use in specified state.
• System.ObjectDisposedException: Safe handle has been closed

Stress test code sample:

const int threadCount = 2;
var sha1 = SHA1.Create();
var b = new Barrier(threadCount);
Action start = () => {
                    b.SignalAndWait();
                    for (int i = 0; i < 10000; i++)
                    {
                        var pwd = Guid.NewGuid().ToString();
                        var bytes = Encoding.UTF8.GetBytes(pwd);
                        sha1.ComputeHash(bytes);
                    }
                };
var threads = Enumerable.Range(0, threadCount)
                        .Select(_ => new ThreadStart(start))
                        .Select(x => new Thread(x))
                        .ToList();
foreach (var t in threads) t.Start();
foreach (var t in threads) t.Join();

Zama

Remarkable mishap, the safe handle looks fine. But undeniable, good answer. – Vinegarroon

@HansPassant. Thank you. I would guess it may be the SHA1CryptoServiceProvider.Initialize method, which appears to do a non-thread safe Dispose and then re-create on the _safeHashHandle field. – Zama

Hehe, bizarro, Initialize() is called after computing the hash. Must be some kind of safety thing. You got it. – Vinegarroon

@HansPassant. Yes, it would be better called Reset. On further inspection MD5CryptoServiceProvider does exactly the same, and the others have other variants of internal state which will have similar effects. Take home is definitely that instance methods on any HashAlgorithm subclass are not thread-safe. – Zama

i can't actually see the bit on the doco that highlights this issue.. i searched on the page for 'thread' and 'safe' ?? – Grison

@Grison since I wrote this answer, many years ago, MS documentation has moved to a new format, new versions of the framework have been released so ... ymmv on this answer I guess. – Zama

fair point.. I'll check the date next time 🙂 the exceptions discussed still occur 👍 – Grison