bug 124309, fix various issues with the cache/temp store and thread safety. Is one line different than the patch on the bug, the one line seemed to fix the reported crash.

security/nss/lib/pk11wrap/pk11cert.c

@@ -94,14 +94,35 @@ static PRStatus convert_and_cache_cert(NSSCertificate *c, void *arg)
     PRStatus nssrv;
     NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
     struct nss3_cert_cbstr *nss3cb = (struct nss3_cert_cbstr *)arg;
-    nssrv = convert_cert(c, arg);
+    NSSCertificate *cp = nssCertificate_AddRef(c);
-    if (nssrv == PR_SUCCESS) {
+    /* The cert coming in has been retrieved from a token.  It was not in
-	nssTrustDomain_AddCertsToCache(td, &c, 1);
+     * the cache when the search was begun.  But it may be in the cache now,
-	if (!nssList_Get(nss3cb->cached, c)) {
+     * and if it isn't, it will be, because it is going to be cracked into
-	    nssCertificate_AddRef(c);
+     * a CERTCertificate and fed into the callback.
-	    nssList_Add(nss3cb->cached, c);
+     */
-	}
+    nssrv = nssTrustDomain_AddCertsToCache(td, &c, 1);
+    if (!nssList_Get(nss3cb->cached, c)) {
+	nssCertificate_AddRef(c);
+	nssList_Add(nss3cb->cached, c);
     }
+    /* This is why the hack of copying the cert was done above.  The pointer
+     * c passed to this function is provided by retrieve_cert.  That function
+     * will destroy the pointer once this function returns.  Since c is a local
+     * copy, there is no way to notify retrieve_cert if it has changed.  That
+     * would happen if the above call to add it to the cache found the cert
+     * already there.  In that case, the pointer c passed to the callback
+     * below will be the cached cert, and the pointer c that retrieve_cert
+     * has will be the same as the copy made above.  Thus, retrieve_cert will
+     * destroy the reference to the copy, the callback will use the reference
+     * to the cached entry, and everyone should be happy.
+     */
+    if (cp == c) {
+	/* However, if the call to add c to the cache was successful, cp is
+	 * now an extra copy within this function and needs to be destroyed.
+	 */
+	NSSCertificate_Destroy(cp);
+    }
+    nssrv = convert_cert(c, arg);
     return nssrv;
 }
 

security/nss/lib/pki/certificate.c

@@ -32,7 +32,7 @@
  */
 
 #ifdef DEBUG
-static const char CVS_ID[] = "@(#) $RCSfile: certificate.c,v $ $Revision: 1.30 $ $Date: 2002/02/04 22:34:22 $ $Name:  $";
+static const char CVS_ID[] = "@(#) $RCSfile: certificate.c,v $ $Revision: 1.31 $ $Date: 2002/02/08 02:51:37 $ $Name:  $";
 #endif /* DEBUG */
 
 #ifndef NSSPKI_H
@@ -847,3 +847,18 @@ nssCertificateList_DoCallback
     return PR_SUCCESS;
 }
 
+static PRStatus add_ref_callback(NSSCertificate *c, void *a)
+{
+    nssCertificate_AddRef(c);
+    return PR_SUCCESS;
+}
+
+NSS_IMPLEMENT void
+nssCertificateList_AddReferences
+(
+  nssList *certList
+)
+{
+    (void)nssCertificateList_DoCallback(certList, add_ref_callback, NULL);
+}
+

security/nss/lib/pki/pkim.h

@@ -35,7 +35,7 @@
 #define PKIM_H
 
 #ifdef DEBUG
-static const char PKIM_CVS_ID[] = "@(#) $RCSfile: pkim.h,v $ $Revision: 1.15 $ $Date: 2002/02/01 17:25:15 $ $Name:  $";
+static const char PKIM_CVS_ID[] = "@(#) $RCSfile: pkim.h,v $ $Revision: 1.16 $ $Date: 2002/02/08 02:51:38 $ $Name:  $";
 #endif /* DEBUG */
 
 #ifndef BASE_H
