Bug 935769: Fix shutdown locks for nssCerList and nssCertListEnumerator. r=bsmith
authorCamilo Viecco <cviecco@mozilla.com>
Mon, 11 Nov 2013 15:16:45 -0800
changeset 156142 0ed8a5d6395c0c502f69ab3568fe71aa6da53a34
parent 156141 f921dc945f7b8850aed6accb1c5f4c1856e33eb7
child 156143 3f40322844ed2abf92f166dc8d648ee08bb86651
push idunknown
push userunknown
push dateunknown
reviewersbsmith
bugs935769
milestone28.0a1
Bug 935769: Fix shutdown locks for nssCerList and nssCertListEnumerator. r=bsmith
security/manager/ssl/public/nsIX509CertList.idl
security/manager/ssl/src/nsNSSCertCache.cpp
security/manager/ssl/src/nsNSSCertificate.cpp
security/manager/ssl/src/nsNSSCertificate.h
security/manager/ssl/src/nsNSSCertificateDB.cpp
--- a/security/manager/ssl/public/nsIX509CertList.idl
+++ b/security/manager/ssl/public/nsIX509CertList.idl
@@ -8,16 +8,19 @@ interface nsISimpleEnumerator;
 interface nsIX509Cert;
 
 [scriptable, uuid(a539759b-e22d-462f-94ea-2915b11b33e8)]
 interface nsIX509CertList : nsISupports {
    void addCert(in nsIX509Cert cert);
    void deleteCert(in nsIX509Cert cert);
    nsISimpleEnumerator getEnumerator();
 
+   /* getRawCertList MUST be called only from functions where
+    * the nssShutdownPreventionLock has been adquired.
+    */
    [notxpcom, noscript] voidPtr getRawCertList();
 
 };
 
 %{C++
 
 #define NS_X509CERRTLIST_CID { /* 959fb165-6517-487f-ab9b-d8913be53197 */ \
     0x959fb165,                                                           \
--- a/security/manager/ssl/src/nsNSSCertCache.cpp
+++ b/security/manager/ssl/src/nsNSSCertCache.cpp
@@ -47,17 +47,17 @@ nsNSSCertCache::CacheAllCerts()
     return NS_ERROR_NOT_AVAILABLE;
 
   nsCOMPtr<nsIInterfaceRequestor> cxt = new PipUIContext();
   
   CERTCertList *newList = PK11_ListCerts(PK11CertListUnique, cxt);
 
   if (newList) {
     MutexAutoLock lock(mutex);
-    mCertList = new nsNSSCertList(newList, true); // adopt
+    mCertList = new nsNSSCertList(newList, locker);
   }
   
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSCertCache::CacheCertList(nsIX509CertList *list)
 {
--- a/security/manager/ssl/src/nsNSSCertificate.cpp
+++ b/security/manager/ssl/src/nsNSSCertificate.cpp
@@ -1470,33 +1470,56 @@ char* nsNSSCertificate::defaultServerNic
     count++;
   }
   PR_FREEIF(servername);
   return nickname;
 }
 
 NS_IMPL_ISUPPORTS1(nsNSSCertList, nsIX509CertList)
 
-nsNSSCertList::nsNSSCertList(CERTCertList *certList, bool adopt)
+nsNSSCertList::nsNSSCertList(CERTCertList *certList,
+                             const nsNSSShutDownPreventionLock &proofOfLock)
 {
   if (certList) {
-    if (adopt) {
-      mCertList = certList;
-    } else {
-      mCertList = DupCertList(certList);
-    }
+    mCertList = certList;
   } else {
     mCertList = CERT_NewCertList();
   }
 }
 
