Bug 472975: crash when deleting user certificates, r=rrelyea

2 files changed, 56 insertions, 46 deletions

diff --git a/security/nss/lib/pk11wrap/pk11cert.c b/security/nss/lib/pk11wrap/pk11cert.c
index ff975192a..e9245d64f 100644
--- a/security/nss/lib/pk11wrap/pk11cert.c
+++ b/security/nss/lib/pk11wrap/pk11cert.c
@@ -250,17 +250,18 @@ pk11_isID0(PK11SlotInfo *slot, CK_OBJECT_HANDLE certID)
 
 /*
  * Create an NSSCertificate from a slot/certID pair, return it as a
- * CERTCertificate.
+ * CERTCertificate.  Optionally, output the nickname string.
  */
-static CERTCertificate
-*pk11_fastCert(PK11SlotInfo *slot, CK_OBJECT_HANDLE certID, 
-			CK_ATTRIBUTE *privateLabel, char **nickptr)
+static CERTCertificate *
+pk11_fastCert(PK11SlotInfo *slot, CK_OBJECT_HANDLE certID, 
+	      CK_ATTRIBUTE *privateLabel, char **nickptr)
 {
     NSSCertificate *c;
     nssCryptokiObject *co = NULL;
     nssPKIObject *pkio;
     NSSToken *token;
     NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
+    PRStatus status;
 
     /* Get the cryptoki object from the handle */
     token = PK11Slot_GetNSSToken(slot);
@@ -287,19 +288,30 @@ static CERTCertificate
 	return NULL;
     }
 
-    nssTrustDomain_AddCertsToCache(td, &c, 1);
-
-    /* Build the old-fashioned nickname */
+    /* Build and output a nickname, if desired. 
+     * This must be done before calling nssTrustDomain_AddCertsToCache
+     * because that function may destroy c, pkio and co!
+     */
     if ((nickptr) && (co->label)) {
 	CK_ATTRIBUTE label, id;
+
 	label.type = CKA_LABEL;
 	label.pValue = co->label;
 	label.ulValueLen = PORT_Strlen(co->label);
+
 	id.type = CKA_ID;
 	id.pValue = c->id.data;
 	id.ulValueLen = c->id.size;
+
 	*nickptr = pk11_buildNickname(slot, &label, privateLabel, &id);
     }
+
+    /* This function may destroy the cert in "c" and all its subordinate
+     * structures, and replace the value in "c" with the address of a 
+     * different NSSCertificate that it found in the cache.
+     * Presumably, the nickname which we just output above remains valid. :)
+     */
+    status = nssTrustDomain_AddCertsToCache(td, &c, 1);
     return STAN_GetCERTCertificateOrRelease(c);
 }
 
