397832 - libpkix leaks memory if a macro calls a function that returns an error. cleanup in pkix_pl_hashtable.c. r=nelson
authoralexei.volkov.bugs%sun.com
Wed, 14 Nov 2007 19:46:39 +0000
changeset 8223 9bdc6eefd1b841fa1f18fad422df32db1aeae250
parent 8222 26b81a6794b618448a20c48cad990bca1e82a3f8
child 8225 f2aed397966bf3043910ff94c2cc9f827ae5835c
push idunknown
push userunknown
push dateunknown
reviewersnelson
bugs397832
397832 - libpkix leaks memory if a macro calls a function that returns an error. cleanup in pkix_pl_hashtable.c. r=nelson
security/nss/lib/libpkix/pkix/util/pkix_errpaths.c
security/nss/lib/libpkix/pkix/util/pkix_tools.h
security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c
security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.h
--- a/security/nss/lib/libpkix/pkix/util/pkix_errpaths.c
+++ b/security/nss/lib/libpkix/pkix/util/pkix_errpaths.c
@@ -59,17 +59,16 @@ PKIX_DoThrow(PKIX_StdVars * stdVars, PKI
     return pkixReturnResult;
 }
 
 PKIX_Error *
 PKIX_DoReturn(PKIX_StdVars * stdVars, PKIX_ERRORCLASS errClass,
               PKIX_Boolean doLogger, void *plContext)
 {
     PKIX_OBJECT_UNLOCK(lockedObject);
-    PKIX_MUTEX_UNLOCK(lockedMutex);
     if ((pkixErrorReceived) || (pkixErrorResult))
 	return PKIX_DoThrow(stdVars, errClass, pkixErrorCode, plContext);
     /* PKIX_DEBUG_EXIT(type); */
     if (doLogger)
 	_PKIX_DEBUG_TRACE(pkixLoggersDebugTrace, "<<<", PKIX_LOGGER_LEVEL_TRACE);
     return NULL;
 }
 
--- a/security/nss/lib/libpkix/pkix/util/pkix_tools.h
+++ b/security/nss/lib/libpkix/pkix/util/pkix_tools.h
@@ -89,45 +89,42 @@ typedef struct pkixStdVarsStr {
     PKIX_Error        *aPkixReturnResult;
     PKIX_ERRORCODE     aPkixErrorCode;
     const char        *aPkixErrorMsg;
     PKIX_Boolean       aPkixErrorReceived;
     PKIX_Boolean       aPkixTempErrorReceived;
     PKIX_ERRORCLASS    aPkixErrorClass;
     PKIX_UInt32        aPkixType;
     PKIX_PL_Object    *aLockedObject;
-    PKIX_PL_Mutex     *aLockedMutex;
 } PKIX_StdVars;
 
 #ifdef PKIX_STDVARS_POINTER
 #define myFuncName              stdVars->aMyFuncName
 #define pkixErrorResult		stdVars->aPkixErrorResult
 #define pkixTempResult		stdVars->aPkixTempResult
 #define pkixReturnResult	stdVars->aPkixReturnResult
 #define pkixErrorCode		stdVars->aPkixErrorCode
 #define pkixErrorMsg		stdVars->aPkixErrorMsg
 #define pkixErrorReceived	stdVars->aPkixErrorReceived
 #define pkixTempErrorReceived 	stdVars->aPkixTempErrorReceived 
 #define pkixErrorClass		stdVars->aPkixErrorClass
 #define pkixType		stdVars->aPkixType
 #define lockedObject		stdVars->aLockedObject
-#define lockedMutex		stdVars->aLockedMutex
 #else
 #define myFuncName              stdVars.aMyFuncName
 #define pkixErrorResult		stdVars.aPkixErrorResult
 #define pkixTempResult		stdVars.aPkixTempResult
 #define pkixReturnResult	stdVars.aPkixReturnResult
 #define pkixErrorCode		stdVars.aPkixErrorCode
 #define pkixErrorMsg		stdVars.aPkixErrorMsg
 #define pkixErrorReceived	stdVars.aPkixErrorReceived
 #define pkixTempErrorReceived 	stdVars.aPkixTempErrorReceived 
 #define pkixErrorClass		stdVars.aPkixErrorClass
 #define pkixType		stdVars.aPkixType
 #define lockedObject		stdVars.aLockedObject