+nsNSSCertList::~nsNSSCertList()
+{
+  nsNSSShutDownPreventionLock locker;
+  destructorSafeDestroyNSSReference();
+  shutdown(calledFromObject);
+}
+
+void nsNSSCertList::virtualDestroyNSSReference()
+{
+  destructorSafeDestroyNSSReference();
+}
+
+void nsNSSCertList::destructorSafeDestroyNSSReference()
+{
+  if (isAlreadyShutDown()) {
+    return;
+  }
+  if (mCertList) {
+    mCertList = nullptr;
+  }
+}
+
 /* void addCert (in nsIX509Cert cert); */
 NS_IMETHODIMP
 nsNSSCertList::AddCert(nsIX509Cert *aCert) 
 {
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
   /* This should be a query interface, but currently this his how the
    * rest of PSM is working */
   nsCOMPtr<nsIX509Cert2> nssCert = do_QueryInterface(aCert);
   CERTCertificate *cert;
 
   cert = nssCert->GetCert();
   if (!cert) {
     NS_ERROR("Somehow got nullptr for mCertificate in nsNSSCertificate.");
@@ -1510,16 +1533,20 @@ nsNSSCertList::AddCert(nsIX509Cert *aCer
   CERT_AddCertToListTail(mCertList,cert);
   return NS_OK;
 }
 
 /* void deleteCert (in nsIX509Cert cert); */
 NS_IMETHODIMP
 nsNSSCertList::DeleteCert(nsIX509Cert *aCert)
 {
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
   /* This should be a query interface, but currently this his how the
    * rest of PSM is working */
   nsCOMPtr<nsIX509Cert2> nssCert = do_QueryInterface(aCert);
   CERTCertificate *cert = nssCert->GetCert();
   CERTCertListNode *node;
 
   if (!cert) {
     NS_ERROR("Somehow got nullptr for mCertificate in nsNSSCertificate.");
@@ -1537,17 +1564,18 @@ nsNSSCertList::DeleteCert(nsIX509Cert *a
 	CERT_RemoveCertListNode(node);
         return NS_OK;
     }
   }
   return NS_OK; /* should we fail if we couldn't find it? */
 }
 
 CERTCertList *
-nsNSSCertList::DupCertList(CERTCertList *aCertList)
+nsNSSCertList::DupCertList(CERTCertList *aCertList,
+                           const nsNSSShutDownPreventionLock &/*proofOfLock*/)
 {
   if (!aCertList)
     return nullptr;
 
   CERTCertList *newList = CERT_NewCertList();
 
   if (!newList) {
     return nullptr;
@@ -1560,51 +1588,91 @@ nsNSSCertList::DupCertList(CERTCertList 
     CERT_AddCertToListTail(newList, cert);
   }
   return newList;
 }
 
 void *
 nsNSSCertList::GetRawCertList()
 {
+  // This function should only be called after adquiring a
+  // nsNSSShutDownPreventionLock
   return mCertList;
 }
 
 /* nsISimpleEnumerator getEnumerator (); */
 NS_IMETHODIMP
 nsNSSCertList::GetEnumerator(nsISimpleEnumerator **_retval) 
 {
-  nsCOMPtr<nsISimpleEnumerator> enumerator = new nsNSSCertListEnumerator(mCertList);
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+  nsCOMPtr<nsISimpleEnumerator> enumerator =
+    new nsNSSCertListEnumerator(mCertList, locker);
 
   *_retval = enumerator;
   NS_ADDREF(*_retval);
   return NS_OK;
 }
 
 NS_IMPL_ISUPPORTS1(nsNSSCertListEnumerator, nsISimpleEnumerator)
 
-nsNSSCertListEnumerator::nsNSSCertListEnumerator(CERTCertList *certList)
+nsNSSCertListEnumerator::nsNSSCertListEnumerator(CERTCertList *certList,
+                                                 const nsNSSShutDownPreventionLock &proofOfLock)
+{
+  mCertList = nsNSSCertList::DupCertList(certList, proofOfLock);
+}
+
+nsNSSCertListEnumerator::~nsNSSCertListEnumerator()
 {
-  mCertList = nsNSSCertList::DupCertList(certList);
+  nsNSSShutDownPreventionLock locker;
+  destructorSafeDestroyNSSReference();
+  shutdown(calledFromObject);
+}
+
+void nsNSSCertListEnumerator::virtualDestroyNSSReference()
+{
+  destructorSafeDestroyNSSReference();
+}
+
+void nsNSSCertListEnumerator::destructorSafeDestroyNSSReference()
+{
+  if (isAlreadyShutDown()) {
+    return;
+  }
+  if (mCertList) {
+    mCertList = nullptr;
+  }
 }
 
 /* boolean hasMoreElements (); */
 NS_IMETHODIMP
 nsNSSCertListEnumerator::HasMoreElements(bool *_retval)
 { 
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   NS_ENSURE_TRUE(mCertList, NS_ERROR_FAILURE);
 
   *_retval = !CERT_LIST_EMPTY(mCertList);
   return NS_OK;
 }
 
 /* nsISupports getNext(); */
 NS_IMETHODIMP
 nsNSSCertListEnumerator::GetNext(nsISupports **_retval) 
 {
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   NS_ENSURE_TRUE(mCertList, NS_ERROR_FAILURE);
 
   CERTCertListNode *node = CERT_LIST_HEAD(mCertList);
   if (CERT_LIST_END(node, mCertList)) {
     return NS_ERROR_FAILURE;
   }
 
   nsCOMPtr<nsIX509Cert> nssCert = nsNSSCertificate::Create(node->cert);
--- a/security/manager/ssl/src/nsNSSCertificate.h
+++ b/security/manager/ssl/src/nsNSSCertificate.h
@@ -71,43 +71,52 @@ private:
   enum { 
     ev_status_unknown = -1, ev_status_invalid = 0, ev_status_valid = 1
   } mCachedEVStatus;
   SECOidTag mCachedEVOidTag;
   nsresult hasValidEVOidTag(SECOidTag &resultOidTag, bool &validEV);
   nsresult getValidEVOidTag(SECOidTag &resultOidTag, bool &validEV);
 };
 
-class nsNSSCertList: public nsIX509CertList
+class nsNSSCertList: public nsIX509CertList,
+                     public nsNSSShutDownObject
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIX509CERTLIST
 
-  nsNSSCertList(CERTCertList *certList = nullptr, bool adopt = false);
+  nsNSSCertList(CERTCertList *certList,
+                const nsNSSShutDownPreventionLock &proofOfLock);
 
-  static CERTCertList *DupCertList(CERTCertList *aCertList);
+  static CERTCertList *DupCertList(CERTCertList *aCertList,
+                                   const nsNSSShutDownPreventionLock &proofOfLock);
 private:
-   virtual ~nsNSSCertList() { }
+   virtual ~nsNSSCertList();
+   virtual void virtualDestroyNSSReference();
+   void destructorSafeDestroyNSSReference();
 
    mozilla::ScopedCERTCertList mCertList;
 
    nsNSSCertList(const nsNSSCertList &) MOZ_DELETE;
    void operator=(const nsNSSCertList &) MOZ_DELETE;
 };
 
-class nsNSSCertListEnumerator: public nsISimpleEnumerator
+class nsNSSCertListEnumerator: public nsISimpleEnumerator,
+                               public nsNSSShutDownObject
 {
 public:
    NS_DECL_THREADSAFE_ISUPPORTS
    NS_DECL_NSISIMPLEENUMERATOR
 
-   nsNSSCertListEnumerator(CERTCertList *certList);
+   nsNSSCertListEnumerator(CERTCertList *certList,
+                           const nsNSSShutDownPreventionLock &proofOfLock);
 private:
-   virtual ~nsNSSCertListEnumerator() { }
+   virtual ~nsNSSCertListEnumerator();
+   virtual void virtualDestroyNSSReference();
+   void destructorSafeDestroyNSSReference();
 
    mozilla::ScopedCERTCertList mCertList;
 
    nsNSSCertListEnumerator(const nsNSSCertListEnumerator &) MOZ_DELETE;
    void operator=(const nsNSSCertListEnumerator &) MOZ_DELETE;
 };
 
 
--- a/security/manager/ssl/src/nsNSSCertificateDB.cpp
+++ b/security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ -1634,17 +1634,17 @@ nsNSSCertificateDB::GetCerts(nsIX509Cert
   CERTCertList *certList;
 
   nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
   nsCOMPtr<nsIX509CertList> nssCertList;
   certList = PK11_ListCerts(PK11CertListUnique, ctx);
 
   // nsNSSCertList 1) adopts certList, and 2) handles the nullptr case fine.
   // (returns an empty list) 
-  nssCertList = new nsNSSCertList(certList, true);
+  nssCertList = new nsNSSCertList(certList, locker);
 
   *_retval = nssCertList;
   NS_ADDREF(*_retval);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSCertificateDB::GetRecentBadCerts(bool isPrivate, nsIRecentBadCerts** result)