Refactor X509Certificate caching to cache the OS handle, rather than the X509Certificate

net/base/x509_certificate.cc

Index: net/base/x509_certificate.cc
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index 6a2aaf8dd701222470aa5ebb3b26c0ed75e89d5c..24a39ded6b398fd656072ae313dfc0be60d7c53a 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -41,30 +41,59 @@ const char kCertificateHeader[] = "CERTIFICATE";
 // The PEM block header used for PKCS#7 data
 const char kPKCS7Header[] = "PKCS7";
 
-// A thread-safe cache for X509Certificate objects.
+// A thread-safe cache for OS certificate handles.
 //
-// The cache does not hold a reference to the certificate objects.  The objects
-// must |Remove| themselves from the cache upon destruction (or else the cache
-// will be holding dead pointers to the objects).
-// TODO(rsleevi): There exists a chance of a use-after-free, due to a race
-// between Find() and Remove(). See http://crbug.com/49377
+// Within each of the supported underlying crypto libraries, a certificate
+// handle is represented as a ref-counted object that contains the parsed
+// data for the certificate. In addition, the underlying OS handle may also
+// contain a copy of the original ASN.1 DER used to constructed the handle.
+//
+// In order to reduce the memory usage when multiple SSL connections exist,
+// with each connection storing the server's identity certificate plus any
+// intermediates supplied, the certificate handles are cached. Any two
+// X509Certificates that were created from the same ASN.1 DER data,
+// regardless of where that data came from, will share the same underlying
+// OS certificate handle.
 class X509CertificateCache {
  public:
-  void Insert(X509Certificate* cert);
-  void Remove(X509Certificate* cert);
-  X509Certificate* Find(const SHA1Fingerprint& fingerprint);
+  // Performs a compare-and-swap like operation. If an OS certificate handle
+  // for the same certificate data as |*cert_handle| already exists in the
+  // cache, the original |*cert_handle| will be freed, and |cert_handle|
+  // will be updated to point to the existing cached certificate, after
+  // adding to the reference count. If an equivalent OS certificate handle
+  // is not found, |*cert_handle| will be added to the cache and its
+  // reference count increased, while otherwise remaining unmodified and
+  // pointing to the caller's original certificate.
+  void InsertOrUpdate(X509Certificate::OSCertHandle* cert_handle);
+
+  // Decrements the reference count for |cert_handle|, and if it is the last
+  // reference held in the cache, frees the underlying OS certificate
+  // handle. InsertOrUpdate() must have been called prior to Remove() for
+  // the |cert_handle|.
+  void Remove(X509Certificate::OSCertHandle cert_handle);
 
  private:
-  typedef std::map<SHA1Fingerprint, X509Certificate*, SHA1FingerprintLessThan>
-      CertMap;
+  // A single entry in the cache. Certificates will be keyed by their SHA1
+  // fingerprints, but will not be considered equivalent unless the entire
+  // certificate data matches.
+  struct Entry {
+    Entry() : cert_handle(NULL), ref_count(0) {}
+    X509Certificate::OSCertHandle cert_handle;
+
+    // Increased by each call to InsertOrUpdate(), and balanced by each call
+    // to Remove(). When it equals 0, all references have been released, so
+    // the cache entry will be removed and the OS certificate released.
+    size_t ref_count;
+  };
+  typedef std::map<SHA1Fingerprint, Entry, SHA1FingerprintLessThan> CertMap;
 
   // Obtain an instance of X509CertificateCache via a LazyInstance.
   X509CertificateCache() {}
   ~X509CertificateCache() {}
   friend struct base::DefaultLazyInstanceTraits<X509CertificateCache>;
 
-  // You must acquire this lock before using any private data of this object.
-  // You must not block while holding this lock.
+  // You must acquire this lock before using any private data of this
+  // object. You must not block while holding this lock.
   Lock lock_;
 
   // The certificate cache.  You must acquire |lock_| before using |cache_|.
@@ -77,39 +106,69 @@ base::LazyInstance<X509CertificateCache,
                    base::LeakyLazyInstanceTraits<X509CertificateCache> >
     g_x509_certificate_cache(base::LINKER_INITIALIZED);
 
-// Insert |cert| into the cache.  The cache does NOT AddRef |cert|.
-// Any existing certificate with the same fingerprint will be replaced.
-void X509CertificateCache::Insert(X509Certificate* cert) {
-  AutoLock lock(lock_);
-
-  DCHECK(!IsNullFingerprint(cert->fingerprint())) <<
-      "Only insert certs with real fingerprints.";
-  cache_[cert->fingerprint()] = cert;
-};
+void X509CertificateCache::InsertOrUpdate(
+    X509Certificate::OSCertHandle* cert_handle) {
+  DCHECK(cert_handle);
+  SHA1Fingerprint fingerprint =
+      X509Certificate::CalculateFingerprint(*cert_handle);
+  DCHECK(!IsNullFingerprint(fingerprint)) <<
+       "Only insert certs with real fingerprints.";
 
-// Remove |cert| from the cache.  The cache does not assume that |cert| is
-// already in the cache.
-void X509CertificateCache::Remove(X509Certificate* cert) {
   AutoLock lock(lock_);
+  CertMap::iterator pos(cache_.find(fingerprint));
+  if (pos == cache_.end()) {
+    // Cached entry not found. Initialize a new entry. |*cert_handle| is
+    // guaranteed to have at least one OS reference (the caller's), which
+    // will be incremented, along with cache_entry.ref_count, below.
+    Entry cache_entry;
+    cache_entry.cert_handle = *cert_handle;
+    cache_entry.ref_count = 0;
+    CertMap::value_type cache_value(fingerprint, cache_entry);
+    pos = cache_.insert(cache_value).first;
+  } else {
+    bool is_same_cert =
+        X509Certificate::IsSameOSCert(*cert_handle, pos->second.cert_handle);
+    if (!is_same_cert) {
+      // Two certificates don't match, likely due to a SHA1 hash collision.
+      // Given the low probability, the simplest solution is to not cache
+      // the certificate, which should not affect performance too
+      // negatively.
+      return;
+    }
+    // Release the caller's reference to |*cert_handle|, as it will be
+    // replaced by the cached handle just found.
+    X509Certificate::FreeOSCertHandle(*cert_handle);
+    DHISTOGRAM_COUNTS("X509CertificateReuseCount", 1);
+  }
 
-  CertMap::iterator pos(cache_.find(cert->fingerprint()));
-  if (pos == cache_.end())
-    return;  // It is not an error to remove a cert that is not in the cache.
-  cache_.erase(pos);
-};
+  ++pos->second.ref_count;
+  *cert_handle = X509Certificate::DupOSCertHandle(pos->second.cert_handle);
+}
 
-// Find a certificate in the cache with the given fingerprint.  If one does
-// not exist, this method returns NULL.
-X509Certificate* X509CertificateCache::Find(
-    const SHA1Fingerprint& fingerprint) {
+void X509CertificateCache::Remove(X509Certificate::OSCertHandle cert_handle) {
+  SHA1Fingerprint fingerprint =
+      X509Certificate::CalculateFingerprint(cert_handle);
   AutoLock lock(lock_);
 
-  CertMap::iterator pos(cache_.find(fingerprint));
-  if (pos == cache_.end())
-    return NULL;
-
-  return pos->second;
-};
+   CertMap::iterator pos(cache_.find(fingerprint));
+   if (pos == cache_.end())
+    return;  // A cache collision where the winning cert was already freed.
+
+  bool is_same_cert = X509Certificate::IsSameOSCert(cert_handle,
+                                                    pos->second.cert_handle);
+  DCHECK(is_same_cert) << "Hash collision between different certificates";
+  if (!is_same_cert)
+    return;  // A cache collision where the winning cert is still around.
+
+  if (--pos->second.ref_count == 0) {
+    // The last reference to |cert_handle| has been removed, so release the
+    // underlying OS handle and remove from the cache. The caller still
+    // holds a reference to the underlying OS handle and still bears
+    // responsibility for freeing it.
+    X509Certificate::FreeOSCertHandle(pos->second.cert_handle);
+    cache_.erase(pos);
+  }
+}
 
 }  // namespace
 