@@ -289,6 +289,12 @@ nssCertificateList_DoCallback
   void *arg
 );
 
+NSS_EXTERN void
+nssCertificateList_AddReferences
+(
+  nssList *certList
+);
+
 NSS_EXTERN PRStatus
 nssPKIObject_Initialize
 (

security/nss/lib/pki/pkistore.c

@@ -32,7 +32,7 @@
  */
 
 #ifdef DEBUG
-static const char CVS_ID[] = "@(#) $RCSfile: pkistore.c,v $ $Revision: 1.7 $ $Date: 2002/02/06 23:11:08 $ $Name:  $";
+static const char CVS_ID[] = "@(#) $RCSfile: pkistore.c,v $ $Revision: 1.8 $ $Date: 2002/02/08 02:51:38 $ $Name:  $";
 #endif /* DEBUG */
 
 #ifndef PKIM_H
@@ -242,6 +242,10 @@ nssCertificateStore_Add
 {
     PRStatus nssrv;
     PZ_Lock(store->lock);
+    if (nssHash_Exists(store->issuer_and_serial, cert)) {
+	PZ_Unlock(store->lock);
+	return PR_SUCCESS;
+    }
     nssrv = add_certificate_entry(store, cert);
     if (nssrv == PR_SUCCESS) {
 	nssrv = add_subject_entry(store, cert);
@@ -312,8 +316,11 @@ nssCertificateStore_Remove
   NSSCertificate *cert
 )
 {
+    certificate_hash_entry *entry;
     PZ_Lock(store->lock);
-    if (nssHash_Exists(store->issuer_and_serial, cert)) {
+    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 */
@@ -331,11 +338,15 @@ nssCertificateStore_FindCertificatesBySubject
   NSSArena *arenaOpt
 )
 {
-    PRUint32 i, count;
+    PRUint32 count;
     NSSCertificate **rvArray = NULL;
     nssList *subjectList;
     PZ_Lock(store->lock);
     subjectList = (nssList *)nssHash_Lookup(store->subject, subject);
+    if (subjectList) {
+	/* get references before leaving the store's lock protection */
+	nssCertificateList_AddReferences(subjectList);
+    }
     PZ_Unlock(store->lock);
     if (subjectList) {
 	count = nssList_Count(subjectList);
@@ -344,14 +355,12 @@ nssCertificateStore_FindCertificatesBySubject
 	}
 	if (rvOpt) {
 	    nssList_GetArray(subjectList, (void **)rvOpt, count);
-	    for (i=0; i<count; i++) nssCertificate_AddRef(rvOpt[i]);
 	} else {
 	    rvArray = nss_ZNEWARRAY(arenaOpt, NSSCertificate *, count + 1);
 	    if (!rvArray) {
 		return (NSSCertificate **)NULL;
 	    }
 	    nssList_GetArray(subjectList, (void **)rvArray, count);
-	    for (i=0; i<count; i++) nssCertificate_AddRef(rvArray[i]);
 	}
     }
     return rvArray;
@@ -401,13 +410,17 @@ nssCertificateStore_FindCertificatesByNickname
   NSSArena *arenaOpt
 )
 {
-    PRUint32 i, count;
+    PRUint32 count;
     NSSCertificate **rvArray = NULL;
     struct nickname_template_str nt;
     nt.nickname = nickname;
     nt.subjectList = NULL;
     PZ_Lock(store->lock);
     nssHash_Iterate(store->subject, match_nickname, &nt);
+    if (nt.subjectList) {
+	/* get references before leaving the store's lock protection */
+	nssCertificateList_AddReferences(nt.subjectList);
+    }
     PZ_Unlock(store->lock);
     if (nt.subjectList) {
 	count = nssList_Count(nt.subjectList);
@@ -416,14 +429,12 @@ nssCertificateStore_FindCertificatesByNickname
 	}
 	if (rvOpt) {
 	    nssList_GetArray(nt.subjectList, (void **)rvOpt, count);
-	    for (i=0; i<count; i++) nssCertificate_AddRef(rvOpt[i]);
 	} else {
 	    rvArray = nss_ZNEWARRAY(arenaOpt, NSSCertificate *, count + 1);
 	    if (!rvArray) {
 		return (NSSCertificate **)NULL;
 	    }
 	    nssList_GetArray(nt.subjectList, (void **)rvArray, count);
-	    for (i=0; i<count; i++) nssCertificate_AddRef(rvArray[i]);
 	}
     }
     return rvArray;
@@ -472,7 +483,7 @@ nssCertificateStore_FindCertificatesByEmail
   NSSArena *arenaOpt
 )
 {
-    PRUint32 i, count;
+    PRUint32 count;
     NSSCertificate **rvArray = NULL;
     struct email_template_str et;
     et.email = email;
@@ -482,6 +493,10 @@ nssCertificateStore_FindCertificatesByEmail
     }
     PZ_Lock(store->lock);
     nssHash_Iterate(store->subject, match_email, &et);
+    if (et.emailList) {
+	/* get references before leaving the store's lock protection */
+	nssCertificateList_AddReferences(et.emailList);
+    }
     PZ_Unlock(store->lock);
     if (et.emailList) {
 	count = nssList_Count(et.emailList);
@@ -490,12 +505,10 @@ nssCertificateStore_FindCertificatesByEmail
 	}
 	if (rvOpt) {
 	    nssList_GetArray(et.emailList, (void **)rvOpt, count);
-	    for (i=0; i<count; i++) nssCertificate_AddRef(rvOpt[i]);
 	} else {
 	    rvArray = nss_ZNEWARRAY(arenaOpt, NSSCertificate *, count + 1);
 	    if (rvArray) {
 		nssList_GetArray(et.emailList, (void **)rvArray, count);
-		for (i=0; i<count; i++) nssCertificate_AddRef(rvArray[i]);
 	    }
 	}
 	nssList_Destroy(et.emailList);
@@ -513,16 +526,17 @@ nssCertificateStore_FindCertificateByIssuerAndSerialNumber
 {
     certificate_hash_entry *entry;
     NSSCertificate index;
+    NSSCertificate *rvCert = NULL;
     index.issuer = *issuer;
     index.serial = *serial;
     PZ_Lock(store->lock);
     entry = (certificate_hash_entry *)
                            nssHash_Lookup(store->issuer_and_serial, &index);
-    PZ_Unlock(store->lock);
     if (entry) {
-	return nssCertificate_AddRef(entry->cert);
+	rvCert = nssCertificate_AddRef(entry->cert);
     }
-    return NULL;
+    PZ_Unlock(store->lock);
+    return rvCert;
 }
 
 /* XXX Get this to use issuer/serial! */
@@ -562,12 +576,16 @@ nssCertificateStore_FindCertificateByEncodedCertificate
 )
 {
     struct der_template_str der;
+    NSSCertificate *rvCert = NULL;
     der.encoding = encoding;
     der.cert = NULL;
     PZ_Lock(store->lock);
     nssHash_Iterate(store->subject, match_encoding, &der);
+    if (der.cert) {
+	rvCert = nssCertificate_AddRef(der.cert);
+    }
     PZ_Unlock(store->lock);
-    return nssCertificate_AddRef(der.cert);
+    return rvCert;
 }
 
 NSS_EXTERN PRStatus

