Remove the recursive mutex from the registry.

This is fairly straightforward -- the only change is that the functions get_new_key_allowed and get_key_factory are internal anyhow, and hence should *not* be guarded by the mutex.

Also, there's a pre-existing clang-tidy warning because of all the global variables which are illegal. We should make the registry a class and properly "static-initialize" it.

PiperOrigin-RevId: 214780540
GitOrigin-RevId: ae98b3f64affd4ca770fb31fa2c1c4d46086228c

1 files changed, 40 insertions, 33 deletions

diff --git a/cc/registry.h b/cc/registry.h
index 14a689f39..626731f5e 100644
--- a/cc/registry.h
+++ b/cc/registry.h
@@ -21,6 +21,8 @@
 #include <typeinfo>
 #include <unordered_map>
 
+#include "absl/base/thread_annotations.h"
+#include "absl/synchronization/mutex.h"
 #include "tink/catalogue.h"
 #include "tink/key_manager.h"
 #include "tink/keyset_handle.h"
@@ -62,7 +64,7 @@ class Registry {
   // see https://goo.gl/x0ymDz)
   template <class P>
   static crypto::tink::util::StatusOr<const Catalogue<P>*> get_catalogue(
-      const std::string& catalogue_name);
+      const std::string& catalogue_name) LOCKS_EXCLUDED(maps_mutex_);
 
   // Adds the given 'catalogue' under the specified 'catalogue_name',
   // to enable custom configuration of key types and key managers.
@@ -74,17 +76,20 @@ class Registry {
   // Takes ownership of 'catalogue', which must be non-nullptr
   // (in case of failure, 'catalogue' is deleted).
   template <class P>
-  static crypto::tink::util::Status AddCatalogue(
-      const std::string& catalogue_name, Catalogue<P>* catalogue);
+  static crypto::tink::util::Status AddCatalogue(const std::string& catalogue_name,
+                                                 Catalogue<P>* catalogue)
+      LOCKS_EXCLUDED(maps_mutex_);
 
   // Registers the given 'manager' for the key type 'manager->get_key_type()'.
   // Takes ownership of 'manager', which must be non-nullptr.
   template <class P>
-  static crypto::tink::util::Status RegisterKeyManager(
-      KeyManager<P>* manager, bool new_key_allowed);
+  static crypto::tink::util::Status RegisterKeyManager(KeyManager<P>* manager,
+                                                       bool new_key_allowed)
+      LOCKS_EXCLUDED(maps_mutex_);
 
   template <class P>
-  static crypto::tink::util::Status RegisterKeyManager(KeyManager<P>* manager) {
+  static crypto::tink::util::Status RegisterKeyManager(KeyManager<P>* manager)
+      LOCKS_EXCLUDED(maps_mutex_) {
     return RegisterKeyManager(manager, /* new_key_allowed= */ true);
   }
 
@@ -96,21 +101,23 @@ class Registry {
   // see https://goo.gl/x0ymDz)
   template <class P>
   static crypto::tink::util::StatusOr<const KeyManager<P>*> get_key_manager(
-      const std::string& type_url);
+      const std::string& type_url) LOCKS_EXCLUDED(maps_mutex_);
 
   // Convenience method for creating a new primitive for the key given
   // in 'key_data'.  It looks up a KeyManager identified by key_data.type_url,
   // and calls manager's GetPrimitive(key_data)-method.
   template <class P>
   static crypto::tink::util::StatusOr<std::unique_ptr<P>> GetPrimitive(
-      const google::crypto::tink::KeyData& key_data);
+      const google::crypto::tink::KeyData& key_data)
+      LOCKS_EXCLUDED(maps_mutex_);
 
   // Convenience method for creating a new primitive for the key given
   // in 'key'.  It looks up a KeyManager identified by type_url,
   // and calls manager's GetPrimitive(key)-method.
   template <class P>
   static crypto::tink::util::StatusOr<std::unique_ptr<P>> GetPrimitive(
-      const std::string& type_url, const portable_proto::MessageLite& key);
+      const std::string& type_url, const portable_proto::MessageLite& key)
+      LOCKS_EXCLUDED(maps_mutex_);
 
   // Creates a set of primitives corresponding to the keys with
   // (status == ENABLED) in the keyset given in 'keyset_handle',
@@ -122,53 +129,53 @@ class Registry {
   template <class P>
   static crypto::tink::util::StatusOr<std::unique_ptr<PrimitiveSet<P>>>
   GetPrimitives(const KeysetHandle& keyset_handle,
-                const KeyManager<P>* custom_manager);
+                const KeyManager<P>* custom_manager)
+      LOCKS_EXCLUDED(maps_mutex_);
 
   // Generates a new KeyData for the specified 'key_template'.
   // It looks up a KeyManager identified by key_template.type_url,
   // and calls KeyManager::NewKeyData.
   // This method should be used solely for key management.
   static crypto::tink::util::StatusOr<
-    std::unique_ptr<google::crypto::tink::KeyData>>
-  NewKeyData(const google::crypto::tink::KeyTemplate& key_template);
+      std::unique_ptr<google::crypto::tink::KeyData>>
+  NewKeyData(const google::crypto::tink::KeyTemplate& key_template)
+      LOCKS_EXCLUDED(maps_mutex_);
 
   // Convenience method for extracting the public key data from the
   // private key given in serialized_private_key.
   // It looks up a KeyManager identified by type_url, whose KeyFactory must be
   // a PrivateKeyFactory, and calls PrivateKeyFactory::GetPublicKeyData.
   static crypto::tink::util::StatusOr<