@@ -318,11 +330,12 @@ PK11_MakeCertFromHandle(PK11SlotInfo *slot,CK_OBJECT_HANDLE certID,
     PRBool swapNickname = PR_FALSE;
 
     cert = pk11_fastCert(slot,certID,privateLabel, &nickname);
-    if (cert == NULL) goto loser;
+    if (cert == NULL) 
+    	goto loser;
 	
     if (nickname) {
 	if (cert->nickname != NULL) {
-		cert->dbnickname = cert->nickname;
+	    cert->dbnickname = cert->nickname;
 	} 
 	cert->nickname = PORT_ArenaStrdup(cert->arena,nickname);
 	PORT_Free(nickname);
@@ -341,12 +354,11 @@ PK11_MakeCertFromHandle(PK11SlotInfo *slot,CK_OBJECT_HANDLE certID,
     }
 
     trust = (CERTCertTrust*)PORT_ArenaAlloc(cert->arena, sizeof(CERTCertTrust));
-    if (trust == NULL) goto loser;
+    if (trust == NULL) 
+    	goto loser;
     PORT_Memset(trust,0, sizeof(CERTCertTrust));
     cert->trust = trust;
 
-    
-
     if(! pk11_HandleTrustObject(slot, cert, trust) ) {
 	unsigned int type;
 
@@ -386,12 +398,13 @@ PK11_MakeCertFromHandle(PK11SlotInfo *slot,CK_OBJECT_HANDLE certID,
 	trust->emailFlags |= CERTDB_USER;
 	/*    trust->objectSigningFlags |= CERTDB_USER; */
     }
-
     return cert;
 
 loser:
-    if (nickname) PORT_Free(nickname);
-    if (cert) CERT_DestroyCertificate(cert);
+    if (nickname) 
+    	PORT_Free(nickname);
+    if (cert) 
+    	CERT_DestroyCertificate(cert);
     return NULL;
 }
 
diff --git a/security/nss/lib/pki/pkibase.c b/security/nss/lib/pki/pkibase.c
index db1672395..9d62c7713 100644
--- a/security/nss/lib/pki/pkibase.c
+++ b/security/nss/lib/pki/pkibase.c
@@ -199,48 +199,45 @@ nssPKIObject_AddInstance (
   nssCryptokiObject *instance
 )
 {
+    nssCryptokiObject **newInstances = NULL;
+
     nssPKIObject_Lock(object);
     if (object->numInstances == 0) {
-	object->instances = nss_ZNEWARRAY(object->arena,
-	                                  nssCryptokiObject *,
-	                                  object->numInstances + 1);
+	newInstances = nss_ZNEWARRAY(object->arena,
+				     nssCryptokiObject *,
+				     object->numInstances + 1);
     } else {
+	PRBool found = PR_FALSE;
 	PRUint32 i;
 	for (i=0; i<object->numInstances; i++) {
 	    if (nssCryptokiObject_Equal(object->instances[i], instance)) {
-		nssPKIObject_Unlock(object);
-		if (instance->label) {
-		    if (!object->instances[i]->label ||
-		        !nssUTF8_Equal(instance->label,
-		                       object->instances[i]->label, NULL))
-		    {
-			/* Either the old instance did not have a label,
-			 * or the label has changed.
-			 */
-			nss_ZFreeIf(object->instances[i]->label);
-			object->instances[i]->label = instance->label;
-			instance->label = NULL;
-		    }
-		} else if (object->instances[i]->label) {
-		    /* The old label was removed */
-		    nss_ZFreeIf(object->instances[i]->label);
-		    object->instances[i]->label = NULL;
-		}
-		nssCryptokiObject_Destroy(instance);
-		return PR_SUCCESS;
+		found = PR_TRUE;
+		break;
 	    }
 	}
-	object->instances = nss_ZREALLOCARRAY(object->instances,
-	                                      nssCryptokiObject *,
-	                                      object->numInstances + 1);
+	if (found) {
+	    /* The new instance is identical to one in the array, except
+	     * perhaps that the label may be different.  So replace 
+	     * the label in the array instance with the label from the 
+	     * new instance, and discard the new instance.
+	     */
+	    nss_ZFreeIf(object->instances[i]->label);
+	    object->instances[i]->label = instance->label;
+	    nssPKIObject_Unlock(object);
+	    instance->label = NULL;
+	    nssCryptokiObject_Destroy(instance);
+	    return PR_SUCCESS;
+	}
+	newInstances = nss_ZREALLOCARRAY(object->instances,
+					 nssCryptokiObject *,
+					 object->numInstances + 1);
     }
-    if (!object->instances) {
-	nssPKIObject_Unlock(object);
-	return PR_FAILURE;
+    if (newInstances) {
+	object->instances = newInstances;
+	newInstances[object->numInstances++] = instance;
     }
-    object->instances[object->numInstances++] = instance;
     nssPKIObject_Unlock(object);
-    return PR_SUCCESS;
+    return (newInstances ? PR_SUCCESS : PR_FAILURE);
 }
 
 NSS_IMPLEMENT PRBool