security/nss/lib/pki/tdcache.c

@@ -32,7 +32,7 @@
  */
 
 #ifdef DEBUG
-static const char CVS_ID[] = "@(#) $RCSfile: tdcache.c,v $ $Revision: 1.24 $ $Date: 2002/02/05 23:55:43 $ $Name:  $";
+static const char CVS_ID[] = "@(#) $RCSfile: tdcache.c,v $ $Revision: 1.25 $ $Date: 2002/02/08 02:51:38 $ $Name:  $";
 #endif /* DEBUG */
 
 #ifndef PKIM_H
@@ -724,8 +724,8 @@ add_cert_to_cache
 	}
 #endif
     }
-    PZ_Unlock(td->cache->lock);
     nssCertificate_AddRef(cert);
+    PZ_Unlock(td->cache->lock);
     return nssrv;
 loser:
     /* Remove any handles that have been created */
@@ -796,13 +796,13 @@ collect_subject_certs
 {
     NSSCertificate *c;
     NSSCertificate **rvArray = NULL;
-    PRUint32 i, count;
+    PRUint32 count;
     if (rvCertListOpt) {
 	nssListIterator *iter = nssList_CreateIterator(subjectList);
 	for (c  = (NSSCertificate *)nssListIterator_Start(iter);
 	     c != (NSSCertificate *)NULL;
 	     c  = (NSSCertificate *)nssListIterator_Next(iter)) {
-	    nssList_Add(rvCertListOpt, nssCertificate_AddRef(c));
+	    nssList_Add(rvCertListOpt, c);
 	}
 	nssListIterator_Finish(iter);
 	nssListIterator_Destroy(iter);
@@ -813,7 +813,6 @@ collect_subject_certs
 	    return (NSSCertificate **)NULL;
 	}
 	nssList_GetArray(subjectList, (void **)rvArray, count);
-	for (i=0; i<count; i++) nssCertificate_AddRef(rvArray[i]);
     }
     return rvArray;
 }