-#define lockedMutex		stdVars.aLockedMutex
 #endif
 
 extern PKIX_Error * PKIX_DoReturn(PKIX_StdVars * stdVars, 
                                   PKIX_ERRORCLASS errClass, 
                                   PKIX_Boolean doLogger,
 				  void * plContext);
 
 extern PKIX_Error * PKIX_DoThrow(PKIX_StdVars * stdVars, 
@@ -206,28 +203,16 @@ extern const PKIX_StdVars zeroStdVars;
 		    PKIX_PL_Object_Unlock \
 		    ((PKIX_PL_Object *)(obj), plContext); \
 	    if (pkixTempResult) \
 		return pkixTempResult; \
 	    lockedObject = NULL; \
 	} \
     } while (0)
 
-#define PKIX_MUTEX_UNLOCK(mutex) \
-    do { \
-	if (mutex){ \
-	    PORT_Assert(lockedMutex == (PKIX_PL_Mutex *)(mutex)); \
-	    pkixTempResult = \
-		    PKIX_PL_Mutex_Unlock((mutex), plContext); \
-	    if (pkixTempResult) \
-		return pkixTempResult; \
-	    lockedMutex = NULL; \
-	} \
-    } while (0)
-
 #define PKIX_DECREF(obj) \
     do { \
 	if (obj){ \
 	    pkixTempResult = PKIX_PL_Object_DecRef \
 			((PKIX_PL_Object *)(obj), plContext); \
 	    if (pkixTempResult) \
 		return pkixTempResult; \
 	    obj = NULL; \
@@ -251,33 +236,31 @@ extern const PKIX_StdVars zeroStdVars;
     return PKIX_DoThrow(&stdVars, (PKIX_ ## type ## _ERROR), descNum, plContext);
 #endif
 
 
 #if defined(DEBUG) && !defined(DEBUG_nb95248)
 #define PKIX_RETURN(type) \
     { \
 	PKIX_OBJECT_UNLOCK(lockedObject); \
-	PKIX_MUTEX_UNLOCK(lockedMutex); \
 	if ((pkixErrorReceived) || (pkixErrorResult)) \
 	    PKIX_THROW(type, pkixErrorCode); \
 	PKIX_DEBUG_EXIT(type); \
 	_PKIX_DEBUG_TRACE(pkixLoggersDebugTrace, "<<<", PKIX_LOGGER_LEVEL_TRACE); \
 	return NULL; \
     }
 #else
 #define PKIX_RETURN(type) \
     return PKIX_DoReturn(&stdVars, (PKIX_ ## type ## _ERROR), PKIX_TRUE, plContext);
 #endif
 
 #if defined(DEBUG) && !defined(DEBUG_nb95248)
 #define PKIX_RETURN_NO_LOGGER(type) \
     { \
 	PKIX_OBJECT_UNLOCK(lockedObject); \
-	PKIX_MUTEX_UNLOCK(lockedMutex); \
 	if ((pkixErrorReceived) || (pkixErrorResult)) \
 	    PKIX_THROW(type, pkixErrorCode); \
 	PKIX_DEBUG_EXIT(type); \
 	return NULL; \
     }
 #else
 #define PKIX_RETURN_NO_LOGGER(type) \
     return PKIX_DoReturn(&stdVars, (PKIX_ ## type ## _ERROR), PKIX_FALSE, plContext);
@@ -396,25 +379,16 @@ extern const PKIX_StdVars zeroStdVars;
 	if (obj){ \
 	    PKIX_CHECK(PKIX_PL_Object_Lock \
 		    ((PKIX_PL_Object*)(obj), plContext), \
 		    PKIX_OBJECTLOCKFAILED); \
 	    lockedObject = (PKIX_PL_Object *)(obj); \
 	} \
     } while (0)
 
-#define PKIX_MUTEX_LOCK(obj) \
-    do { \
-	if (obj){ \
-	    PKIX_CHECK(PKIX_PL_Mutex_Lock((obj), plContext), \
-		    PKIX_MUTEXLOCKFAILED); \
-	    lockedMutex = (obj); \
-	} \
-    } while (0)
-
 #define PKIX_ERROR_CREATE(type, descNum, error) \
     { \
 	pkixTempResult = (PKIX_Error*)pkix_Throw \
 		(PKIX_ ## type ## _ERROR,  myFuncName, \
 		descNum, NULL, &error, plContext); \
 	if (pkixTempResult)  \
 	    error = pkixTempResult; \
     }
--- a/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c
+++ b/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c
@@ -38,18 +38,53 @@
  * pkix_pl_hashtable.c
  *
  * Hashtable Object Functions
  *
  */
 
 #include "pkix_pl_hashtable.h"
 
+/* --Private-Structure-------------------------------------------- */
+
+struct PKIX_PL_HashTableStruct {
+        pkix_pl_PrimHashTable *primHash;
+        PKIX_PL_Mutex *tableLock;
+        PKIX_UInt32 maxEntriesPerBucket;
+};
+
 /* --Private-Functions-------------------------------------------- */
 
+#define PKIX_MUTEX_UNLOCK(mutex) \
+    do { \
+        if (mutex && lockedMutex == (PKIX_PL_Mutex *)(mutex)) { \
+            pkixTempResult = \
+                PKIX_PL_Mutex_Unlock((mutex), plContext); \
+            PORT_Assert(pkixTempResult == NULL); \
+            if (pkixTempResult) { \
+                /* PKIX_DoAddError(&stdVars, pkixTempResult, plContext); */ \
+                pkixTempResult = NULL; \
+            } \
+            lockedMutex = NULL; \
+        } else { \
+            PORT_Assert(lockedMutex == NULL); \
+        }\
+    } while (0)
+
+
+#define PKIX_MUTEX_LOCK(mutex) \
+    do { \
+        if (mutex){ \
+            PORT_Assert(lockedMutex == NULL); \
+            PKIX_CHECK(PKIX_PL_Mutex_Lock((mutex), plContext), \
+                       PKIX_MUTEXLOCKFAILED); \
+            lockedMutex = (mutex); \
+        } \
+    } while (0)
+
 /*
  * FUNCTION: pkix_pl_HashTable_Destroy
  * (see comments for PKIX_PL_DestructorCallback in pkix_pl_system.h)
  */
 static PKIX_Error *
 pkix_pl_HashTable_Destroy(
         PKIX_PL_Object *object,
         void *plContext)
@@ -178,21 +213,21 @@ cleanup:
  */
 PKIX_Error *
 PKIX_PL_HashTable_Add(
         PKIX_PL_HashTable *ht,
         PKIX_PL_Object *key,
         PKIX_PL_Object *value,
         void *plContext)
 {
+        PKIX_PL_Mutex  *lockedMutex = NULL;
         PKIX_PL_Object *deletedKey = NULL;
         PKIX_PL_Object *deletedValue = NULL;
         PKIX_UInt32 hashCode;
         PKIX_PL_EqualsCallback keyComp;
-        PKIX_Boolean tableLocked = PKIX_FALSE;
         PKIX_UInt32 bucketSize = 0;
 
         PKIX_ENTER(HASHTABLE, "PKIX_PL_HashTable_Add");
         PKIX_NULLCHECK_THREE(ht, key, value);
 
         /* Insert into primitive hashtable */
 
         PKIX_CHECK(PKIX_PL_Object_Hashcode(key, &hashCode, plContext),
@@ -219,153 +254,141 @@ PKIX_PL_HashTable_Add(
                         (void **) &deletedValue,
                         plContext),
                         PKIX_PRIMHASHTABLEGETBUCKETSIZEFAILED);
                 PKIX_DECREF(deletedKey);
                 PKIX_DECREF(deletedValue);
         }
 
         PKIX_MUTEX_LOCK(ht->tableLock);
-        tableLocked = PKIX_TRUE;
 
         PKIX_CHECK(pkix_pl_PrimHashTable_Add
                 (ht->primHash,
                 (void *)key,
                 (void *)value,
                 hashCode,
                 keyComp,
                 plContext),
                 PKIX_PRIMHASHTABLEADDFAILED);
 
         PKIX_MUTEX_UNLOCK(ht->tableLock);
-        tableLocked = PKIX_FALSE;
 
         PKIX_INCREF(key);
         PKIX_INCREF(value);
 
         /*
          * we don't call PKIX_PL_InvalidateCache here b/c we have
          * not implemented toString or hashcode for this Object
          */
 
 cleanup:
 
-        if (PKIX_ERROR_RECEIVED && tableLocked){
-                PKIX_MUTEX_UNLOCK(ht->tableLock);
-        }
+        PKIX_MUTEX_UNLOCK(ht->tableLock);
 
         PKIX_RETURN(HASHTABLE);
 }
 
 /*
  * FUNCTION: PKIX_PL_HashTable_Remove (see comments in pkix_pl_system.h)
  */
 PKIX_Error *
 PKIX_PL_HashTable_Remove(
         PKIX_PL_HashTable *ht,
         PKIX_PL_Object *key,
         void *plContext)
 {
+        PKIX_PL_Mutex  *lockedMutex = NULL;
         PKIX_PL_Object *result = NULL;
         PKIX_UInt32 hashCode;
         PKIX_PL_EqualsCallback keyComp;
-        PKIX_Boolean tableLocked = PKIX_FALSE;
 
         PKIX_ENTER(HASHTABLE, "PKIX_PL_HashTable_Remove");
         PKIX_NULLCHECK_TWO(ht, key);
 
         PKIX_CHECK(PKIX_PL_Object_Hashcode(key, &hashCode, plContext),
                     PKIX_OBJECTHASHCODEFAILED);
 
         PKIX_CHECK(pkix_pl_Object_RetrieveEqualsCallback
                     (key, &keyComp, plContext),
                     PKIX_OBJECTRETRIEVEEQUALSCALLBACKFAILED);
 
         PKIX_MUTEX_LOCK(ht->tableLock);
-        tableLocked = PKIX_TRUE;
 
         /* Remove from primitive hashtable */
         PKIX_CHECK(pkix_pl_PrimHashTable_Remove
                 (ht->primHash,
                 (void *)key,
                 hashCode,
                 keyComp,
                 (void **)&result,
                 plContext),
                 PKIX_PRIMHASHTABLEREMOVEFAILED);
 
         PKIX_MUTEX_UNLOCK(ht->tableLock);
-        tableLocked = PKIX_FALSE;
 
         if (result != NULL) {
                 PKIX_DECREF(result);
         } else {
                 PKIX_ERROR(PKIX_ATTEMPTTOREMOVENONEXISTANTITEM);
         }
 
         /*
          * we don't call PKIX_PL_InvalidateCache here b/c we have
          * not implemented toString or hashcode for this Object
          */
 
 cleanup:
 
-        if (PKIX_ERROR_RECEIVED && tableLocked){
-                PKIX_MUTEX_UNLOCK(ht->tableLock);
-        }
+        PKIX_MUTEX_UNLOCK(ht->tableLock);
 
         PKIX_RETURN(HASHTABLE);
 }
 
 /*
  * FUNCTION: PKIX_PL_HashTable_Lookup (see comments in pkix_pl_system.h)
  */
 PKIX_Error *
 PKIX_PL_HashTable_Lookup(
         PKIX_PL_HashTable *ht,
         PKIX_PL_Object *key,
         PKIX_PL_Object **pResult,
         void *plContext)
 {
+        PKIX_PL_Mutex *lockedMutex = NULL;
         PKIX_UInt32 hashCode;
         PKIX_PL_EqualsCallback keyComp;
         PKIX_PL_Object *result = NULL;
-        PKIX_Boolean tableLocked = PKIX_FALSE;
 
         PKIX_ENTER(HASHTABLE, "PKIX_PL_HashTable_Lookup");
         PKIX_NULLCHECK_THREE(ht, key, pResult);
 
         PKIX_CHECK(PKIX_PL_Object_Hashcode(key, &hashCode, plContext),
                     PKIX_OBJECTHASHCODEFAILED);
 
         PKIX_CHECK(pkix_pl_Object_RetrieveEqualsCallback
                     (key, &keyComp, plContext),
                     PKIX_OBJECTRETRIEVEEQUALSCALLBACKFAILED);
 
         PKIX_MUTEX_LOCK(ht->tableLock);
-        tableLocked = PKIX_TRUE;
 
         /* Lookup in primitive hashtable */
         PKIX_CHECK(pkix_pl_PrimHashTable_Lookup
                 (ht->primHash,
                 (void *)key,
                 hashCode,
                 keyComp,
                 (void **)&result,
                 plContext),
                 PKIX_PRIMHASHTABLELOOKUPFAILED);
 
         PKIX_MUTEX_UNLOCK(ht->tableLock);
-        tableLocked = PKIX_FALSE;
 
         if (result != NULL) {
                 PKIX_INCREF(result);
         }
         *pResult = result;
 
 cleanup:
-
-        if (PKIX_ERROR_RECEIVED && tableLocked){
-                PKIX_MUTEX_UNLOCK(ht->tableLock);
-        }
+        
+        PKIX_MUTEX_UNLOCK(ht->tableLock);
 
         PKIX_RETURN(HASHTABLE);
 }
--- a/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.h
+++ b/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.h
@@ -45,22 +45,16 @@
 #define _PKIX_PL_HASHTABLE_H
 
 #include "pkix_pl_common.h"
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-struct PKIX_PL_HashTableStruct {
-        pkix_pl_PrimHashTable *primHash;
-        PKIX_PL_Mutex *tableLock;
-        PKIX_UInt32 maxEntriesPerBucket;
-};
-
 /* see source file for function documentation */
 
 PKIX_Error *
 pkix_pl_HashTable_RegisterSelf(void *plContext);
 
 #ifdef __cplusplus
 }
 #endif