Change handling of hash table for OSCP hashes to delete both hash key and
authorthayes%netscape.com
Thu, 06 Apr 2000 00:24:43 +0000
changeset 227 c3cafdf20df33c8bfa61e546c20b083d8f2e10d7
parent 226 9a87c11f500fdc3f8ce4b331c8df575448ea3366
child 228 755e39411d951ca545e9b31a1809d6b880d50656
push idunknown
push userunknown
push dateunknown
bugs390117
Change handling of hash table for OSCP hashes to delete both hash key and associated value in the hashtable "free entry" routine. Fixes a memory leak. (Re Netscape bug: 390117)
security/nss/lib/certdb/pcertdb.c
--- a/security/nss/lib/certdb/pcertdb.c
+++ b/security/nss/lib/certdb/pcertdb.c
@@ -5479,25 +5479,93 @@ typedef struct SPKDigestInfoStr {
 /*
  * Since the key hash information is "hidden" (in a void pointer in the handle)
  * these macros with the appropriate casts make it easy to get at the parts.
  */
 #define SPK_DIGEST_TABLE(handle)	\
 	(((SPKDigestInfo *)(handle->spkDigestInfo))->table)
 
 /*
+** Hash allocator ops for the SPKDigest hash table.  The rules are:
+**   + The index and value fields are "owned" by the hash table, and are
+**     freed when the table entry is deleted.
+**   + Replacing a value in the table is not allowed, since the caller can't
+**     tell whether the index field was used or not, resulting in a memory
+**     leak.  (This is a bug in the PL_Hash routines.
+*/
+static void * PR_CALLBACK
+spkAllocTable(void *pool, PRSize size)
+{
+#if defined(XP_MAC)
+#pragma unused (pool)
+#endif
+
+    return PR_MALLOC(size);
+}
+
+static void PR_CALLBACK
+spkFreeTable(void *pool, void *item)
+{
+#if defined(XP_MAC)
+#pragma unused (pool)
+#endif
+
+    PR_Free(item);
+}
+
+/* NOTE: the key argument here appears to be useless, since the RawAdd
+ * routine in PL_Hash just uses the original anyway.
+ */
+static PLHashEntry * PR_CALLBACK
+spkAllocEntry(void *pool, const void *key)
+{
+#if defined(XP_MAC)
+#pragma unused (pool,key)
+#endif
+
+    return PR_NEW(PLHashEntry);
+}
+
+static void PR_CALLBACK
+spkFreeEntry(void *pool, PLHashEntry *he, PRUintn flag)
+{
+#if defined(XP_MAC)
+#pragma unused (pool)
+#endif
+
+    SECItem *value = (SECItem *)he->value;
+
+    /* The flag should always be to free the whole entry.  Otherwise the
+     * index field gets leaked because the caller can't tell whether
+     * the "new" value (which is the same as the old) was used or not.
+     */
+    PORT_Assert(flag == HT_FREE_ENTRY);
+
+    /* We always free the value */
+    SECITEM_FreeItem(value, PR_TRUE);
+    
+    if (flag == HT_FREE_ENTRY)
+    {
+        /* Comes from BTOA, is this the right free call? */
+        PORT_Free((char *)he->key);
+        PR_Free(he);
+    }
+}
+
+static PLHashAllocOps spkHashAllocOps = {
+    spkAllocTable, spkFreeTable,
+    spkAllocEntry, spkFreeEntry
+};
+
+
+/*
  * Create the key hash lookup table.  Note that the table, and the
  * structure which holds it and a little more information, is never freed.
  * This is because the temporary database is never actually closed out,
- * so there is no safe/obvious place to free the whole thing.  Also,
- * every time an entry is added to the table, the key under which it is
- * added is leaked.  I think the only way to fix that is to provide our
- * own allocation functions to PL_NewHashTable.  But I'm not in the mood
- * to do that, given that all of this is about to change, and we should
- * have the key hash lookup done inside the permanent database itself.
+ * so there is no safe/obvious place to free the whole thing.
  *
  * The database must be locked already.
  */
 static SECStatus
 InitDBspkDigestInfo(CERTCertDBHandle *handle)
 {
     SPKDigestInfo *spkDigestInfo;
     PLHashTable *table;
@@ -5507,17 +5575,17 @@ InitDBspkDigestInfo(CERTCertDBHandle *ha
 
     spkDigestInfo = PORT_ZAlloc(sizeof(SPKDigestInfo));
     if ( spkDigestInfo == NULL ) {
 	return(SECFailure);
     }
 
     table = PL_NewHashTable(128, PL_HashString, PL_CompareStrings,
 			    (PLHashComparator) SECITEM_ItemsAreEqual,
-			    NULL, NULL);
+			    &spkHashAllocOps, NULL);
     if ( table == NULL ) {
 	PORT_Free(spkDigestInfo);
 	return(SECFailure);
     }
 
     spkDigestInfo->table = table;
     handle->spkDigestInfo = spkDigestInfo;
     return(SECSuccess);
@@ -5657,51 +5725,36 @@ spkDigestIndexFromCert(CERTCertificate *
  *
  * The database must be locked already.
  */
 static SECStatus
 RemoveCertFromSPKDigestTableForAlg(CERTCertDBHandle *handle,
 				   CERTCertificate *cert, SECOidTag digestAlg)
 {
     SECStatus rv = SECSuccess;
-    SECItem *certDBKey = NULL;
     char *index = NULL;
     PLHashTable *table;
 
     /* Expect to only be called if there is a table to work with. */
     PORT_Assert(handle->spkDigestInfo != NULL);
 
     table = SPK_DIGEST_TABLE(handle);
     PORT_Assert(table != NULL);
 
     index = spkDigestIndexFromCert(cert, digestAlg);
     if ( index == NULL ) {
 	rv = SECFailure;
 	goto done;
     }
 
-    /*
-     * Get hold of this value so we can free it after doing the remove.
-     */
-    certDBKey = PL_HashTableLookup(table, index);
-    if ( certDBKey == NULL ) {
+    if ( PL_HashTableRemove(table, index) != PR_TRUE ) {
 	/* not found means nothing to remove, which is fine */
-	goto done;
-    }
-
-    if ( PL_HashTableRemove(table, index) != PR_TRUE ) {
-	/* This should not actually happen, since we already did a lookup. */
-	PORT_Assert(0);
-	rv = SECFailure;
     }
 
 done:
-    if ( certDBKey != NULL ) {
-	SECITEM_FreeItem(certDBKey, PR_TRUE);
-    }
     if ( index != NULL ) {
 	PORT_Free(index);
     }
     return(rv);
 }
 
 /*
  * Remove the spk digests for the given cert from the spk digest table,
@@ -5738,17 +5791,17 @@ RemoveCertFromSPKDigestTable(CERTCertDBH
  * once per cert.  This whole thing should be done differently in the
  * new rewrite (Stan), and then the problem will go away.
  */
 static SECStatus
 AddCertToSPKDigestTableForAlg(CERTCertDBHandle *handle, CERTCertificate *cert,
 			      SECItem *certDBKey, SECOidTag digestAlg)
 {
     SECStatus rv = SECFailure;
-    SECItem *oldCertDBKey = NULL;
+    SECItem *oldCertDBKey;
     PRBool addit = PR_TRUE;
     CERTCertificate *oldCert = NULL;
     char *index = NULL;
     PLHashTable *table;
 
     /*
      * After running some testing doing key hash lookups (like using OCSP),
      * if these are never hit, they can probably be removed.
@@ -5780,25 +5833,23 @@ AddCertToSPKDigestTableForAlg(CERTCertDB
 	 * in that case the best plan I can think of is to hang onto the
 	 * most recent one.
 	 */
 	oldCert = CERT_FindCertByKey(handle, oldCertDBKey);
 	if ( oldCert != NULL ) {
 	    if ( cert == oldCert ) {
 		/* They are the same cert, so we are done. */
 		addit = PR_FALSE;
-		oldCertDBKey = NULL;		/* don't want to free it */
 	    } else if ( CERT_IsNewer(cert, oldCert) ) {
 		if ( PL_HashTableRemove(table, index) != PR_TRUE ) {
 		    goto loser;
 		}
 	    } else {
 		/* oldCert is "newer", so we are done. */
 		addit = PR_FALSE;
-		oldCertDBKey = NULL;		/* don't want to free it */
 	    }
 	}
     }
 
     if ( addit ) {
 	certDBKey = SECITEM_DupItem(certDBKey);
 	if ( certDBKey == NULL ) {
 	    goto loser;
@@ -5809,19 +5860,16 @@ AddCertToSPKDigestTableForAlg(CERTCertDB
 	    goto loser;
 	}
 	index = NULL;				/* don't want to free it */
     }
 
     rv = SECSuccess;
 
 loser:
-    if ( oldCertDBKey != NULL ) {
-	SECITEM_FreeItem(oldCertDBKey, PR_TRUE);
-    }
     if ( index != NULL ) {
 	PORT_Free(index);
     }
     if ( oldCert != NULL ) {
 	CERT_DestroyCertificate(oldCert);
     }
     return(rv);
 }