Make X509Certificate::CreateFromHandle() copy the OSCertHandle, rather than assume ownership

R=wtc
BUG=47463
TEST=none

Review URL: http://codereview.chromium.org/2867026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@50938 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 32 insertions, 24 deletions

diff --git a/net/base/cert_test_util.cc b/net/base/cert_test_util.cc
index 372c256..9fc6573 100644
--- a/net/base/cert_test_util.cc
+++ b/net/base/cert_test_util.cc
@@ -55,9 +55,12 @@ X509Certificate* LoadTemporaryRootCert(const FilePath& filename) {
     return NULL;
   }
 
-  return X509Certificate::CreateFromHandle(cert,
+  X509Certificate* result = X509Certificate::CreateFromHandle(
+      cert,
       X509Certificate::SOURCE_LONE_CERT_IMPORT,
       X509Certificate::OSCertHandles());
+  CERT_DestroyCertificate(cert);
+  return result;
 }
 #endif
 
@@ -89,7 +92,7 @@ X509Certificate* LoadTemporaryRootCert(const FilePath& filename) {
 
   SecCertificateRef cert_ref = static_cast<SecCertificateRef>(
       const_cast<void*>(CFArrayGetValueAtIndex(cert_array, 0)));
-  CFRetain(cert_ref);
+
   return X509Certificate::CreateFromHandle(cert_ref,
       X509Certificate::SOURCE_LONE_CERT_IMPORT,
       X509Certificate::OSCertHandles());
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index 367afda..27a4ae0 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -141,8 +141,6 @@ X509Certificate* X509Certificate::CreateFromHandle(
         (cached_cert->source_ == source &&
          cached_cert->HasIntermediateCertificates(intermediates))) {
       // Return the certificate with the same fingerprint from our cache.
-      // But we own the input OSCertHandle, which makes it our job to free it.
-      FreeOSCertHandle(cert_handle);
       DHISTOGRAM_COUNTS("X509CertificateReuseCount", 1);
       return cached_cert;
     }
@@ -163,15 +161,17 @@ X509Certificate* X509Certificate::CreateFromBytes(const char* data,
   if (!cert_handle)
     return NULL;
 
-  return CreateFromHandle(cert_handle,
-                          SOURCE_LONE_CERT_IMPORT,
-                          OSCertHandles());
+  X509Certificate* cert = CreateFromHandle(cert_handle,
+                                           SOURCE_LONE_CERT_IMPORT,
+                                           OSCertHandles());
+  FreeOSCertHandle(cert_handle);
+  return cert;
 }
 
 X509Certificate::X509Certificate(OSCertHandle cert_handle,
                                  Source source,
                                  const OSCertHandles& intermediates)
-    : cert_handle_(cert_handle),
+    : cert_handle_(DupOSCertHandle(cert_handle)),
       source_(source) {
 #if defined(OS_MACOSX) || defined(OS_WIN)
   // Copy/retain the intermediate cert handles.
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index 5c80b2d..b962154 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -82,13 +82,11 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
     VERIFY_EV_CERT = 1 << 1,
   };
 
-  // Create an X509Certificate from a handle to the certificate object
-  // in the underlying crypto library. This is a transfer of ownership;
-  // X509Certificate will properly dispose of |cert_handle| for you.
-  // |source| specifies where |cert_handle| comes from.  Given two
-  // certificate handles for the same certificate, our certificate cache
-  // prefers the handle from the network because our HTTP cache isn't
-  // caching the corresponding intermediate CA certificates yet
+  // Create an X509Certificate from a handle to the certificate object in the
+  // underlying crypto library. |source| specifies where |cert_handle| comes
+  // from.  Given two certificate handles for the same certificate, our
+  // certificate cache prefers the handle from the network because our HTTP
+  // cache isn't caching the corresponding intermediate CA certificates yet
   // (http://crbug.com/7065).
   // The list of intermediate certificates is ignored under NSS (i.e. Linux.)
   // The returned pointer must be stored in a scoped_refptr<X509Certificate>.
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 25e2104..8d7970a 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -735,7 +735,6 @@ bool X509Certificate::IsIssuedBy(
   for (int i = 0; i < n; ++i) {
     SecCertificateRef cert_handle = reinterpret_cast<SecCertificateRef>(
         const_cast<void*>(CFArrayGetValueAtIndex(cert_chain, i)));
-    CFRetain(cert_handle);
     scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
         cert_handle,
         X509Certificate::SOURCE_LONE_CERT_IMPORT,
@@ -795,6 +794,7 @@ bool X509Certificate::GetSSLClientCertificates (
     err = SecIdentityCopyCertificate(identity, &cert_handle);
     if (err != noErr)
       continue;
+    scoped_cftyperef<SecCertificateRef> scoped_cert_handle(cert_handle);
 
     scoped_refptr<X509Certificate> cert(
         CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT,
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index 0ea240f..03c696a 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -372,6 +372,7 @@ TEST(X509CertificateTest, Cache) {
   scoped_refptr<X509Certificate> cert1 = X509Certificate::CreateFromHandle(
       google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
       X509Certificate::OSCertHandles());
+  X509Certificate::FreeOSCertHandle(google_cert_handle);
 
   // Add a certificate from the same source (SOURCE_LONE_CERT_IMPORT).  This
   // should return the cached certificate (cert1).
@@ -380,6 +381,7 @@ TEST(X509CertificateTest, Cache) {
   scoped_refptr<X509Certificate> cert2 = X509Certificate::CreateFromHandle(
       google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
       X509Certificate::OSCertHandles());
+  X509Certificate::FreeOSCertHandle(google_cert_handle);
 
   EXPECT_EQ(cert1, cert2);
 
@@ -390,6 +392,7 @@ TEST(X509CertificateTest, Cache) {
   scoped_refptr<X509Certificate> cert3 = X509Certificate::CreateFromHandle(
       google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
       X509Certificate::OSCertHandles());
+  X509Certificate::FreeOSCertHandle(google_cert_handle);
 
   EXPECT_NE(cert1, cert3);
 
@@ -400,6 +403,7 @@ TEST(X509CertificateTest, Cache) {
   scoped_refptr<X509Certificate> cert4 = X509Certificate::CreateFromHandle(
       google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
       X509Certificate::OSCertHandles());
+  X509Certificate::FreeOSCertHandle(google_cert_handle);
 
   EXPECT_EQ(cert3, cert4);
 
@@ -408,6 +412,7 @@ TEST(X509CertificateTest, Cache) {
   scoped_refptr<X509Certificate> cert5 = X509Certificate::CreateFromHandle(
       google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
       X509Certificate::OSCertHandles());
+  X509Certificate::FreeOSCertHandle(google_cert_handle);
 
   EXPECT_EQ(cert3, cert5);
 }
@@ -495,9 +500,7 @@ TEST(X509CertificateTest, IntermediateCertificates) {
   intermediates2.push_back(thawte_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert2;
   cert2 = X509Certificate::CreateFromHandle(
-      X509Certificate::DupOSCertHandle(google_handle),
-      X509Certificate::SOURCE_FROM_NETWORK,
-      intermediates2);
+      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates2);
 
   // The cache should have stored cert2 'cause it has more intermediates:
   EXPECT_NE(cert1, cert2);
@@ -515,12 +518,13 @@ TEST(X509CertificateTest, IntermediateCertificates) {
   intermediates2.push_back(thawte_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert3;
   cert3 = X509Certificate::CreateFromHandle(
-      X509Certificate::DupOSCertHandle(google_handle),
-      X509Certificate::SOURCE_FROM_NETWORK,
-      intermediates3);
+      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates3);
 
   // The cache should have returned cert2 'cause it has more intermediates:
   EXPECT_EQ(cert3, cert2);
+
+  // Cleanup
+  X509Certificate::FreeOSCertHandle(google_handle);
 }
 #endif
 
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index a02db9b7..23f4230 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -482,8 +482,11 @@ X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
       NULL, reinterpret_cast<const void **>(&cert_handle)))
     return NULL;
 
-  return CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT,
-                          OSCertHandles());
+  X509Certificate* cert = CreateFromHandle(cert_handle,
+                                           SOURCE_LONE_CERT_IMPORT,
+                                           OSCertHandles());
+  FreeOSCertHandle(cert_handle);
+  return cert;
 }
 
 void X509Certificate::Persist(Pickle* pickle) {