Problem with SHA256Managed when running multi-threaded

  • Thread starter Thread starter Tristan MSDN Keen
  • Start date Start date
T

Tristan MSDN Keen

Hi,

I am experiencing problems with the SHA256Managed class when multiple
threads are simultaneously using it. I have reduced the problem to a small
piece of code (see below). This creates two threads, each of which repeatedly
attempts to hash a string and compare the result to a previous attempt. After
a number of tries, the result coming back is different. Running a single
thread or locking the hashAlgo object stops the problem. However, this
shouldn't be necessary as the documentation states that this class is thread
safe when using a static instance.


--- Code example:

using System;
using System.Security.Cryptography;
using System.Text;

namespace HashTester
{
class Program
{
static void Main(string[] args)
{
for (int i = 0; i < 2; i++)
{
System.Threading.Thread ff = new System.Threading.Thread(new
System.Threading.ParameterizedThreadStart(HashTestFn));
ff.Start("pa$$word");
}
}

private static void HashTestFn(object o)
{
HashTest test = new HashTest(o.ToString());

for (int i = 0; i < 10000; i++)
{
test.Test();
}
}
}

class HashTest
{
private static HashAlgorithm hashAlgo = new
System.Security.Cryptography.SHA256Managed();

private string input;
private string output;

public HashTest(string toHash)
{
input = toHash;
byte[] hashBytes;
hashBytes = hashAlgo.ComputeHash(Encoding.ASCII.GetBytes(input));

output = Convert.ToBase64String(hashBytes).ToUpper();
}

public void Test()
{
byte[] hashBytes =
hashAlgo.ComputeHash(Encoding.ASCII.GetBytes(input));
string newOut = Convert.ToBase64String(hashBytes).ToUpper();

if (newOut != output)
{
throw new Exception("Bad Hash!!!!");
}
}
}
}
 
Tristan MSDN Keen said:
I am experiencing problems with the SHA256Managed class when multiple
threads are simultaneously using it. I have reduced the problem to a small
piece of code (see below). This creates two threads, each of which repeatedly
attempts to hash a string and compare the result to a previous attempt. After
a number of tries, the result coming back is different. Running a single
thread or locking the hashAlgo object stops the problem. However, this
shouldn't be necessary as the documentation states that this class is thread
safe when using a static instance.
Click to expand...

No it doesn't. It says:

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

It's got nothing to do with whether your *variable* is static or not -
it's to do with whether you're calling static *members* on
SHA256Managed. You're not - you're calling ComputeHash, which is an
instance member.
 
Jon is right.
Furthermore, "Any public static (Shared in Visual Basic) members of this
type are
thread safe. Any instance members are not guaranteed to be thread safe" is
applied to almost all clr classes by MS in the documentation. I dont
remember every coming across with any exception.

When developing multithreaded applications(and actually all asp.net web apps
are), one has to be careful about this.

However, one special class group is those dealing with encoding, e.g.
UTF8Encoding, while the above statement is also applied to it, howver, by
checking the source code for asp.net by MS itself, it shows lots of usage of
the common instance UTF8Encoding.UTF8 in multithread environment, this lead
me to think the instance members for some classes are acutally thread-safe,
or MS itslef is producing buggy code.
 
JX said:
Furthermore, "Any public static (Shared in Visual Basic) members of this
type are thread safe. Any instance members are not guaranteed to be
thread safe" is applied to almost all clr classes by MS in the
documentation. I dont remember every coming across with any exception.
Click to expand...
There are multiple: all synchronization primitives, by obvious necessity
(ManualResetEvent, AutoResetEvent, etc.), Thread (equally obvious), all
immutable types by construction, TraceListener and derivatives, Socket.
Those are the most notable I can think of, I'm probably forgetting some
specialized types.

In the majority of cases, a type can't provide useful synchronization itself
(if clients need to perform multiple dependent operations on the object,
they'll need to synchronize anyway, for example) and synchronization isn't
free, so it makes sense to leave it to the client. Other types may actually
be thread-safe by construction, but this isn't mentioned because Microsoft
doesn't want to commit itself to keeping it that way. And in other cases,
they just plain forgot to change the docs.
However, one special class group is those dealing with encoding, e.g.
UTF8Encoding, while the above statement is also applied to it, howver, by
checking the source code for asp.net by MS itself, it shows lots of usage of
the common instance UTF8Encoding.UTF8 in multithread environment, this lead
me to think the instance members for some classes are acutally thread-safe,
or MS itslef is producing buggy code.
Click to expand...
This seems to be one of those oversights. It's hard to imagine people not
relying on the fact that UTF8Encoding is thread-safe and will remain that
way -- it's used *everywhere*, and they didn't provide a static instance for
nothing (if you had to lock that, it would defeat the purpose).

And yes, it *is* thread-safe, even if the docs don't say so. UTF8Encoding
has no mutable state, and in general no Encoding does. Only the Decoder and
Encoder instances you get from .GetDecoder()/.GetEncoder() do, and those
aren't thread-safe (but not generally shared across threads either).
 
Agreed. By the time I wrote " I dont remember every coming across with any
exception", I really hadn't given much thought to it. But anyway, that is
the most common assertion I met.

From MS point of view, it's a lot of effort to keep things thread-safe and
the docs synchronized. However from programmer user's point of view, we
have to make sure things are as certain as possible or we take the risk of
introducing bugs difficult to find and fix. Since the docs leaves it that
way, it takes more effort for the devs to achieve this certainty.
 
Back
Top