-    std::unique_ptr<google::crypto::tink::KeyData>>
-  GetPublicKeyData(const std::string& type_url,
-                   const std::string& serialized_private_key);
+      std::unique_ptr<google::crypto::tink::KeyData>>
+  GetPublicKeyData(const std::string& type_url, const std::string& serialized_private_key)
+      LOCKS_EXCLUDED(maps_mutex_);
 
   // Resets the registry.
   // After reset the registry is empty, i.e. it contains neither catalogues
   // nor key managers. This method is intended for testing only.
-  static void Reset();
+  static void Reset() LOCKS_EXCLUDED(maps_mutex_);
 
  private:
-  typedef std::unordered_map<std::string,
-                             std::unique_ptr<void, void (*)(void*)>>
+  typedef std::unordered_map<std::string, std::unique_ptr<void, void (*)(void*)>>
       LabelToObjectMap;
   typedef std::unordered_map<std::string, const char*> LabelToTypeNameMap;
   typedef std::unordered_map<std::string, bool> LabelToBoolMap;
-  typedef std::unordered_map<std::string, const KeyFactory*>
-      LabelToKeyFactoryMap;
+  typedef std::unordered_map<std::string, const KeyFactory*> LabelToKeyFactoryMap;
 
-  static std::recursive_mutex maps_mutex_;
+  static absl::Mutex maps_mutex_;
   // Maps for key manager data.
-  static LabelToObjectMap type_to_manager_map_;      // guarded by maps_mutex_
-  static LabelToTypeNameMap type_to_primitive_map_;  // guarded by maps_mutex_
-  static LabelToBoolMap type_to_new_key_allowed_map_;        // by maps_mutex_
-  static LabelToKeyFactoryMap type_to_key_factory_map_;      // by maps_mutex_
+  static LabelToObjectMap type_to_manager_map_ GUARDED_BY(maps_mutex_);
+  static LabelToTypeNameMap type_to_primitive_map_ GUARDED_BY(maps_mutex_);
+  static LabelToBoolMap type_to_new_key_allowed_map_ GUARDED_BY(maps_mutex_);
+  static LabelToKeyFactoryMap type_to_key_factory_map_ GUARDED_BY(maps_mutex_);
   // Maps for catalogue-data.
-  static LabelToObjectMap name_to_catalogue_map_;    // guarded by maps_mutex_
-  static LabelToTypeNameMap name_to_primitive_map_;  // guarded by maps_mutex_
+  static LabelToObjectMap name_to_catalogue_map_ GUARDED_BY(maps_mutex_);
+  static LabelToTypeNameMap name_to_primitive_map_ GUARDED_BY(maps_mutex_);
 
   static crypto::tink::util::StatusOr<bool> get_new_key_allowed(
-      const std::string& type_url);
+      const std::string& type_url) SHARED_LOCKS_REQUIRED(maps_mutex_);
   static crypto::tink::util::StatusOr<const KeyFactory*> get_key_factory(
-      const std::string& type_url);
+      const std::string& type_url) SHARED_LOCKS_REQUIRED(maps_mutex_);
 };
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -194,7 +201,7 @@ crypto::tink::util::Status Registry::AddCatalogue(
         "Parameter 'catalogue' must be non-null.");
   }
   std::unique_ptr<void, void (*)(void*)> entry(catalogue, delete_catalogue<P>);
-  std::lock_guard<std::recursive_mutex> lock(maps_mutex_);
+  absl::MutexLock lock(&maps_mutex_);
   auto curr_catalogue = name_to_catalogue_map_.find(catalogue_name);
   if (curr_catalogue != name_to_catalogue_map_.end()) {
     auto existing = static_cast<Catalogue<P>*>(curr_catalogue->second.get());
@@ -216,7 +223,7 @@ crypto::tink::util::Status Registry::AddCatalogue(
 template <class P>
 crypto::tink::util::StatusOr<const Catalogue<P>*> Registry::get_catalogue(
     const std::string& catalogue_name) {
-  std::lock_guard<std::recursive_mutex> lock(maps_mutex_);
+  absl::MutexLock lock(&maps_mutex_);
   auto catalogue_entry = name_to_catalogue_map_.find(catalogue_name);
   if (catalogue_entry == name_to_catalogue_map_.end()) {
     return ToStatusF(crypto::tink::util::error::NOT_FOUND,
@@ -249,7 +256,7 @@ crypto::tink::util::Status Registry::RegisterKeyManager(
                      "The manager does not support type '%s'.",
                      type_url.c_str());
   }
-  std::lock_guard<std::recursive_mutex> lock(maps_mutex_);
+  absl::MutexLock lock(&maps_mutex_);
   auto curr_manager = type_to_manager_map_.find(type_url);
   if (curr_manager != type_to_manager_map_.end()) {
     auto existing = static_cast<KeyManager<P>*>(curr_manager->second.get());
@@ -285,7 +292,7 @@ crypto::tink::util::Status Registry::RegisterKeyManager(
 template <class P>
 crypto::tink::util::StatusOr<const KeyManager<P>*> Registry::get_key_manager(
     const std::string& type_url) {
-  std::lock_guard<std::recursive_mutex> lock(maps_mutex_);
+  absl::MutexLock lock(&maps_mutex_);
   auto manager_entry = type_to_manager_map_.find(type_url);
   if (manager_entry == type_to_manager_map_.end()) {
     return ToStatusF(crypto::tink::util::error::NOT_FOUND,