Random number generator returning zeros

Question

I have an ASP.NET application that relies on the Random class to generate a pseudo-random string. It uses the following code (this is part of a larger piece of sample code provided by Google for apps SSO):

    public static class SamlUtility
{
    private static Random random = new Random();

    private static char[] charMapping =  { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p' };

    public static string CreateId()
    {
        byte[] bytes = new byte[20]; // 160 bits

        random.NextBytes(bytes);

        char[] chars = new char[40];

        for (int i = 0; i < bytes.Length; i++)
        {
            int left = (bytes[i] >> 4) & 0x0f;
            int right = bytes[i] & 0x0f;
            chars[i * 2] = charMapping[left];
            chars[i * 2 + 1] = charMapping[right];
        }

        return new string(chars);
    }
}

This normally works very well, but from time to time it starts generating a string of 'a'. From what I can tell from debugging, Random simply stops returning random numbers and instead fills bytes with the same value over and over. I've patched this by using a GUID instead, but I'm curious what happened in the original code. I'm assuming some form of entropy exhaustion, but I can't find any reference in the docs. Also, each time this has happened, performing iisreset restored the correct behavior.

Any suggestions regarding what was going wrong would be much appreciated.

Answer 1

The Random class is not thread-safe.
If you generate random numbers on the same instance on multiple threads at once, its internal state will be corrupted and it will start returning zeroes.

You need to make the Random instance [ThreadStatic] to ensure that each instance is not shared by multiple threads.
Note that initializers for [ThreadStatic] fields will only run once, so you need to check whether it's null every time you use the field and initialize it if necessary.
It would also be a good idea to include both the thread ID and the current time in the seed to prevent seed collisions.

Note, by the way, that the Random class is not secure; consider using the RNGCryptoServiceProvider class