As noted by other commenters, it's not threadsafe in that two different threads might initialize the class at the same time. However, I don't see that anyone bothered to ask "So what?"

If two threads initialize it at once, they'll each create an instance. One will live for the life of the AppDomain. The other will get used briefly, then thrown away. It's a little messy, but will it cause any actual problems?

It's not holding any important state that must not be lost, and it's not holding on to any resources that might be leaked. The worst case is just that you initialize two RNGs instead of one, so you might get slightly different random numbers than you would otherwise get. But they're supposed to be crypto-secure numbers, not "repeatable random" like you might want for a saved game seed, so that seems like it should be acceptable. Unless there's some deep crypto reason that such a thing would be bad?

The two things that might actually matter are that the GeneratedBytesCount might be wrong, and if someone registered an event handler for the GenerateRandom256Pre event it might get lost. Neither of those things are actually used anywhere in the code, but it is possible that client code could use them.

Bottom line I'd say it's not great, but in the code that they actually have it will not cause any problems.

Aside: technically the class is not marked as threadsafe, but since it's a singleton it should probably be threadsafe anyway.

Any takers on that question?

Well, it's not thread safe but they might not think that's an issue. It looks like this:

		private static CryptoRandom m_pInstance = null;
		public static CryptoRandom Instance
		{
			get
			{
				if(m_pInstance != null) return m_pInstance;

				m_pInstance = new CryptoRandom();
				return m_pInstance;
			}
		}

Philbarr is correct. The pattern they are using is fundamentally supposed to provide a thread-safe Singleton, and it fails to do that. Is that a security problem in this specific context? No.

But it's a "No" because the authors are lucky in this case - not because they are competent.

Now, that's just one instance of poor skill. There are many more. Are you sure none of them have security implications?

That's quite harsh. I guess you verified that there are actually different threads able to access this code before making such a statement? For instance, if I make a single-threaded application in the first place then I don't care about thread-safety at all. Because I am not going to need it.

The odds of an singleton being called for the first time simultaneously is abysmal. Usually you even call a singleton first in your own initialization code making errors impossible.

I do agree that technically you are correct and you should wear belt and suspenders, especially if it's a library for third-party consumption and labeled as thread-safe... but still... its pretty esoteric, and only used by the author (I assume) who knows its not thread-safe. Locking isn't exactly without its performance implications either and even though that is neglible it feels unecessary if a race condition is de facto near impossible.

Later on in the class I notice they have this:

		public ulong GeneratedBytesCount
		{
			get
			{
				ulong u;
				lock(m_oSyncRoot) { u = m_uGeneratedBytesCount; }
				return u;
			}
		}

...so if you care about thread safety at some point in your class, then you should care about it during it's initialisation.

I have verified that the CryptoRandom class is part of a standalone library with (1) should be thread-safe since it cannot dictate how it will be used by callers; (2) the authors clearly intended this library to be thread-safe (based on "thread-safe" comments in its source code).

And in all likelihood it is thread-safe - but that's due to being lucky - not competent.

The larger issue is that we have a widely-used crypto software which is clearly (1) not designed well; (2) not implemented well.

How much trust one is willing to place into current and future versions by the same author(s) is up to you.

Well, "(1) should be thread-safe since it cannot dictate how it will be used by callers" doesn't hold. For instance most of .NET classes are not thread-safe because thread-safety has a cost. So it's a question of documentation (it's well documented in .NET).

But "(2) the authors clearly intended this library to be thread-safe" means that piece of code is bad. So you have a point here.

To be a little more specific, the class itself is not marked as thread-safe. Only the AddEntropy() and GetRandomBytes() methods are so marked. So it would technically be permissible for any other part of the class to not be thread-safe.

But since it's a singleton, having a non-thread-safe initializer is cutting that distinction a little fine.

Given the discussion expressed in this chain and the fact that this is a reasonably small open source project. How many times could a solution have been submitted to the project for this and other noticed issues in the time it was discussed?

I'm sure the author would be very happy to see a sudden influx of contributions to the project, and we'd all have a better product in the end too.

Seems odd the spirit of open source in this respect tends to be more about pointing out the failures of the author than to collectively improve the actual product.

That's a fair point. If this was on Github I might take that to heart and submit a PR. Since it's hosted on Sourceforge, which I don't have an account on and don't want to given their recent behavior, I'm not going to.

That should be a perfectly usable Singleton, if you just stay on one thread? Is the application using multiple threads? Should it be?

Most UI applications sooner or later need at least some basic background processing but if I was writing a simple password manager, I'd most likely just do everything blocking on the UI thread.

That said: for simple patterns like singleton, there is really no reason not to use the builtin and recommended way which is the Lazy<T>.

Here's Jon Skeet's writeup of why this is bad and what you should be doing:

http://csharpindepth.com/Articles/General/Singleton.aspx

Note that using the last example isn't necessarily "the best", it really depends on your requirements.

Nonetheless, a very interesting read.

I assume that they are implying that the code is not constant time. In this snippet, the code bails as soon as a deviation is detected. This can, in theory, allow an attacker to determine the desired value by measuring the time taken to reject incorrect options. I haven't reviewed the code to see if this is actually a problem, but that's my guess for why it was highlighted.

I took it to mean they should use the Linq `x.SequenceEqual(y)` instead (assuming .NET 3.5+) (and x isn't null...).

Wrong.