@@ -842,6 +841,8 @@ nssTrustDomain_GetCertsForSubjectFromCache
 #ifdef DEBUG_CACHE
 	PR_LOG(s_log, PR_LOG_DEBUG, ("... found, %d hits", ce->hits));
 #endif
+	/* get references before leaving the cache's lock protection */
+	nssCertificateList_AddReferences(ce->entry.list);
     }
     PZ_Unlock(td->cache->lock);
     if (ce) {
@@ -874,6 +875,8 @@ nssTrustDomain_GetCertsForNicknameFromCache
 #ifdef DEBUG_CACHE
 	PR_LOG(s_log, PR_LOG_DEBUG, ("... found, %d hits", ce->hits));
 #endif
+	/* get references before leaving the cache's lock protection */
+	nssCertificateList_AddReferences(ce->entry.list);
     }
     PZ_Unlock(td->cache->lock);
     if (ce) {
@@ -896,6 +899,8 @@ nssTrustDomain_GetCertsForEmailAddressFromCache
     NSSCertificate **rvArray = NULL;
     cache_entry *ce;
     nssList *collectList = NULL;
+    nssListIterator *iter = NULL;
+    nssList *subjectList;
 #ifdef DEBUG_CACHE
     PR_LOG(s_log, PR_LOG_DEBUG, ("looking for cert by email %s", email));
 #endif
@@ -907,17 +912,23 @@ nssTrustDomain_GetCertsForEmailAddressFromCache
 #ifdef DEBUG_CACHE
 	PR_LOG(s_log, PR_LOG_DEBUG, ("... found, %d hits", ce->hits));
 #endif
+	/* loop over subject lists and get refs for certs */
+	iter = nssList_CreateIterator(ce->entry.list);
+	for (subjectList  = (nssList *)nssListIterator_Start(iter);
+	     subjectList != (nssList *)NULL;
+	     subjectList  = (nssList *)nssListIterator_Next(iter)) 
+	{
+	    /* get references before leaving the cache's lock protection */
+	    nssCertificateList_AddReferences(subjectList);
+	}
     }
     PZ_Unlock(td->cache->lock);
     if (ce) {
-	nssListIterator *iter;
-	nssList *subjectList;
 	if (certListOpt) {
 	    collectList = certListOpt;
 	} else {
 	    collectList = nssList_Create(NULL, PR_FALSE);
 	}
-	iter = nssList_CreateIterator(ce->entry.list);
 	for (subjectList  = (nssList *)nssListIterator_Start(iter);
 	     subjectList != (nssList *)NULL;
 	     subjectList  = (nssList *)nssListIterator_Next(iter)) {
@@ -964,13 +975,13 @@ nssTrustDomain_GetCertForIssuerAndSNFromCache
     if (ce) {
 	ce->hits++;
 	ce->lastHit = PR_Now();
-	rvCert = ce->entry.cert;
+	rvCert = nssCertificate_AddRef(ce->entry.cert);
 #ifdef DEBUG_CACHE
 	PR_LOG(s_log, PR_LOG_DEBUG, ("... found, %d hits", ce->hits));
 #endif
     }
     PZ_Unlock(td->cache->lock);
-    return nssCertificate_AddRef(rvCert);
+    return rvCert;
 }
 
 #ifdef NSS_3_4_CODE
@@ -1030,7 +1041,7 @@ nssTrustDomain_GetCertByDERFromCache
     PORT_Free(issuer.data);
     PORT_Free(serial.data);
 #endif
-    return nssCertificate_AddRef(rvCert);
+    return rvCert;
 }
 
 static void cert_iter(const void *k, void *v, void *a)

