Minor clean-up tasks that were TODO(snej)

net/base/x509_certificate.cc

 // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/x509_certificate.h"
 
 #if defined(OS_MACOSX)
 #include <Security/Security.h>
 #elif defined(USE_NSS)
 #include <cert.h>
 #endif
 
+#include <map>
+
 #include "base/histogram.h"
 #include "base/logging.h"
+#include "base/singleton.h"
 #include "base/time.h"
 
 namespace net {
 
 namespace {
 
 // Returns true if this cert fingerprint is the null (all zero) fingerprint.
 // We use this as a bogus fingerprint value.
-bool IsNullFingerprint(const X509Certificate::Fingerprint& fingerprint) {
+bool IsNullFingerprint(const SHA1Fingerprint& fingerprint) {
   for (size_t i = 0; i < arraysize(fingerprint.data); ++i) {
     if (fingerprint.data[i] != 0)
       return false;
   }
   return true;
 }
 
 }  // namespace
 
 // static
@@ skipping 16 matching lines @@
 #elif defined(USE_NSS)
   return a->derCert.len == b->derCert.len &&
       memcmp(a->derCert.data, b->derCert.data, a->derCert.len) == 0;
 #else
   // TODO(snej): not implemented
   UNREACHED();
   return false;
 #endif
 }
 
-bool X509Certificate::FingerprintLessThan::operator()(
-    const SHA1Fingerprint& lhs,
-    const SHA1Fingerprint& rhs) const {
-  for (size_t i = 0; i < sizeof(lhs.data); ++i) {
-    if (lhs.data[i] < rhs.data[i])
-      return true;
-    if (lhs.data[i] > rhs.data[i])
-      return false;
-  }
-  return false;
-}
-
 bool X509Certificate::LessThan::operator()(X509Certificate* lhs,
                                            X509Certificate* rhs) const {
   if (lhs == rhs)
     return false;
 
-  X509Certificate::FingerprintLessThan fingerprint_functor;
+  SHA1FingerprintLessThan fingerprint_functor;
   return fingerprint_functor(lhs->fingerprint_, rhs->fingerprint_);
 }
 
 // A thread-safe cache for X509Certificate objects.
 //
 // 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): Fix the cache to hold references, as there is a race where:
+//   Thread A: Release the last reference to an X509Certificate.
+//   Thread B: Calls Find(), which acquires |lock_|
+//   Thread A: X509Certificate DTOR invokes, and calls Remove(), which blocks
+//             on |lock_|
+//   Thread B: Finds the X509Certificate (that is DTORing in Thread A) and
+//             returns it, releasing |lock_|
+//   Thread A: Acquires |lock_|, removes the X509Certificate, and then
+//             proceeds with DTOR
+//   Thread B: Now holds a dead pointer
+class X509Certificate::Cache {
+ public:
+  static Cache* GetInstance();
+  void Insert(X509Certificate* cert);
+  void Remove(X509Certificate* cert);
+  X509Certificate* Find(const SHA1Fingerprint& fingerprint);
+
+ private:
+  typedef std::map<SHA1Fingerprint, X509Certificate*, SHA1FingerprintLessThan>
+      CertMap;
+
+  // Obtain an instance of X509Certificate::Cache via GetInstance().
+  Cache() {}
+  friend struct DefaultSingletonTraits<Cache>;
+
+  // 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_|.
+  CertMap cache_;
+
+  DISALLOW_COPY_AND_ASSIGN(Cache);
+};
 
 // Get the singleton object for the cache.
 // static
 X509Certificate::Cache* X509Certificate::Cache::GetInstance() {
   return Singleton<X509Certificate::Cache>::get();
 }
 
 // Insert |cert| into the cache.  The cache does NOT AddRef |cert|.
 // Any existing certificate with the same fingerprint will be replaced.
 void X509Certificate::Cache::Insert(X509Certificate* cert) {
@@ skipping 10 matching lines @@
   AutoLock lock(lock_);
 
   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);
 };
 
 // Find a certificate in the cache with the given fingerprint.  If one does
 // not exist, this method returns NULL.
-X509Certificate* X509Certificate::Cache::Find(const Fingerprint& fingerprint) {
+X509Certificate* X509Certificate::Cache::Find(
+    const SHA1Fingerprint& fingerprint) {
   AutoLock lock(lock_);
 
   CertMap::iterator pos(cache_.find(fingerprint));
   if (pos == cache_.end())
     return NULL;
 
   return pos->second;
 };
 
 // static
@@ skipping 97 matching lines @@
 
 bool X509Certificate::HasIntermediateCertificates(const OSCertHandles& certs) {
   for (size_t i = 0; i < certs.size(); ++i) {
     if (!HasIntermediateCertificate(certs[i]))
       return false;
   }
   return true;
 }
 
 }  // namespace net