Optimize X509 DER cache for memory.

net/cert/x509_util_openssl.cc

Index: net/cert/x509_util_openssl.cc
diff --git a/net/cert/x509_util_openssl.cc b/net/cert/x509_util_openssl.cc
index 2327deddcf29cde90ab173a4e2a4da0a3d74d5d0..af83ed5d43fd1f8cac5cff9905682bb32d43742b 100644
--- a/net/cert/x509_util_openssl.cc
+++ b/net/cert/x509_util_openssl.cc
@@ -10,6 +10,7 @@
 #include <openssl/mem.h>
 
 #include <algorithm>
+#include <map>
 #include <memory>
 
 #include "base/lazy_instance.h"
@@ -17,6 +18,7 @@
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
+#include "base/synchronization/lock.h"
 #include "crypto/ec_private_key.h"
 #include "crypto/openssl_util.h"
 #include "crypto/rsa_private_key.h"
@@ -172,34 +174,83 @@ bool SignAndDerEncodeCert(X509* cert,
   return DerEncodeCert(cert, der_encoded);
 }
 
-struct DERCache {
-  std::string data;
-};
+// Re-encoding the DER data via i2d_X509 is an expensive operation, but
+// it's necessary for comparing two certificates. Re-encode at most once
+// per certificate and cache the pointer to the data within the X509 cert
+// using X509_set_ex_data. For memory efficiency the DER data itself is
+// stored globally, shared between certificates and is deleted once all
+// certificates that reference it are deleted.
+class DerCache {
+ public:
+  // Returns nullptr only if DerEncodeCert() has failed.
+  static const std::string* Get(X509* cert) {
+    const std::string* cached_der = GetInstance().Retrieve(cert);
+    if (cached_der) {
+      return cached_der;
+    }
+    std::string der;
+    if (!DerEncodeCert(cert, &der)) {
+      return nullptr;
+    }
+    return GetInstance().Put(cert, std::move(der));
+  }
 
-void DERCache_free(void* parent, void* ptr, CRYPTO_EX_DATA* ad, int idx,
-                   long argl, void* argp) {
-  DERCache* der_cache = static_cast<DERCache*>(ptr);
-  delete der_cache;
-}
+ private:
+  friend struct base::DefaultLazyInstanceTraits<DerCache>;
 
-class DERCacheInitSingleton {
- public:
-  DERCacheInitSingleton() {
+  DerCache() {
     crypto::EnsureOpenSSLInit();
-    der_cache_ex_index_ = X509_get_ex_new_index(0, 0, 0, 0, DERCache_free);
-    DCHECK_NE(-1, der_cache_ex_index_);
+    x509_value_index_ =
+        X509_get_ex_new_index(0, nullptr, nullptr, nullptr, &free_x509_value);
+    DCHECK_NE(-1, x509_value_index_);
   }
 
-  int der_cache_ex_index() const { return der_cache_ex_index_; }
+  const std::string* Retrieve(X509* cert) const {
+    return static_cast<const std::string*>(
+        X509_get_ex_data(cert, x509_value_index_));
+  }
 
- private:
-  int der_cache_ex_index_;
+  const std::string* Put(X509* cert, std::string der) {
+    base::AutoLock lock(lock_);
+    auto der_iter = count_by_der_.emplace(std::move(der), 0).first;
+    der_iter->second++;  // add a reference
+    const std::string* cached_der = &der_iter->first;
+    X509_set_ex_data(cert, x509_value_index_,
+                     const_cast<std::string*>(cached_der));
+    return cached_der;
+  }
 
-  DISALLOW_COPY_AND_ASSIGN(DERCacheInitSingleton);
-};
+  void Remove(const std::string& der) {
+    base::AutoLock lock(lock_);
+    auto der_iter = count_by_der_.find(der);
+    DCHECK(count_by_der_.end() != der_iter);
+    der_iter->second--;  // remove a reference
+    if (der_iter->second == 0) {
+      count_by_der_.erase(der_iter);
+    }
+  }
+
+  static DerCache& GetInstance() {
+    static base::LazyInstance<DerCache>::Leaky instance =
+        LAZY_INSTANCE_INITIALIZER;
+    return instance.Get();
+  }
 
-base::LazyInstance<DERCacheInitSingleton>::Leaky g_der_cache_singleton =
-    LAZY_INSTANCE_INITIALIZER;
+  static void free_x509_value(void* parent,
+                              void* value,
+                              CRYPTO_EX_DATA* ad,
+                              int idx,
+                              long argl,
+                              void* argp) {
+    const std::string* cached_der = static_cast<const std::string*>(value);
+    DCHECK(cached_der);
+    GetInstance().Remove(*cached_der);
+  }
+
+  int x509_value_index_;
+  std::map<std::string, size_t> count_by_der_;
+  base::Lock lock_;
+};
 
 }  // namespace
 
@@ -284,27 +335,15 @@ bool ParseDate(ASN1_TIME* x509_time, base::Time* time) {
   return ParseCertificateDate(str_date, format, time);
 }
 
-// Returns true if |der_cache| points to valid data, false otherwise.
-// (note: the DER-encoded data in |der_cache| is owned by |cert|, callers should
+// Returns true if |out_der| points to valid data, false otherwise.
+// (note: the DER-encoded data in |out_der| is owned by |cert|, callers should
 // not free it).
-bool GetDER(X509* x509, base::StringPiece* der_cache) {
-  int x509_der_cache_index =
-      g_der_cache_singleton.Get().der_cache_ex_index();
-
-  // Re-encoding the DER data via i2d_X509 is an expensive operation,
-  // but it's necessary for comparing two certificates. Re-encode at
-  // most once per certificate and cache the data within the X509 cert
-  // using X509_set_ex_data.
-  DERCache* internal_cache = static_cast<DERCache*>(
-      X509_get_ex_data(x509, x509_der_cache_index));
-  if (!internal_cache) {
-    std::unique_ptr<DERCache> new_cache(new DERCache);
-    if (!DerEncodeCert(x509, &new_cache->data))
-      return false;
-    internal_cache = new_cache.get();
-    X509_set_ex_data(x509, x509_der_cache_index, new_cache.release());
+bool GetDER(X509* x509, base::StringPiece* out_der) {
+  const std::string* cached_der = DerCache::Get(x509);
+  if (!cached_der) {
+    return false;
   }
-  *der_cache = base::StringPiece(internal_cache->data);
+  *out_der = base::StringPiece(*cached_der);
   return true;
 }