security/nss/lib/pki/trustdomain.c

@@ -32,7 +32,7 @@
  */
 
 #ifdef DEBUG
-static const char CVS_ID[] = "@(#) $RCSfile: trustdomain.c,v $ $Revision: 1.31 $ $Date: 2002/02/01 17:25:15 $ $Name:  $";
+static const char CVS_ID[] = "@(#) $RCSfile: trustdomain.c,v $ $Revision: 1.32 $ $Date: 2002/02/08 02:51:38 $ $Name:  $";
 #endif /* DEBUG */
 
 #ifndef NSSPKI_H
@@ -903,15 +903,37 @@ static PRStatus traverse_callback(NSSCertificate *c, void *arg)
 {
     PRStatus nssrv;
     struct traverse_arg *ta = (struct traverse_arg *)arg;
-    nssrv = (*ta->callback)(c, ta->arg);
+    NSSCertificate *cp = nssCertificate_AddRef(c);
-    if (nssrv == PR_SUCCESS) {
+    NSSTrustDomain *td = NSSCertificate_GetTrustDomain(c);
-	NSSTrustDomain *td = NSSCertificate_GetTrustDomain(c);
+    /* The cert coming in has been retrieved from a token.  It was not in
-	nssTrustDomain_AddCertsToCache(td, &c, 1);
+     * the cache when the search was begun.  But it may be in the cache now,
-	if (!nssList_Get(ta->cached, c)) {
+     * and if it isn't, it will be, because it is going to be cracked into
-	    nssCertificate_AddRef(c);
+     * a CERTCertificate and fed into the callback.
-	    nssList_Add(ta->cached, c);
+     */
-	}
+    nssrv = nssTrustDomain_AddCertsToCache(td, &c, 1);
+    if (!nssList_Get(ta->cached, c)) {
+	/* add it to the cached list for this search */
+	nssCertificate_AddRef(c);
+	nssList_Add(ta->cached, c);
     }
+    /* This is why the hack of copying the cert was done above.  The pointer
+     * c passed to this function is provided by retrieve_cert.  That function
+     * will destroy the pointer once this function returns.  Since c is a local
+     * copy, there is no way to notify retrieve_cert if it has changed.  That
+     * would happen if the above call to add it to the cache found the cert
+     * already there.  In that case, the pointer c passed to the callback
+     * below will be the cached cert, and the pointer c that retrieve_cert
+     * has will be the same as the copy made above.  Thus, retrieve_cert will
+     * destroy the reference to the copy, the callback will use the reference
+     * to the cached entry, and everyone should be happy.
+     */
+    if (cp == c) {
+	/* However, if the call to add c to the cache was successful, cp is
+	 * now an extra copy within this function and needs to be destroyed.
+	 */
+	NSSCertificate_Destroy(cp);
+    }
+    nssrv = (*ta->callback)(c, ta->arg);
     return nssrv;
 }