The Algorithm_Factory has this logic on looking for an object:

 - Check the cache; if found, return value
 - Populate cache, if the value is already there, delete the old
   object and save the new one.
 - Recheck the cache value

Raja <[email protected]> pointed out on the list that this could race
if multiple threads called a lookup function in close succession while
the cache was cold. All of them would fail the lookup, then each of
them would add it, but the values returned would be deleted by other
threads.

Instead, declare that first write wins. Then, the cache stays
consistent even if there is a race, the only issue is an extra search
and delete.

Modify GOST and Skein, as their name() function did not roundtrip
properly which caused failures otherwise.

5 files changed, 36 insertions, 8 deletions

diff --git a/doc/log.txt b/doc/log.txt
index 3deb98e12..ed8a08f21 100644
--- a/doc/log.txt
+++ b/doc/log.txt
@@ -10,6 +10,11 @@ Series 1.10
 Version 1.10.1, Not Yet Released
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+* A race condition in `Algorithm_Factory` could cause crashes in
+  multithreaded code. See `this thread on botan-devel
+  <http://lists.randombit.net/pipermail/botan-devel/2011-July/001455.html>`_
+  for details and workarounds.
+
 Version 1.10.0, 2011-06-20
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/src/algo_factory/algo_cache.h b/src/algo_factory/algo_cache.h
index 0d891ad3f..25f2db023 100644
--- a/src/algo_factory/algo_cache.h
+++ b/src/algo_factory/algo_cache.h
@@ -1,6 +1,6 @@
 /*
 * An algorithm cache (used by Algorithm_Factory)
-* (C) 2008-2009 Jack Lloyd
+* (C) 2008-2009,2011 Jack Lloyd
 *
 * Distributed under the terms of the Botan license
 */
@@ -171,14 +171,16 @@ void Algorithm_Cache<T>::add(T* algo,
 
    Mutex_Holder lock(mutex);
 
-   delete algorithms[algo->name()][provider];
-   algorithms[algo->name()][provider] = algo;
-
    if(algo->name() != requested_name &&
       aliases.find(requested_name) == aliases.end())
       {
       aliases[requested_name] = algo->name();
       }
+
+   if(!algorithms[algo->name()][provider])
+      algorithms[algo->name()][provider] = algo;
+   else
+      delete algo;
    }
 
 /*
diff --git a/src/block/gost_28147/gost_28147.cpp b/src/block/gost_28147/gost_28147.cpp
index 07f3359cd..d4a9faa40 100644
--- a/src/block/gost_28147/gost_28147.cpp
+++ b/src/block/gost_28147/gost_28147.cpp
@@ -1,6 +1,6 @@
 /*
 * GOST 28147-89
-* (C) 1999-2009 Jack Lloyd
+* (C) 1999-2009,2011 Jack Lloyd
 *
 * Distributed under the terms of the Botan license
 */
@@ -58,12 +58,31 @@ GOST_28147_89::GOST_28147_89(const GOST_28147_89_Params& param) :
    for(size_t i = 0; i != 4; ++i)
       for(size_t j = 0; j != 256; ++j)
          {
-         u32bit T = (param.sbox_entry(2*i  , j % 16)) |
-                    (param.sbox_entry(2*i+1, j / 16) << 4);
+         const u32bit T = (param.sbox_entry(2*i  , j % 16)) |
+                          (param.sbox_entry(2*i+1, j / 16) << 4);
          SBOX[256*i+j] = rotate_left(T, (11+8*i) % 32);
          }
    }
 
+std::string GOST_28147_89::name() const
+   {
+   /*
+   'Guess' the right name for the sbox on the basis of the values.
+   This would need to be updated if support for other sbox parameters
+   is added. Preferably, we would just store the string value in the
+   constructor, but can't break binary compat.
+   */
+   std::string sbox_name = "";
+   if(SBOX[0] == 0x00072000)
+      sbox_name = "R3411_94_TestParam";
+   else if(SBOX[0] == 0x0002D000)
+      sbox_name = "R3411_CryptoPro";
+   else
+      throw Internal_Error("GOST-28147 unrecognized sbox value");
+
+   return "GOST-28147-89(" + sbox_name + ")";
+   }
+
 /*
 * Two rounds of GOST
 */
diff --git a/src/block/gost_28147/gost_28147.h b/src/block/gost_28147/gost_28147.h
index 75ba74c44..bc26da774 100644
--- a/src/block/gost_28147/gost_28147.h
+++ b/src/block/gost_28147/gost_28147.h
@@ -57,7 +57,7 @@ class BOTAN_DLL GOST_28147_89 : public Block_Cipher_Fixed_Params<8, 32>
 
       void clear() { zeroise(EK); }
 
-      std::string name() const { return "GOST-28147-89"; }
+      std::string name() const;
       BlockCipher* clone() const { return new GOST_28147_89(SBOX); }
 
       /**
diff --git a/src/hash/skein/skein_512.cpp b/src/hash/skein/skein_512.cpp
index f85968e84..571bf9c0b 100644
--- a/src/hash/skein/skein_512.cpp
+++ b/src/hash/skein/skein_512.cpp
@@ -184,6 +184,8 @@ Skein_512::Skein_512(size_t arg_output_bits,
 
 std::string Skein_512::name() const
    {
+   if(personalization != "")
+      return "Skein-512(" + to_string(output_bits) + "," + personalization + ")";
    return "Skein-512(" + to_string(output_bits) + ")";
    }