@@ -125,31 +184,9 @@ bool X509Certificate::LessThan::operator()(X509Certificate* lhs,
 // static
 X509Certificate* X509Certificate::CreateFromHandle(
     OSCertHandle cert_handle,
-    Source source,
     const OSCertHandles& intermediates) {
   DCHECK(cert_handle);
-  DCHECK(source != SOURCE_UNUSED);
-
-  // Check if we already have this certificate in memory.
-  X509CertificateCache* cache = g_x509_certificate_cache.Pointer();
-  X509Certificate* cached_cert =
-      cache->Find(CalculateFingerprint(cert_handle));
-  if (cached_cert) {
-    DCHECK(cached_cert->source_ != SOURCE_UNUSED);
-    if (cached_cert->source_ > source ||
-        (cached_cert->source_ == source &&
-         cached_cert->HasIntermediateCertificates(intermediates))) {
-      // Return the certificate with the same fingerprint from our cache.
-      DHISTOGRAM_COUNTS("X509CertificateReuseCount", 1);
-      return cached_cert;
-    }
-    // Else the new cert is better and will replace the old one in the cache.
-  }
-
-  // Otherwise, allocate and cache a new object.
-  X509Certificate* cert = new X509Certificate(cert_handle, source,
-                                              intermediates);
-  cache->Insert(cert);
+  X509Certificate* cert = new X509Certificate(cert_handle, intermediates);
   return cert;
 }
 
@@ -184,8 +221,7 @@ X509Certificate* X509Certificate::CreateFromDERCertChain(
 
   OSCertHandle handle = CreateOSCert(der_certs[0]);
   DCHECK(handle);
-  X509Certificate* cert =
-      CreateFromHandle(handle, SOURCE_FROM_NETWORK, intermediate_ca_certs);
+  X509Certificate* cert = CreateFromHandle(handle, intermediate_ca_certs);
   FreeOSCertHandle(handle);
   for (size_t i = 0; i < intermediate_ca_certs.size(); i++)
     FreeOSCertHandle(intermediate_ca_certs[i]);
@@ -200,9 +236,7 @@ X509Certificate* X509Certificate::CreateFromBytes(const char* data,
   if (!cert_handle)
     return NULL;
 
-  X509Certificate* cert = CreateFromHandle(cert_handle,
-                                           SOURCE_LONE_CERT_IMPORT,
-                                           OSCertHandles());
+  X509Certificate* cert = CreateFromHandle(cert_handle, OSCertHandles());
   FreeOSCertHandle(cert_handle);
   return cert;
 }
@@ -247,8 +281,7 @@ X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
 
   if (!cert_handle)
     return NULL;
-  X509Certificate* cert = CreateFromHandle(cert_handle, SOURCE_FROM_CACHE,
-                                           intermediates);
+  X509Certificate* cert = CreateFromHandle(cert_handle, intermediates);
   FreeOSCertHandle(cert_handle);
   for (size_t i = 0; i < intermediates.size(); ++i)
     FreeOSCertHandle(intermediates[i]);
@@ -325,8 +358,7 @@ CertificateList X509Certificate::CreateCertificateListFromBytes(
 
   for (OSCertHandles::iterator it = certificates.begin();
        it != certificates.end(); ++it) {
-    X509Certificate* result = CreateFromHandle(*it, SOURCE_LONE_CERT_IMPORT,
-                                               OSCertHandles());
+    X509Certificate* result = CreateFromHandle(*it, OSCertHandles());
     results.push_back(scoped_refptr<X509Certificate>(result));
     FreeOSCertHandle(*it);
   }
@@ -335,13 +367,19 @@ CertificateList X509Certificate::CreateCertificateListFromBytes(
 }
 
 X509Certificate::X509Certificate(OSCertHandle cert_handle,
-                                 Source source,
                                  const OSCertHandles& intermediates)
-    : cert_handle_(DupOSCertHandle(cert_handle)),
-      source_(source) {
-  // Copy/retain the intermediate cert handles.
-  for (size_t i = 0; i < intermediates.size(); ++i)
-    intermediate_ca_certs_.push_back(DupOSCertHandle(intermediates[i]));
+    : cert_handle_(DupOSCertHandle(cert_handle)) {
+  X509CertificateCache* cache = g_x509_certificate_cache.Pointer();
+  cache->InsertOrUpdate(&cert_handle_);
+  for (size_t i = 0; i < intermediates.size(); ++i) {
+    // Duplicate the incoming certificate, as the caller retains ownership
+    // of |intermediates|.
+    OSCertHandle intermediate = DupOSCertHandle(intermediates[i]);
+    // Update the cache, which will assume ownership of the duplicated
+    // handle and return a suitable equivalent, potentially from the cache.
+    cache->InsertOrUpdate(&intermediate);
+    intermediate_ca_certs_.push_back(intermediate);
+  }
   // Platform-specific initialization.
   Initialize();
 }
@@ -354,18 +392,20 @@ X509Certificate::X509Certificate(const std::string& subject,
       issuer_(issuer),
       valid_start_(start_date),
       valid_expiry_(expiration_date),
-      cert_handle_(NULL),
-      source_(SOURCE_UNUSED) {
+      cert_handle_(NULL) {
   memset(fingerprint_.data, 0, sizeof(fingerprint_.data));
 }
 
 X509Certificate::~X509Certificate() {
-  // We might not be in the cache, but it is safe to remove ourselves anyway.
-  g_x509_certificate_cache.Get().Remove(this);
-  if (cert_handle_)
+  X509CertificateCache* cache = g_x509_certificate_cache.Pointer();
+  if (cert_handle_) {
+    cache->Remove(cert_handle_);
     FreeOSCertHandle(cert_handle_);
-  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i)
+  }
+  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
+    cache->Remove(intermediate_ca_certs_[i]);
     FreeOSCertHandle(intermediate_ca_certs_[i]);
+  }
 }
 
 void X509Certificate::Persist(Pickle* pickle) {