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?

What does this have to do with Dispose? Also, which "SHA1" class is that? — John Saunders, Oct 27 '14 at 16:51
Are you sure the class SHA1 is threadsafe? Are you able to grab the password being hashed when it fails? — Rob, Oct 27 '14 at 16:51
@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. http://msdn.microsoft.com/en-us/library/e7hyyd4e(v=vs.110).aspx — MvdD, Oct 27 '14 at 16:53
@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 — MvdD, Oct 27 '14 at 16:59
@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. — Rob, Oct 27 '14 at 17:04
@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 — Rob, Oct 27 '14 at 17:05
@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. — MvdD, Oct 27 '14 at 17:18

Andy Brown · Accepted Answer · 2014-10-27T17:21:17.567

36

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();

edited Oct 27 '14 at 17:21

answered Oct 27 '14 at 17:01

Andy Brown

18,961
3
52
62

Remarkable mishap, the safe handle looks fine. But undeniable, good answer. – Hans Passant Oct 27 '14 at 17:25
1

@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. – Andy Brown Oct 27 '14 at 17:33
Hehe, bizarro, Initialize() is called *after* computing the hash. Must be some kind of safety thing. You got it. – Hans Passant Oct 27 '14 at 17:42
1

@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. – Andy Brown Oct 27 '14 at 18:03
i can't actually see the bit on the doco that highlights this issue.. i searched on the page for 'thread' and 'safe' ?? – m1nkeh Feb 18 '19 at 16:06
@m1nkeh 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. – Andy Brown Feb 18 '19 at 20:08
1

fair point.. I'll check the date next time the exceptions discussed still occur – m1nkeh Feb 18 '19 at 23:08

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

1 Answers1

Linked

Related