bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent r=jcj
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 07 Dec 2017 15:08:43 -0800
changeset 395774 89457644e9227b2710919dfb19e73bba7c0656e7
parent 395773 e6cd64922e36f13ed05be389be95c31accbcd540
child 395775 bc0b13dae4bee9347397c9038eb1fcf77b4ccb9a
push id56806
push userdkeeler@mozilla.com
push dateFri, 08 Dec 2017 17:36:18 +0000
treeherderautoland@89457644e922 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1424085
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent r=jcj nsIX509Cert::GetCert() returns a CERTCertificate whose reference count has already been increased. Before this patch, when IsCertificateDistrustImminent called CertDNIsInList(rootCert->GetCert(), RootSymantecDNs) and CertDNIsInList(aCert->GetCert(), RootAppleAndGoogleDNs), the reference count on those certificates would never get a corresponding decrement, so we would keep those certificates alive until shut down. A reasonable and consistent solution is to introduce a UniqueCERTCertificate handle in each case to own the reference. The status of this fix can be verified by setting MOZ_LOG="pipnss:4", running Firefox, connecting to any host, and then shutting down. If an NSS resource reference has been leaked, "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" will be in the console output. Otherwise, "[Main Thread]: D/pipnss NSS shutdown =====>> OK <<=====" will be in the console output. This patch also removes nsIX509CertList::DeleteCert because it would also leak a reference. Luckily, nothing was using it. This patch also clarifies the implementation of nsIX509CertList::AddCert by making the ownership transfers explicit. MozReview-Commit-ID: 2qHo3DmhTPz
security/manager/ssl/nsIX509CertList.idl
security/manager/ssl/nsNSSCallbacks.cpp
security/manager/ssl/nsNSSCertificate.cpp
--- a/security/manager/ssl/nsIX509CertList.idl
+++ b/security/manager/ssl/nsIX509CertList.idl
@@ -17,18 +17,16 @@ class nsNSSCertList;
 %}
 [ptr] native nsNSSCertListPtr(nsNSSCertList);
 
 [scriptable, builtinclass, uuid(ae74cda5-cd2f-473f-96f5-f0b7fff62c68)]
 interface nsIX509CertList : nsISupports {
   [must_use]
   void addCert(in nsIX509Cert cert);
   [must_use]
-  void deleteCert(in nsIX509Cert cert);
-  [must_use]
   nsISimpleEnumerator getEnumerator();
 
   /**
    * Returns the raw, backing cert list.
    * Must be called only from functions where an nsNSSShutDownPreventionLock
    * has been acquired.
    */
   [notxpcom, noscript, must_use]
--- a/security/manager/ssl/nsNSSCallbacks.cpp
+++ b/security/manager/ssl/nsNSSCallbacks.cpp
@@ -1303,30 +1303,34 @@ IsCertificateDistrustImminent(nsIX509Cer
 
   // If the end entity's notBefore date is after 2016-06-01, this algorithm
   // doesn't apply, so exit false before we do any iterating
   if (notBefore >= JUNE_1_2016) {
     aResult = false;
     return NS_OK;
   }
 
+  // We need an owning handle when calling nsIX509Cert::GetCert().
+  UniqueCERTCertificate nssRootCert(rootCert->GetCert());
   // If the root is not one of the Symantec roots, exit false
-  if (!CertDNIsInList(rootCert->GetCert(), RootSymantecDNs)) {
+  if (!CertDNIsInList(nssRootCert.get(), RootSymantecDNs)) {
     aResult = false;
     return NS_OK;
   }
 
   // Look for one of the intermediates to be in the whitelist
   bool foundInWhitelist = false;
   RefPtr<nsNSSCertList> intCertList = intCerts->GetCertList();
 
   intCertList->ForEachCertificateInChain(
     [&foundInWhitelist] (nsCOMPtr<nsIX509Cert> aCert, bool aHasMore,
                          /* out */ bool& aContinue) {
-      if (CertDNIsInList(aCert->GetCert(), RootAppleAndGoogleDNs)) {
+      // We need an owning handle when calling nsIX509Cert::GetCert().
+      UniqueCERTCertificate nssCert(aCert->GetCert());
+      if (CertDNIsInList(nssCert.get(), RootAppleAndGoogleDNs)) {
         foundInWhitelist = true;
         aContinue = false;
       }
       return NS_OK;
   });
 
   // If this chain did not match the whitelist, exit true
   aResult = !foundInWhitelist;
--- a/security/manager/ssl/nsNSSCertificate.cpp
+++ b/security/manager/ssl/nsNSSCertificate.cpp
@@ -1150,59 +1150,32 @@ nsNSSCertList::GetCertList()
 
 NS_IMETHODIMP
 nsNSSCertList::AddCert(nsIX509Cert* aCert)
 {
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     return NS_ERROR_NOT_AVAILABLE;
   }
-  CERTCertificate* cert = aCert->GetCert();
+  // We need an owning handle when calling nsIX509Cert::GetCert().
+  UniqueCERTCertificate cert(aCert->GetCert());
   if (!cert) {
     NS_ERROR("Somehow got nullptr for mCertificate in nsNSSCertificate.");
     return NS_ERROR_FAILURE;
   }
 
   if (!mCertList) {
     NS_ERROR("Somehow got nullptr for mCertList in nsNSSCertList.");
     return NS_ERROR_FAILURE;
   }
-  // XXX: check return value!
-  CERT_AddCertToListTail(mCertList.get(), cert);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsNSSCertList::DeleteCert(nsIX509Cert* aCert)
-{
-  nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown()) {
-    return NS_ERROR_NOT_AVAILABLE;
+  if (CERT_AddCertToListTail(mCertList.get(), cert.get()) != SECSuccess) {
+    return NS_ERROR_OUT_OF_MEMORY;
   }
-  CERTCertificate* cert = aCert->GetCert();
-  CERTCertListNode* node;
-
-  if (!cert) {
-    NS_ERROR("Somehow got nullptr for mCertificate in nsNSSCertificate.");
-    return NS_ERROR_FAILURE;
-  }
-
-  if (!mCertList) {
-    NS_ERROR("Somehow got nullptr for mCertList in nsNSSCertList.");
-    return NS_ERROR_FAILURE;
-  }
-
-  for (node = CERT_LIST_HEAD(mCertList.get());
-       !CERT_LIST_END(node, mCertList.get()); node = CERT_LIST_NEXT(node)) {
-    if (node->cert == cert) {
-	CERT_RemoveCertListNode(node);
-        return NS_OK;
-    }
-  }
-  return NS_OK; // XXX Should we fail if we couldn't find it?
+  Unused << cert.release(); // Ownership transferred to the cert list.
+  return NS_OK;
 }
 
 UniqueCERTCertList
 nsNSSCertList::DupCertList(const UniqueCERTCertList& certList,
                            const nsNSSShutDownPreventionLock& /*proofOfLock*/)
 {
   if (!certList) {
     return nullptr;