bug 177366, clean up refcounting

r=relyea

5 files changed, 96 insertions, 93 deletions

diff --git a/security/nss/lib/pki/certificate.c b/security/nss/lib/pki/certificate.c
index c7c616d7e..22196584d 100644
--- a/security/nss/lib/pki/certificate.c
+++ b/security/nss/lib/pki/certificate.c
@@ -51,6 +51,8 @@ static const char CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ $Name$";
 #include "dev.h"
 #endif /* DEV_H */
 
+#include "pkistore.h"
+
 #ifdef NSS_3_4_CODE
 #include "pki3hack.h"
 #include "pk11func.h"
@@ -116,13 +118,43 @@ nssCertificate_Destroy (
   NSSCertificate *c
 )
 {
-    PRBool destroyed;
     if (c) {
+	PRUint32 i;
 	nssDecodedCert *dc = c->decoding;
-	destroyed = nssPKIObject_Destroy(&c->object);
-	if (destroyed) {
-	    if (dc) {
-		nssDecodedCert_Destroy(dc);
+	NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
+	NSSCryptoContext *cc = c->object.cryptoContext;
+
+	PR_ASSERT(c->object.refCount > 0);
+
+	/* --- LOCK storage --- */
+	if (cc) {
+	    nssCertificateStore_Lock(cc->certStore);
+	} else {
+	    nssTrustDomain_LockCertCache(td);
+	}
+	PR_AtomicDecrement(&c->object.refCount);
+	if (c->object.refCount == 0) {
+	    /* --- remove cert and UNLOCK storage --- */
+	    if (cc) {
+		nssCertificateStore_RemoveCertLOCKED(cc->certStore, c);
+		nssCertificateStore_Unlock(cc->certStore);
+	    } else {
+		nssTrustDomain_RemoveCertFromCacheLOCKED(td, c);
+		nssTrustDomain_UnlockCertCache(td);
+	    }
+	    /* free cert data */
+	    for (i=0; i<c->object.numInstances; i++) {
+		nssCryptokiObject_Destroy(c->object.instances[i]);
+	    }
+	    PZ_DestroyLock(c->object.lock);
+	    nssArena_Destroy(c->object.arena);
+	    nssDecodedCert_Destroy(dc);
+	} else {
+	    /* --- UNLOCK storage --- */
+	    if (cc) {
+		nssCertificateStore_Unlock(cc->certStore);
+	    } else {
+		nssTrustDomain_UnlockCertCache(td);
 	    }
 	}
     }
diff --git a/security/nss/lib/pki/pkim.h b/security/nss/lib/pki/pkim.h
index 4ed5c0e9c..9f6ad74c2 100644
--- a/security/nss/lib/pki/pkim.h
+++ b/security/nss/lib/pki/pkim.h
@@ -593,17 +593,19 @@ nssTrustDomain_AddCertsToCache
 );
 
 NSS_EXTERN void
-nssTrustDomain_RemoveCertFromCache
-(
+nssTrustDomain_RemoveCertFromCacheLOCKED (
   NSSTrustDomain *td,
   NSSCertificate *cert
 );
 
 NSS_EXTERN void
-nssTrustDomain_FlushCache
-(
-  NSSTrustDomain *td,
-  PRFloat64 threshold
+nssTrustDomain_LockCertCache (
+  NSSTrustDomain *td
+);
+
+NSS_EXTERN void
+nssTrustDomain_UnlockCertCache (
+  NSSTrustDomain *td
 );
 
 NSS_IMPLEMENT PRStatus
diff --git a/security/nss/lib/pki/pkistore.c b/security/nss/lib/pki/pkistore.c
index 2109978a9..9d843312c 100644
--- a/security/nss/lib/pki/pkistore.c
+++ b/security/nss/lib/pki/pkistore.c
@@ -247,9 +247,7 @@ nssCertificateStore_Add (
     nssrv = add_certificate_entry(store, cert);
     if (nssrv == PR_SUCCESS) {
 	nssrv = add_subject_entry(store, cert);
-	if (nssrv == PR_SUCCESS) {
-	    nssCertificate_AddRef(cert); /* obtain a reference for the store */
-	} else {
+	if (nssrv == PR_FAILURE) {
 	    remove_certificate_entry(store, cert);
 	}
     }
@@ -306,47 +304,33 @@ remove_subject_entry (
 }
 
 NSS_IMPLEMENT void
-nssCertificateStore_Remove (
+nssCertificateStore_RemoveCertLOCKED (
   nssCertificateStore *store,
-  NSSCertificate *cert,
-  PRBool force /* described in bug 171198 */
+  NSSCertificate *cert
 )
 {
     certificate_hash_entry *entry;
-    PZ_Lock(store->lock);
-#ifdef NSS_3_4_CODE
-    if (!force && cert->object.refCount > 2) {
-	/* This continues the hack described in CERT_DestroyCertificate.
-	 * Because NSS 3.4 maintains a single, global, crypto context,
-	 * certs must be explicitly removed from it when there are no
-	 * more references to them.  This is done by destroying the cert
-	 * when there are two references left, the one being destroyed,
-	 * and the one here (read: temp db).
-	 * However, there is a race condition with timing the removal
-	 * of the cert from the temp store and deleting the last
-	 * reference.  In CERT_DestroyCertificate, the refCount is checked,
-	 * and if it is two, a call is made here to remove the temp cert.
-	 * But by the time it gets here (and within the safety of the
-	 * store's lock), another thread could have grabbed a reference
-	 * to it.  Removing it now will wreak havoc.
-	 * Therefore, it is necessary to check the refCount again, 
-	 * after obtaining the store's lock, to make sure the cert is
-	 * actually ready to be deleted.  This check is safe, because
-	 * within the store's lock a cert that has only two references
-	 * *must* have one in the store, and the one being deleted.
-	 * See bug 125263.
-	 */
-	PZ_Unlock(store->lock);
-	return;
-    }
-#endif
     entry = (certificate_hash_entry *)
                               nssHash_Lookup(store->issuer_and_serial, cert);
     if (entry && entry->cert == cert) {
 	remove_certificate_entry(store, cert);
 	remove_subject_entry(store, cert);
-	NSSCertificate_Destroy(cert); /* release the store's reference */
     }
+}
+
+NSS_IMPLEMENT void
+nssCertificateStore_Lock (
+  nssCertificateStore *store
+)
+{
+    PZ_Lock(store->lock);
+}
+
+NSS_IMPLEMENT void
+nssCertificateStore_Unlock (
+  nssCertificateStore *store
+)
+{
     PZ_Unlock(store->lock);
 }
 
diff --git a/security/nss/lib/pki/pkistore.h b/security/nss/lib/pki/pkistore.h
index ce4c96099..175298891 100644
--- a/security/nss/lib/pki/pkistore.h
+++ b/security/nss/lib/pki/pkistore.h
@@ -86,11 +86,20 @@ nssCertificateStore_Add
 );
 
 NSS_EXTERN void
-nssCertificateStore_Remove
+nssCertificateStore_RemoveCertLOCKED
 (
   nssCertificateStore *store,
-  NSSCertificate *cert,
-  PRBool force /* described in bug 171198 */
+  NSSCertificate *cert
+);
+
+NSS_EXTERN void
+nssCertificateStore_Lock (
+  nssCertificateStore *store
+);
+
+NSS_EXTERN void
+nssCertificateStore_Unlock (
+  nssCertificateStore *store
 );
 
 NSS_EXTERN NSSCertificate **
diff --git a/security/nss/lib/pki/tdcache.c b/security/nss/lib/pki/tdcache.c
index cb6fa4841..8f88899e1 100644
--- a/security/nss/lib/pki/tdcache.c
+++ b/security/nss/lib/pki/tdcache.c
@@ -362,13 +362,12 @@ remove_email_entry (
 }
 
 NSS_IMPLEMENT void
-nssTrustDomain_RemoveCertFromCache (
+nssTrustDomain_RemoveCertFromCacheLOCKED (
   NSSTrustDomain *td,
   NSSCertificate *cert
 )
 {
     nssList *subjectList;
-    PRStatus nssrv;
     cache_entry *ce;
     NSSArena *arena;
     NSSUTF8 *nickname;
@@ -376,37 +375,22 @@ nssTrustDomain_RemoveCertFromCache (
 #ifdef DEBUG_CACHE
     log_cert_ref("attempt to remove cert", cert);
 #endif
-    PZ_Lock(td->cache->lock);
     ce = (cache_entry *)nssHash_Lookup(td->cache->issuerAndSN, cert);
     if (!ce || ce->entry.cert != cert) {
 	/* If it's not in the cache, or a different cert is (this is really
 	 * for safety reasons, though it shouldn't happen), do nothing 
 	 */
-	PZ_Unlock(td->cache->lock);
 #ifdef DEBUG_CACHE
 	PR_LOG(s_log, PR_LOG_DEBUG, ("but it wasn't in the cache"));
 #endif
 	return;
     }
-    nssrv = remove_issuer_and_serial_entry(td->cache, cert);
-    if (nssrv != PR_SUCCESS) {
-	goto loser;
-    }
-    nssrv = remove_subject_entry(td->cache, cert, &subjectList, 
-							&nickname, &arena);
-    if (nssrv != PR_SUCCESS) {
-	goto loser;
-    }
+    (void)remove_issuer_and_serial_entry(td->cache, cert);
+    (void)remove_subject_entry(td->cache, cert, &subjectList, 
+                               &nickname, &arena);
     if (nssList_Count(subjectList) == 0) {
-	PRStatus nssrv2;
-	nssrv = remove_nickname_entry(td->cache, nickname, subjectList);
-	nssrv2 = remove_email_entry(td->cache, cert, subjectList);
-#ifndef NSS_3_4_CODE
-	/* XXX Again, 3.4 allows for certs w/o either nickname or email */
-	if (nssrv != PR_SUCCESS && nssrv2 != PR_SUCCESS) {
-	    goto loser;
-	}
-#endif
+	(void)remove_nickname_entry(td->cache, nickname, subjectList);
+	(void)remove_email_entry(td->cache, cert, subjectList);
 	(void)nssList_Destroy(subjectList);
 	nssHash_Remove(td->cache->subject, &cert->subject);
 	/* there are no entries left for this subject, free the space used
@@ -416,29 +400,22 @@ nssTrustDomain_RemoveCertFromCache (
 	    nssArena_Destroy(arena);
 	}
     }
-    PZ_Unlock(td->cache->lock);
-    NSSCertificate_Destroy(cert); /* release the reference */
-    return;
-loser:
-    /* if here, then the cache is inconsistent.  For now, flush it. */
-    PZ_Unlock(td->cache->lock);
-#ifdef DEBUG_CACHE
-    PR_LOG(s_log, PR_LOG_DEBUG, ("remove cert failed, flushing"));
-#endif
-    nssTrustDomain_FlushCache(td, -1.0);
-    NSSCertificate_Destroy(cert); /* release the reference */
 }
 
-/* This is used to remove all certs below a certain threshold, where
- * the value is determined by how many times the cert has been requested
- * and when the last request was.
- */
 NSS_IMPLEMENT void
-nssTrustDomain_FlushCache (
-  NSSTrustDomain *td,
-  PRFloat64 threshold
+nssTrustDomain_LockCertCache (
+  NSSTrustDomain *td
+)
+{
+    PZ_Lock(td->cache->lock);
+}
+
+NSS_IMPLEMENT void
+nssTrustDomain_UnlockCertCache (
+  NSSTrustDomain *td
 )
 {
+    PZ_Unlock(td->cache->lock);
 }
 
 struct token_cert_dtor {
@@ -462,7 +439,7 @@ remove_token_certs(const void *k, void *v, void *a)
 	    object->instances[i] = object->instances[object->numInstances-1];
 	    object->instances[object->numInstances-1] = NULL;
 	    object->numInstances--;
-	    dtor->certs[dtor->numCerts++] = nssCertificate_AddRef(c);
+	    dtor->certs[dtor->numCerts++] = c;
 	    if (dtor->numCerts == dtor->arrSize) {
 		dtor->arrSize *= 2;
 		dtor->certs = nss_ZREALLOCARRAY(dtor->certs, 
@@ -500,15 +477,14 @@ nssTrustDomain_RemoveTokenCertsFromCache (
     dtor.arrSize = arrSize;
     PZ_Lock(td->cache->lock);
     nssHash_Iterate(td->cache->issuerAndSN, remove_token_certs, (void *)&dtor);
-    PZ_Unlock(td->cache->lock);
     for (i=0; i<dtor.numCerts; i++) {
 	if (dtor.certs[i]->object.numInstances == 0) {
-	    nssTrustDomain_RemoveCertFromCache(td, dtor.certs[i]);
+	    nssTrustDomain_RemoveCertFromCacheLOCKED(td, dtor.certs[i]);
 	} else {
 	    STAN_ForceCERTCertificateUpdate(dtor.certs[i]);
 	}
-	nssCertificate_Destroy(dtor.certs[i]);
     }
+    PZ_Unlock(td->cache->lock);
     nss_ZFreeIf(dtor.certs);
     return PR_SUCCESS;
 }
@@ -808,7 +784,7 @@ add_cert_to_cache (
 	}
 #endif
     }
-    rvCert = nssCertificate_AddRef(cert);
+    rvCert = cert;
     PZ_Unlock(td->cache->lock);
     return rvCert;
 loser: