Closed Bug 391457 Opened 18 years ago Closed 18 years ago

libpkix does not check for object ref leak at shutdown

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alvolkov.bgs, Assigned: alvolkov.bgs)

Details

(Whiteboard: PKIX)

Attachments

(2 files, 2 obsolete files)

almost every type of objects X in libpkix has associated with it information located in a block of memory equals to the size of PKIX_PL_Object that precedes object X. References to an object X are controlled by PKIX_INCRED and PKIX_DECRED macros and object ref counter which is the member of PKIX_PL_Object. I didn't find a code that would check for ref leaks which seems to me quite logical thing to do.
Blocks: 390888
Whiteboard: PKIX
Priority: -- → P1
Question for all recipients of this comment: Does anyone know why PKIX object structures aren't all defined as containing the basic PKIX_PL_Object structure as the first member? Does anyone know why we have to play pointer arithmetic games to covert a PKIX_PL_Object into one of its subclasses, or to convert a subclass object into the superclass? This seems like an insanely bad idea (having to do pointer arithmetic), and I expect it will be the cause of many bugs in years to come. Consider a project proposal to change libPKIX to make all subclass object structures have the superclass object structure as the first member, so that the SAME pointer value that points to a subclass object also points to the superclass object, and only casting (no arithmetic) is needed to covert from one to the other. Is such a proposal too much work to contemplate for 3.12? Would it be a day, a week, a month? months? If we don't do it before we release 3.12, will we ever have a chance to do it in a later release? Or will backward compatibility compel us to keep it that way for years?
Assignee: nobody → alexei.volkov.bugs
Version: 3.12 → trunk
No longer blocks: 390888
Attached patch Patch v1 (obsolete) — Splinter Review
Initial code update with feature that will allow to identify libpkix object leaks. Test result with current patch shows that some libpkix tests have memory leaks, but all.sh(with libpkix tests excluded) tests passed without any leaks. I also notice, that libpkix has race conditions: libpkix_buildthreads (multi threaded libpkix test) periodically fails with positive object counts.
Attachment #282453 - Flags: review?(nelson)
Attachment #282453 - Attachment is patch: true
Attachment #282453 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 282453 [details] [diff] [review] Patch v1 1. This patch contains a one-line fix for a different bug. I am referring to the one line addition to the macro PKIX_ERROR_FATAL, in lib/libpkix/pkix/util/pkix_tools.h Please attach that tiny patch to that bug, and request review. I'd like that fix to get checked in separately from this huge patch. Please do NOT commit that change as part of this huge patch. 2. The patch adds a single line to many files. The single line is: + entry.objCounter = 0; and the patch adds a single line to struct pkix_ClassTable_EntryStruct in nss/lib/libpkix/pkix/util/pkix_tools.h + pkixErrorCode = descNum; \ r+=nelson for those changes (but be careful to separate the one line addition to struct pkix_ClassTable_EntryStruct from the one line addition to macro PKIX_ERROR_FATAL in lib/libpkix/pkix/util/pkix_tools.h). 3. r+=nelson for the added declaration of FUNCTION: PKIX_Error_GetErrorCode to file nss/lib/libpkix/include/pkix_util.h The changes described in items 2 and 3 above can be checked in now. In the next comment, I will review the parts of this patch that change these files: nss/lib/libpkix/include/pkix_errorstrings.h nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c
Comment on attachment 282453 [details] [diff] [review] Patch v1 In comment 3, I gave r+ for about 80% of this patch. This comment applies to the parts in nss/lib/libpkix/include/pkix_errorstrings.h nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c 1. in function pkix_pl_lifecycle_ObjectLeakCheck, in DEBUG builds only, I'd like to see this function output (to stderr, or to a file named in an environment variable, if present) one line for each class of object that had leaked instances. It should say something like Class NAMENAMENAMENAME leaked NNN objects of size DDDDD bytes, total = DDDDD bytes I realize that this will require having access to an array of class names. I think we should have one already, but if not, it should be possible to create one trivially using the existing macros. It would also be good to print out a total number of leaked objects of all known types. 2. ifdef out all the "user defined class" stuff. 3. This patch gets rid of SOME (but not all) references to a variable named refCountTotal. It should eliminate all references and the definition of that variable. 4. Function PKIX_PL_Object_DecRef has the following erroneous code: > refCountError = (objectHeader->references == 0); > > if (!refCountError) { > objectHeader->references--; This is errneous for several reasons, including: a) the decrement is not atomic b) the value tested for zero is not guaranteed to be the same value either immediately before or immediately after the decrement c) the possibility of a negative reference count value is not considered. This code should ONLY use PR_Atomic functions to manipulate the value and the test should be done on a value that is guaranteed to be the same as the value of the ref counter immediately before, or immediately after the decrement. The function PR_AtomicDecrement will perform an atomic decrement, and will return a number that is guaranteed to be the decremented value (the result of the decrement). The code should use that function, and test the result for being less than zero to detect an error. The code to increment the reference count should likewise use PR_AtomicIncrement and should test the result, rather than testing the value of the variable non-atomically. 5. Function PKIX_PL_Object_DecRef locks the object, 819 PKIX_CHECK(pkix_LockObject(object, plContext), 820 PKIX_ERRORLOCKINGOBJECT); then calls pkix_pl_Object_Destroy, which unlocks the object 190 PKIX_CHECK(pkix_UnlockObject(object, plContext), 191 PKIX_ERRORUNLOCKINGOBJECT); then destroys the lock and frees it. That locking and unlocking shown above is unnecessary and arguably wrong. Please remove it.
Attachment #282453 - Flags: review?(nelson) → review-
> 1. in function pkix_pl_lifecycle_ObjectLeakCheck, in DEBUG builds only, > I'd like to see this function output (to stderr, or to a file named in > an environment variable, if present) one line for each class of object > that had leaked instances. It should say something like > > Class NAMENAMENAMENAME leaked NNN objects of size DDDDD bytes, total = DDDDD > bytes > > I realize that this will require having access to an array of class names. > I think we should have one already, but if not, it should be possible to > create one trivially using the existing macros. > > It would also be good to print out a total number of leaked objects of > all known types. libpkix already has a structure one member of which is a string(a char*) that has class name. I'm going to use it to cast from class type number to class type description. > > 2. ifdef out all the "user defined class" stuff. There are quite a few places where ifdef should occurred. I prefer to do it in a separate patch. For that I've filed a bug 397825 for that matter. > 4. Function PKIX_PL_Object_DecRef has the following erroneous code: > > > refCountError = (objectHeader->references == 0); > > > > if (!refCountError) { > > objectHeader->references--; > > This is errneous for several reasons, including: > a) the decrement is not atomic > b) the value tested for zero is not guaranteed to be the same value > either immediately before or immediately after the decrement > c) the possibility of a negative reference count value is not considered. > > This code should ONLY use PR_Atomic functions to manipulate the value > and the test should be done on a value that is guaranteed to be the > same as the value of the ref counter immediately before, or immediately > after the decrement. The function PR_AtomicDecrement will perform an > atomic decrement, and will return a number that is guaranteed to be > the decremented value (the result of the decrement). The code should > use that function, and test the result for being less than zero to > detect an error. > > The code to increment the reference count should likewise use > PR_AtomicIncrement and should test the result, rather than testing > the value of the variable non-atomically. > > 5. Function PKIX_PL_Object_DecRef locks the object, > > 819 PKIX_CHECK(pkix_LockObject(object, plContext), > 820 PKIX_ERRORLOCKINGOBJECT); > > then calls pkix_pl_Object_Destroy, which unlocks the object > > 190 PKIX_CHECK(pkix_UnlockObject(object, plContext), > 191 PKIX_ERRORUNLOCKINGOBJECT); > > then destroys the lock and frees it. That locking and unlocking > shown above is unnecessary and arguably wrong. Please remove it. > Bug 397830 is filed to take care of issues described as items 4 and 5. The fix will be in a separate patch.
> Class NAMENAMENAMENAME leaked NNN objects of size DDDDD bytes, total = DDDDD > bytes Although size of leaks is nice to have, it is not an absolute MUST have. At this time we would no be able to tell the size of objects since we do not have static object size information. The size of object that libpkix allocated gets defined only at the time PKIX_PL_Object_Alloc is called. PKIX_PL_Object_Alloc( PKIX_TYPENUM type, PKIX_UInt32 size, PKIX_PL_Object **pObject, void *plContext)
I disagree that the size information is merely nice-to-have. There are several simple ways it can be done, including (not limited to) a) make PKIX_PL_Object_Alloc store the size in the object header, or b) store the size in the array of information known about object types (the same array in which the object type name string is).
This (In reply to comment #7) > I disagree that the size information is merely nice-to-have. Well, IMO the count of the objects is the primary objectives, that directly corresponds to the amount of memory, that was leaked. From the code that saw, I've concluded, that size is directly correlates with objects types (I didn't find any tricky memory allocations). And so knowing the size of the structure will provide an info about the number of byte that been leaked. > There are several simple ways it can be done, including (not limited to) > a) make PKIX_PL_Object_Alloc store the size in the object header, or It is simple to do, but since we don't have connections between system class array and the allocated objects, the saved information would be useless for leak detector function from pkix_pl_lifecycle.c . > b) store the size in the array of information known about object types > (the same array in which the object type name string is). This is better solution, but will require synchronization - a compromise. We will need to have looks for it. I suggest that in the long run we get rid of size parameter in Object_Alloc and use size info stored in systemClass info array. I believe it is doable, because of two reasons: * size of object and its type have 1:1 correlation ship; * Object_Alloc is not use for any other operations. So, IMO the way to go is to create a static table of sizes and use it for object allocation.
> So, IMO the way to go is to create a static table of sizes and use it for ^^^^^^ constant global table
libPKIX is designed so that that information about the different classes of objects is dynamically registered at startup time. I think we should preserve that design. So, I'd say that the size of the object should be part of the object class registration. It can be added to the table of registered object classes along with their names. Object_Alloc can (initially) ASSERT that the size it receives as an argument matches the registered size. Once we know that that assertion never fails, then we can eliminate the size argument from Object_Alloc. I think that, today, we developers are mostly unfamiliar with the different libPKIX objects. We don't know all their names, much less their sizes. And I suspect that their sizes differ a lot. Some are small and others may be big. So knowing that we leaked (say) 50 objects does not tell us much about the sizes. A leak of 50 big objects needs to be fixed more urgently than a leak of 50 tiny ones. I expect there will be some big surprises about object sizes when they are reported by this facility.
Attached patch Patch v2 (obsolete) — Splinter Review
More code cleanup. Added assertion to check if requested size for allocation is equal to size of the object. This is done to remove size from the list of argument of PKIX_PL_Object_Alloc. Added logging(through PR_LOG) number of leaked object, and total leaked size of memory per object type.
Attachment #282453 - Attachment is obsolete: true
Attachment #283440 - Flags: review?(nelson)
Lexei, Your monster patch, "Patch v2", is really a number of separate, independent, patches all combined into one. I don't want you to check them all in in one big monster checkin. I want you to check them in as if they were separate patches, with separate checkin comments. In this review comment, I'm going to give r+ to a several SUBSETs of that big patch that I want you to commit separately. PLEASE don't combine separate independent patches into one monster patch. These should be separate checkins: 1. lib/libpkix/include/pkix_util.h You created a public declaration for a formerly undeclared function. Nothing depends on it, and it depends on nothing. Please check it in all by itself. Please don't ask me to review that part of this patch again. If you need to file a separate bug about that, do so. 2. Your patch to lib/libpkix/pkix/util/pkix_tools.h includes a one-line change to #define PKIX_ERROR_FATAL(descNum) I'm sure I gave that r+ before. There is a separate bug for that problem. Please check that one line patch in separately, and the checkin comment should say it is a patch for the bug about that problem. Please do NOT check it in as part of the big patch for THIS bug. 3. The patch for lib/libpkix/pkix/util/pkix_tools.h also adds two members to struct pkix_ClassTable_EntryStruct. That is clearly tied to the MANY files where the only change was to add two lines to them, lines like this: entry.description = "CertSelector"; + entry.objCounter = 0; + entry.typeObjectSize = sizeof(PKIX_CertSelector); Please checkin that change to struct pkix_ClassTable_EntryStruct, and the changes to all the files where the ONLY change was the addition of those two lines. Check those in as a single checkin. Do not check in any other changes together with those. r+ for those 3 separate checkin patch sets above. When you have checked in the above 3 separate patch sets, you will still have the following files from this patch not yet checked in: lib/nss/nssinit.c (This is already checked in) lib/libpkix/config.mk lib/libpkix/include/pkix_pl_system.h lib/libpkix/include/pkixt.h lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h I'll review the patch to those ohter 6 files separately, next.
OK, now before I review the patches to those 6 other files, I need to make a comment about all the patches that add 2 lines to the "RegisterSelf" functions for all the classes. Every one of those functions is inefficient and bloated. Every one of those does these steps: a) allocate a local, "automatic" uninitialized, struct on the stack. b) initialize it with run time code (which is slower and bigger than using a pre-initialized static constant struct.) c) copy it to an identical struct that is in an array. It would be much more efficient if each one of those had a static const structure, initialized by the compiler, and copied that. The MOST efficient thing might be for each RegisterSelf function to have a static non-const structure, intialized by the compiler (not at run time), and have the array be an array of pointers to those structs, not an array of those structs, then have each function set the address of its static function into the array of pointers. But that might not do the right thing if NSS was Initiallized, Shutdown, and reinitialized several times. The static const struct that is copied would be a good compromise. But it's purely a bloat-reduction + efficiency thing, so it can wait.
Sorry, in comment 12, I mean Alexei, not Lexei.
The patch to nss/lib/libpkix/include/pkixt.h removes TYPEMACRO(HTTPCLIENT), \ from the PKIX_TYPES macro, but leaves ERRMACRO(HTTPCLIENT), \ in the PKIX_ERRORCLASSES macro. Why? Should it be removed from both? Is there any reason to keep it in the ERRORCLASSES if it's not a PKIX_TYPE? Should we merge the two macros PKIX_TYPES and PKIX_ERRORCLASSES into one? We should sort both of them into alphabetical order, so that we can spot names in one list but not in the other more quickly.
Comment on attachment 283440 [details] [diff] [review] Patch v2 In comment 12 above, I gave r+ to several parts of this patch. They can be, and should be checked in now. Here I am continuing the review from comment 12... 4. Changing the type of the first argument of PKIX_PL_Object_Alloc nss/lib/libpkix/include/pkix_pl_system.h - the only change nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c - one line change If those changes were in a separate patch, I'd give it r+ 5. Removing all the copies of this declaration extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; from nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c and putting one copy of that declaration into nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h If this was a separate patch, I'd give it r+ 6. Adding function pkix_pl_Object_RegisterSelf(void *plContext) to nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c and adding a declaration of it into nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h If this was a separate patch, I'd give it r+ 7. New PRLogModuleInfo *pkixLog is leaked. It is never destroyed. r- Also since it's global, it doesn't need to be explicitly initialized to NULL. 8. All operations on objectHeader->references are still done non-atomically. Please do all operations on objectHeader->references atomically, as discussed in comment 4, item 4. We may yet get rid of the object lock. 9. pkix_pl_lifecycle_ObjectLeakCheck() This function is defined unconditionally, not inside an ifdef. It has one caller, that is inside #ifdef DEBUG. When DEBUG is not defined, this static function is an orphan, and will generate warnings. r- I suggest that you call it unconditionally, and only ASSERT that the result is zero if NSS_STRICT_SHUTDOWN is in the environment. 10. instead of #ifdef DEBUG_PKIX, I suggest that you test an environment variable and only call PR_NewLogModule if that variable is defined. Then, after that, use a run-time check of pkixLog, rather than a compile time check of DEBUG_PKIX to decide whether or not to take actions that only are useful if we're logging. This is OK even for non-DEBUG builds.
Attachment #283440 - Flags: review?(nelson) → review-
files approved in comment #12 have been integrated.
Attached patch Patch v3Splinter Review
Implementation of suggested changes.
Attachment #283440 - Attachment is obsolete: true
Attachment #284379 - Flags: review?(nelson)
Comment on attachment 284379 [details] [diff] [review] Patch v3 r=nelson. I request some additional changes. 1. In new function pkix_pl_lifecycle_ObjectLeakCheck() the expression systemClasses[typeCounter] appears 7 times in a single loop, indexing the array 7 times with the same index value. Please use a pointer, and compute its value once at the beginning of the loop. Better yet, initialize the pointer before the loop, and just increment the pointer during the loop. 2. In function PKIX_PL_Object_IncRef, instead of PKIX_Boolean refCountError = PKIX_FALSE; and refCountError = (PR_AtomicIncrement(&objectHeader->references) <= 1); if (refCountError) { Please use something like PKIX_Int32 refCount; refCount = PR_AtomicIncrement(&objectHeader->references); if (refCount <= 1) { 3. I'm not happy about the code for the so-called "user-defined types", but I believe you're planning to remove all the code for those types soon, so I'll just ignore it for now. > PKIX_Error * > PKIX_PL_Object_Alloc( >- PKIX_UInt32 type, >+ PKIX_TYPENUM type, > PKIX_UInt32 size, > PKIX_PL_Object **pObject, > void *plContext); > > /* > * FUNCTION: PKIX_PL_Object_IsTypeRegistered > * DESCRIPTION: > * > * Checks whether "type" has been registered by a previous call to > * PKIX_PL_Object_RegisterType() and stores the Boolean result at "pBool". >Index: lib/libpkix/include/pkixt.h >=================================================================== >RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/include/pkixt.h,v >retrieving revision 1.7 >diff -U 10 -p -r1.7 pkixt.h >--- lib/libpkix/include/pkixt.h 25 Sep 2007 00:48:08 -0000 1.7 >+++ lib/libpkix/include/pkixt.h 10 Oct 2007 22:31:43 -0000 >@@ -201,21 +201,20 @@ typedef int PKIX_Boolean; > TYPEMACRO(ERROR), \ > TYPEMACRO(HASHTABLE), \ > TYPEMACRO(LIST), \ > TYPEMACRO(LOGGER), \ > TYPEMACRO(MUTEX), \ > TYPEMACRO(OID), \ > TYPEMACRO(RWLOCK), \ > TYPEMACRO(STRING), \ > TYPEMACRO(CERTBASICCONSTRAINTS), \ > TYPEMACRO(CERT), \ >- TYPEMACRO(HTTPCLIENT), \ > TYPEMACRO(CRL), \ > TYPEMACRO(CRLENTRY), \ > TYPEMACRO(DATE), \ > TYPEMACRO(GENERALNAME), \ > TYPEMACRO(CERTNAMECONSTRAINTS), \ > TYPEMACRO(PUBLICKEY), \ > TYPEMACRO(TRUSTANCHOR), \ > TYPEMACRO(X500NAME), \ > TYPEMACRO(HTTPCERTSTORECONTEXT), \ > TYPEMACRO(BUILDRESULT), \ >@@ -292,21 +291,20 @@ typedef enum { /* Now invoke all tho > ERRMACRO(BYTEARRAY), \ > ERRMACRO(BIGINT), \ > ERRMACRO(HASHTABLE), \ > ERRMACRO(CERT), \ > ERRMACRO(X500NAME), \ > ERRMACRO(GENERALNAME), \ > ERRMACRO(PUBLICKEY), \ > ERRMACRO(DATE), \ > ERRMACRO(TRUSTANCHOR), \ > ERRMACRO(PROCESSINGPARAMS), \ >- ERRMACRO(HTTPCLIENT), \ > ERRMACRO(VALIDATEPARAMS), \ > ERRMACRO(VALIDATE), \ > ERRMACRO(VALIDATERESULT), \ > ERRMACRO(CERTCHAINCHECKER), \ > ERRMACRO(CERTSELECTOR), \ > ERRMACRO(COMCERTSELPARAMS), \ > ERRMACRO(TARGETCERTCHECKERSTATE), \ > ERRMACRO(CERTBASICCONSTRAINTS), \ > ERRMACRO(CERTPOLICYQUALIFIER), \ > ERRMACRO(CERTPOLICYINFO), \ >Index: lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c >=================================================================== >RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c,v >retrieving revision 1.10 >diff -U 10 -p -r1.10 pkix_pl_lifecycle.c >--- lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c 9 Oct 2007 16:51:35 -0000 1.10 >+++ lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c 10 Oct 2007 22:31:44 -0000 >@@ -39,20 +39,21 @@ > * > * Lifecycle Functions for the PKIX PL library. > * > */ > > #include "pkix_pl_lifecycle.h" > > PKIX_Boolean pkix_pl_initialized = PKIX_FALSE; > pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > PRLock *classTableLock; >+PRLogModuleInfo *pkixLog = NULL; > > /* > * PKIX_ALLOC_ERROR is a special error object hard-coded into the > * pkix_error.o object file. It is thrown if system memory cannot be > * allocated. PKIX_ALLOC_ERROR is immutable. > * IncRef, DecRef, and Settor functions cannot be called. > */ > > /* Keep this structure definition here for its is used only once here */ > struct PKIX_Alloc_Error_ObjectStruct { >@@ -77,81 +78,113 @@ static PKIX_Alloc_Error_Object pkix_Allo > (PKIX_Error *)0, /* PKIX_Error *cause */ > (PKIX_PL_Object *)0, /* PKIX_PL_Object *info */ > } > }; > > PKIX_Error* PKIX_ALLOC_ERROR(void) > { > return &pkix_Alloc_Error_Data.error; > } > >+static PKIX_UInt32 >+pkix_pl_lifecycle_ObjectLeakCheck() >+{ >+ int typeCounter = 0; >+ PKIX_UInt32 numObjects = 0; >+ char classNameBuff[128]; >+ char *className = NULL; >+ >+ for (; typeCounter < PKIX_NUMTYPES; typeCounter++) { >+ >+ numObjects += systemClasses[typeCounter].objCounter; >+ >+ if (!pkixLog || !systemClasses[typeCounter].objCounter) { >+ continue; >+ } >+ className = systemClasses[typeCounter].description; >+ if (!className) { >+ className = classNameBuff; >+ sprintf(className, "Unknown(ref %d)", >+ systemClasses[typeCounter].objCounter); >+ } >+ >+ PR_LOG(pkixLog, 1, ("Class %s leaked %d objects of " >+ "size %d bytes, total = %d bytes\n", className, >+ systemClasses[typeCounter].objCounter, >+ systemClasses[typeCounter].typeObjectSize, >+ systemClasses[typeCounter].objCounter * >+ systemClasses[typeCounter].typeObjectSize)); >+ } >+ >+ return numObjects; >+} >+ > /* > * PKIX_PL_Initialize (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Initialize( > PKIX_Boolean platformInitNeeded, > PKIX_Boolean useArenas, > void **pPlContext) > { > void *plContext = NULL; > >- pkix_ClassTable_Entry nullEntry = {NULL}; >- /* XXX currently using canned value for config dir; add to plContext */ >- > PKIX_ENTER(OBJECT, "PKIX_PL_Initialize"); > > /* > * This function can only be called once. If it has already been > * called, we return a positive status. > */ > if (pkix_pl_initialized) { > PKIX_RETURN(OBJECT); > } > > classTableLock = PR_NewLock(); > if (classTableLock == NULL) { > return PKIX_ALLOC_ERROR(); > } > >- /* we don't need to register OBJECT */ >- systemClasses[PKIX_OBJECT_TYPE] = nullEntry; >+ if (PR_GetEnv("NSS_STRICT_SHUTDOWN")) { >+ pkixLog = PR_NewLogModule("pkix"); >+ } >+ /* >+ * Register Object, it is the base object of all other objects. >+ */ >+ pkix_pl_Object_RegisterSelf(plContext); > > /* > * Register Error and String, since they will be needed if > * there is a problem in registering any other type. > */ > pkix_Error_RegisterSelf(plContext); >- (void) pkix_pl_String_RegisterSelf(plContext); >+ pkix_pl_String_RegisterSelf(plContext); > > > /* > * We register all other system types > * (They don't need to be in order, but it's > * easier to keep track of what types are registered > * if we register them in the same order as their > * numbers, defined in pkixt.h. > */ >- (void) pkix_pl_BigInt_RegisterSelf(plContext); /* 1-10 */ >- (void) pkix_pl_ByteArray_RegisterSelf(plContext); >- /* already registered! pkix_Error_RegisterSelf(plContext); */ >- (void) pkix_pl_HashTable_RegisterSelf(plContext); >+ pkix_pl_BigInt_RegisterSelf(plContext); /* 1-10 */ >+ pkix_pl_ByteArray_RegisterSelf(plContext); >+ pkix_pl_HashTable_RegisterSelf(plContext); > pkix_List_RegisterSelf(plContext); > pkix_Logger_RegisterSelf(plContext); >- (void) pkix_pl_Mutex_RegisterSelf(plContext); >- (void) pkix_pl_OID_RegisterSelf(plContext); >- (void) pkix_pl_RWLock_RegisterSelf(plContext); >- /* already registered! pkix_pl_String_RegisterSelf(plContext); */ >+ pkix_pl_Mutex_RegisterSelf(plContext); >+ pkix_pl_OID_RegisterSelf(plContext); >+ pkix_pl_RWLock_RegisterSelf(plContext); > > pkix_pl_CertBasicConstraints_RegisterSelf(plContext); /* 11-20 */ > pkix_pl_Cert_RegisterSelf(plContext); >- /* pkix_HttpClient_RegisterSelf(plContext); */ > pkix_pl_CRL_RegisterSelf(plContext); > pkix_pl_CRLEntry_RegisterSelf(plContext); > pkix_pl_Date_RegisterSelf(plContext); > pkix_pl_GeneralName_RegisterSelf(plContext); > pkix_pl_CertNameConstraints_RegisterSelf(plContext); > pkix_pl_PublicKey_RegisterSelf(plContext); > pkix_TrustAnchor_RegisterSelf(plContext); > > pkix_pl_X500Name_RegisterSelf(plContext); /* 21-30 */ > pkix_pl_HttpCertStoreContext_RegisterSelf(plContext); >@@ -180,21 +213,21 @@ PKIX_PL_Initialize( > pkix_ForwardBuilderState_RegisterSelf(plContext); > pkix_SignatureCheckerState_RegisterSelf(plContext); > pkix_NameConstraintsCheckerState_RegisterSelf(plContext); > pkix_DefaultRevocationChecker_RegisterSelf(plContext); > pkix_pl_LdapRequest_RegisterSelf(plContext); > pkix_pl_LdapResponse_RegisterSelf(plContext); > pkix_pl_LdapDefaultClient_RegisterSelf(plContext); > pkix_pl_Socket_RegisterSelf(plContext); > > pkix_ResourceLimits_RegisterSelf(plContext); /* 51-59 */ >- (void) pkix_pl_MonitorLock_RegisterSelf(plContext); >+ pkix_pl_MonitorLock_RegisterSelf(plContext); > pkix_pl_InfoAccess_RegisterSelf(plContext); > pkix_pl_AIAMgr_RegisterSelf(plContext); > pkix_OcspChecker_RegisterSelf(plContext); > pkix_pl_OcspRequest_RegisterSelf(plContext); > pkix_pl_OcspResponse_RegisterSelf(plContext); > pkix_pl_HttpDefaultClient_RegisterSelf(plContext); > pkix_VerifyNode_RegisterSelf(plContext); > > if (pPlContext) { > PKIX_CHECK(PKIX_PL_NssContext_Create >@@ -210,28 +243,35 @@ cleanup: > > PKIX_RETURN(OBJECT); > } > > /* > * PKIX_PL_Shutdown (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Shutdown(void *plContext) > { >+ PKIX_UInt32 numLeakedObjects = 0; >+ > PKIX_ENTER(OBJECT, "PKIX_PL_Shutdown"); > > if (!pkix_pl_initialized) { > /* The library was not initilized */ > PKIX_RETURN(OBJECT); > } > > PR_DestroyLock(classTableLock); > >+ numLeakedObjects = pkix_pl_lifecycle_ObjectLeakCheck(); >+ >+ if (PR_GetEnv("NSS_STRICT_SHUTDOWN")) { >+ PORT_Assert(numLeakedObjects == 0); >+ } >+ > if (plContext != NULL) { > PKIX_PL_NssContext_Destroy(plContext); > } > > pkix_pl_initialized = PKIX_FALSE; > > PKIX_RETURN(OBJECT); >- > } >Index: lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c >=================================================================== >RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c,v >retrieving revision 1.4 >diff -U 10 -p -r1.4 pkix_pl_object.c >--- lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c 6 Sep 2007 20:48:58 -0000 1.4 >+++ lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c 10 Oct 2007 22:31:44 -0000 >@@ -36,25 +36,20 @@ > * ***** END LICENSE BLOCK ***** */ > /* > * pkix_pl_object.c > * > * Object Construction, Destruction and Callback Functions > * > */ > > #include "pkix_pl_object.h" > >-/* --Debugging---------------------------------------------------- */ >- >-static PKIX_UInt32 refCountTotal = 0; >- >- > /* --Class-Table-Initializers------------------------------------ */ > > /* > * Create storage space for 20 Class Table buckets. > * These are only for user-defined types. System types are registered > * separately by PKIX_PL_Initialize. > */ > > static pkix_pl_HT_Elem* > pkix_Raw_ClassTable_Buckets[] = { >@@ -180,23 +175,20 @@ pkix_pl_Object_Destroy( > PKIX_RECEIVEDCORRUPTEDOBJECTARGUMENT); > > /* Attempt to delete an object still being used */ > if (objectHeader->references != 0) { > PKIX_ERROR_FATAL(PKIX_OBJECTSTILLREFERENCED); > } > > objectHeader->magicHeader = 0; > PKIX_DECREF(objectHeader->stringRep); > >- PKIX_CHECK(pkix_UnlockObject(object, plContext), >- PKIX_ERRORUNLOCKINGOBJECT); >- > /* Destroy this object's lock */ > PKIX_OBJECT_DEBUG("\tCalling PR_DestroyLock).\n"); > PR_DestroyLock(objectHeader->lock); > objectHeader->lock = NULL; > object = NULL; > > PKIX_FREE(objectHeader); > > cleanup: > >@@ -266,21 +258,20 @@ pkix_pl_Object_Equals_Default( > * Returns NULL if the function succeeds. > * Returns an Object Error if the function fails in a non-fatal way. > * Returns a Fatal Error if the function fails in an unrecoverable way. > */ > static PKIX_Error * > pkix_pl_Object_ToString_Default( > PKIX_PL_Object *object, > PKIX_PL_String **pString, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_PL_String *formatString = NULL; > PKIX_PL_String *descString = NULL; > char *format = "%s@Address: %x"; > char *description = NULL; > PKIX_UInt32 type; > > PKIX_ENTER(OBJECT, "pkix_pl_Object_ToString_Default"); > PKIX_NULLCHECK_TWO(object, pString); > >@@ -425,21 +416,20 @@ pkix_pl_Object_Hashcode_Default( > * Returns NULL if the function succeeds. > * Returns an Object Error if the function fails in a non-fatal way. > * Returns a Fatal Error if the function fails in an unrecoverable way. > */ > PKIX_Error * > pkix_pl_Object_RetrieveEqualsCallback( > PKIX_PL_Object *object, > PKIX_PL_EqualsCallback *pEqualsCallback, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > PKIX_PL_Object *objectHeader = NULL; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_PL_EqualsCallback func = NULL; > pkix_ClassTable_Entry entry; > > PKIX_ENTER(OBJECT, "pkix_pl_Object_RetrieveEqualsCallback"); > PKIX_NULLCHECK_TWO(object, pEqualsCallback); > > PKIX_CHECK(pkix_pl_Object_GetHeader > (object, &objectHeader, plContext), >@@ -474,28 +464,66 @@ pkix_pl_Object_RetrieveEqualsCallback( > } else { > *pEqualsCallback = ctEntry->equalsFunction; > } > } > > cleanup: > > PKIX_RETURN(OBJECT); > } > >+/* >+ * FUNCTION: pkix_pl_Object_RegisterSelf >+ * DESCRIPTION: >+ * Registers PKIX_OBJECT_TYPE and its related functions with systemClasses[] >+ * THREAD SAFETY: >+ * Not Thread Safe - for performance and complexity reasons >+ * >+ * Since this function is only called by PKIX_PL_Initialize, which should >+ * only be called once, it is acceptable that this function is not >+ * thread-safe. >+ * >+ * PKIX_PL_Object should have all function pointes to be to NULL: they >+ * work as proxy function to a real objects. >+ * >+ */ >+PKIX_Error * >+pkix_pl_Object_RegisterSelf(void *plContext) >+{ >+ extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; >+ pkix_ClassTable_Entry entry; >+ >+ PKIX_ENTER(ERROR, "pkix_pl_Object_RegisterSelf"); >+ >+ entry.description = "Object"; >+ entry.objCounter = 0; >+ entry.typeObjectSize = sizeof(PKIX_PL_Object); >+ entry.destructor = NULL; >+ entry.equalsFunction = NULL; >+ entry.hashcodeFunction = NULL; >+ entry.toStringFunction = NULL; >+ entry.comparator = NULL; >+ entry.duplicateFunction = NULL; >+ >+ systemClasses[PKIX_OBJECT_TYPE] = entry; >+ >+ PKIX_RETURN(ERROR); >+} >+ > /* --Public-Functions------------------------------------------------------- */ > > /* > * FUNCTION: PKIX_PL_Object_Alloc (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Object_Alloc( >- PKIX_UInt32 type, >+ PKIX_TYPENUM type, > PKIX_UInt32 size, > PKIX_PL_Object **pObject, > void *plContext) > { > PKIX_PL_Object *object = NULL; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_Boolean typeRegistered; > > PKIX_ENTER(OBJECT, "PKIX_PL_Object_Alloc"); > PKIX_NULLCHECK_ONE(pObject); >@@ -519,42 +547,43 @@ PKIX_PL_Object_Alloc( > PR_Unlock(classTableLock); > if (pkixErrorResult){ > PKIX_ERROR_FATAL(PKIX_COULDNOTLOOKUPINHASHTABLE); > } > > typeRegistered = (ctEntry != NULL); > > if (!typeRegistered) { > PKIX_ERROR_FATAL(PKIX_UNKNOWNTYPEARGUMENT); > } >+ } else { >+ ctEntry = &systemClasses[type]; > } >+ >+ PORT_Assert(size == ctEntry->typeObjectSize); > > /* Allocate space for the object header and the requested size */ > PKIX_CHECK(PKIX_PL_Malloc > (((PKIX_UInt32)sizeof (PKIX_PL_Object))+size, > (void **)&object, > plContext), > PKIX_MALLOCFAILED); > > /* Initialize all object fields */ > object->magicHeader = PKIX_MAGIC_HEADER; > object->type = type; > object->references = 1; /* Default to a single reference */ > object->stringRep = NULL; > object->hashcode = 0; > object->hashcodeCached = 0; > >- /* XXX Debugging - not thread safe */ >- refCountTotal++; >- >- PKIX_REF_COUNT_DEBUG_ARG("PKIX_PL_Object_Alloc: refCountTotal == %d \n", >- refCountTotal); >+ /* Atomically increment object counter */ >+ PR_AtomicIncrement(&ctEntry->objCounter); > > /* Cannot use PKIX_PL_Mutex because it depends on Object */ > /* Using NSPR Locks instead */ > PKIX_OBJECT_DEBUG("\tCalling PR_NewLock).\n"); > object->lock = PR_NewLock(); > if (object->lock == NULL) { > PKIX_FREE(pObject); > return (PKIX_ALLOC_ERROR()); > } > >@@ -740,63 +769,43 @@ PKIX_PL_Object_IncRef( > } > > if (object == (PKIX_PL_Object*)PKIX_ALLOC_ERROR()) { > PKIX_ERROR_FATAL(PKIX_ATTEMPTTOINCREFALLOCERROR); > } > > /* Shift pointer from user data to object header */ > PKIX_CHECK(pkix_pl_Object_GetHeader(object, &objectHeader, plContext), > PKIX_RECEIVEDCORRUPTEDOBJECTARGUMENT); > >- /* Lock the test and increment */ >- PKIX_OBJECT_DEBUG("\tAcquiring object lock).\n"); >- PKIX_CHECK(pkix_LockObject(object, plContext), >- PKIX_ERRORLOCKINGOBJECT); >- > /* This object should never have zero references */ >- refCountError = (objectHeader->references == 0); >- >- if (!refCountError) { >- objectHeader->references++; >- refCountTotal++; >- PKIX_REF_COUNT_DEBUG_ARG("PKIX_PL_Object_IncRef: " >- "refCountTotal == %d \n", refCountTotal); >- } >- >- PKIX_CHECK(pkix_UnlockObject(object, plContext), >- PKIX_ERRORUNLOCKINGOBJECT); >+ refCountError = (PR_AtomicIncrement(&objectHeader->references) <= 1); > > if (refCountError) { > PKIX_THROW(FATAL, PKIX_OBJECTWITHNONPOSITIVEREFERENCES); > } > > cleanup: > > PKIX_RETURN(OBJECT); > } > > /* > * FUNCTION: PKIX_PL_Object_DecRef (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Object_DecRef( > PKIX_PL_Object *object, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; >- PKIX_PL_DestructorCallback destructor = NULL; >- pkix_ClassTable_Entry *ctEntry = NULL; >- PKIX_PL_DestructorCallback func = NULL; >+ PKIX_Int32 refCount = 0; > PKIX_PL_Object *objectHeader = NULL; >- pkix_ClassTable_Entry entry; > PKIX_PL_NssContext *context = NULL; >- PKIX_Boolean refCountError = PKIX_FALSE; > > PKIX_ENTER(OBJECT, "PKIX_PL_Object_DecRef"); > PKIX_NULLCHECK_ONE(object); > > if (plContext){ > /* > * PKIX_PL_NssContext is not a complete PKIX Type, it doesn't > * have a header therefore we cannot verify its type before > * casting. > */ >@@ -807,108 +816,88 @@ PKIX_PL_Object_DecRef( > } > > if (object == (PKIX_PL_Object*)PKIX_ALLOC_ERROR()) { > PKIX_ERROR_FATAL(PKIX_ATTEMPTTODECREFALLOCERROR); > } > > /* Shift pointer from user data to object header */ > PKIX_CHECK(pkix_pl_Object_GetHeader(object, &objectHeader, plContext), > PKIX_RECEIVEDCORRUPTEDOBJECTARGUMENT); > >- /* Lock the test for the reference count and the decrement */ >- PKIX_OBJECT_DEBUG("\tAcquiring object lock).\n"); >- PKIX_CHECK(pkix_LockObject(object, plContext), >- PKIX_ERRORLOCKINGOBJECT); >- >- refCountError = (objectHeader->references == 0); >- >- if (!refCountError) { >- objectHeader->references--; >- refCountTotal--; >- >- PKIX_REF_COUNT_DEBUG_ARG("PKIX_PL_Object_DecRef: " >- "refCountTotal == %d \n", refCountTotal); >- >- if (objectHeader->references == 0) { >- /* first, special handling for system types */ >- if (objectHeader->type < PKIX_NUMTYPES){ >- entry = systemClasses[objectHeader->type]; >- func = entry.destructor; >- if (func != NULL){ >- /* Call destructor on user data if necessary */ >- PKIX_CHECK_FATAL(func(object, plContext), >- PKIX_ERRORINOBJECTDEFINEDESTROY); >- } >- } else { >- PKIX_OBJECT_DEBUG("\tCalling PR_Lock).\n"); >- PR_Lock(classTableLock); >- pkixErrorResult = pkix_pl_PrimHashTable_Lookup >- (classTable, >- (void *)&objectHeader->type, >- objectHeader->type, >- NULL, >- (void **)&ctEntry, >- plContext); >- PKIX_OBJECT_DEBUG >- ("\tCalling PR_Unlock).\n"); >- PR_Unlock(classTableLock); >- if (pkixErrorResult){ >- PKIX_ERROR_FATAL >- (PKIX_ERRORINGETTINGDESTRUCTOR); >- } >- >- if (ctEntry != NULL){ >- destructor = ctEntry->destructor; >- } >- >- if (destructor != NULL) { >- /* Call destructor on user data if necessary */ >- PKIX_CHECK_FATAL(destructor >- (object, plContext), >- PKIX_ERRORINOBJECTDEFINEDESTROY); >- } >- } >- >- /* pkix_pl_Object_Destroy assumes the lock is held */ >- /* It will call unlock and destroy the object */ >- return (pkix_pl_Object_Destroy(object, plContext)); >+ refCount = PR_AtomicDecrement(&objectHeader->references); > >+ if (refCount == 0) { >+ PKIX_PL_DestructorCallback destructor = NULL; >+ pkix_ClassTable_Entry *ctEntry = NULL; >+ >+ /* first, special handling for system types */ >+ if (objectHeader->type >= PKIX_NUMTYPES){ >+ PKIX_OBJECT_DEBUG("\tCalling PR_Lock).\n"); >+ PR_Lock(classTableLock); >+ pkixErrorResult = pkix_pl_PrimHashTable_Lookup >+ (classTable, >+ (void *)&objectHeader->type, >+ objectHeader->type, >+ NULL, >+ (void **)&ctEntry, >+ plContext); >+ PKIX_OBJECT_DEBUG >+ ("\tCalling PR_Unlock).\n"); >+ PR_Unlock(classTableLock); >+ if (pkixErrorResult){ >+ PKIX_ERROR_FATAL >+ (PKIX_ERRORINGETTINGDESTRUCTOR); > } >+ >+ if (ctEntry != NULL){ >+ destructor = ctEntry->destructor; >+ } >+ } else { >+ ctEntry = &systemClasses[objectHeader->type]; >+ destructor = ctEntry->destructor; >+ } >+ >+ if (destructor != NULL){ >+ /* Call destructor on user data if necessary */ >+ PKIX_CHECK_FATAL(destructor(object, plContext), >+ PKIX_ERRORINOBJECTDEFINEDESTROY); >+ } >+ >+ /* Atomically decrement object counter */ >+ PR_AtomicDecrement(&ctEntry->objCounter); >+ >+ /* pkix_pl_Object_Destroy assumes the lock is held */ >+ /* It will call unlock and destroy the object */ >+ return (pkix_pl_Object_Destroy(object, plContext)); > } > >- PKIX_OBJECT_DEBUG("\tReleasing object lock).\n"); >- PKIX_CHECK(pkix_UnlockObject(object, plContext), >- PKIX_ERRORUNLOCKINGOBJECT); >- >- /* if a reference count was already zero, throw an error */ >- if (refCountError) { >+ if (refCount < 0) { > return (PKIX_ALLOC_ERROR()); > } > > cleanup: > > PKIX_RETURN(OBJECT); > } > > > > /* > * FUNCTION: PKIX_PL_Object_Equals (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Object_Equals( > PKIX_PL_Object *firstObject, > PKIX_PL_Object *secondObject, > PKIX_Boolean *pResult, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > PKIX_PL_Object *firstObjectHeader = NULL; > PKIX_PL_Object *secondObjectHeader = NULL; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_PL_EqualsCallback func = NULL; > pkix_ClassTable_Entry entry; > > PKIX_ENTER(OBJECT, "PKIX_PL_Object_Equals"); > PKIX_NULLCHECK_THREE(firstObject, secondObject, pResult); > > PKIX_CHECK(pkix_pl_Object_GetHeader >@@ -970,21 +959,20 @@ cleanup: > > /* > * FUNCTION: PKIX_PL_Object_Duplicate (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Object_Duplicate( > PKIX_PL_Object *firstObject, > PKIX_PL_Object **pNewObject, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > PKIX_PL_Object *firstObjectHeader = NULL; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_PL_DuplicateCallback func = NULL; > pkix_ClassTable_Entry entry; > > PKIX_ENTER(OBJECT, "PKIX_PL_Object_Duplicate"); > PKIX_NULLCHECK_TWO(firstObject, pNewObject); > > PKIX_CHECK(pkix_pl_Object_GetHeader > (firstObject, &firstObjectHeader, plContext), >@@ -1031,21 +1019,20 @@ cleanup: > > /* > * FUNCTION: PKIX_PL_Object_Hashcode (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Object_Hashcode( > PKIX_PL_Object *object, > PKIX_UInt32 *pValue, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > PKIX_PL_Object *objectHeader = NULL; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_PL_HashcodeCallback func = NULL; > pkix_ClassTable_Entry entry; > PKIX_Boolean objectLocked = PKIX_FALSE; > PKIX_UInt32 objectHash; > > PKIX_ENTER(OBJECT, "PKIX_PL_Object_Hashcode"); > PKIX_NULLCHECK_TWO(object, pValue); > >@@ -1124,21 +1111,20 @@ cleanup: > > /* > * FUNCTION: PKIX_PL_Object_ToString (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Object_ToString( > PKIX_PL_Object *object, > PKIX_PL_String **pString, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > PKIX_PL_Object *objectHeader = NULL; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_PL_ToStringCallback func = NULL; > pkix_ClassTable_Entry entry; > PKIX_Boolean objectLocked = PKIX_FALSE; > PKIX_PL_String *objectString; > > PKIX_ENTER(OBJECT, "PKIX_PL_Object_ToString"); > PKIX_NULLCHECK_TWO(object, pString); > >@@ -1250,21 +1236,20 @@ cleanup: > /* > * FUNCTION: PKIX_PL_Object_Compare (see comments in pkix_pl_system.h) > */ > PKIX_Error * > PKIX_PL_Object_Compare( > PKIX_PL_Object *firstObject, > PKIX_PL_Object *secondObject, > PKIX_Int32 *pResult, > void *plContext) > { >- extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; > PKIX_PL_Object *firstObjectHeader = NULL; > PKIX_PL_Object *secondObjectHeader = NULL; > pkix_ClassTable_Entry *ctEntry = NULL; > PKIX_PL_ComparatorCallback func = NULL; > pkix_ClassTable_Entry entry; > > PKIX_ENTER(OBJECT, "PKIX_PL_Object_Compare"); > PKIX_NULLCHECK_THREE(firstObject, secondObject, pResult); > > /* Shift pointer from user data to object header */ >Index: lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h >=================================================================== >RCS file: /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h,v >retrieving revision 1.3 >diff -U 10 -p -r1.3 pkix_pl_object.h >--- lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h 31 Aug 2007 03:21:41 -0000 1.3 >+++ lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h 10 Oct 2007 22:31:44 -0000 >@@ -66,21 +66,21 @@ extern "C" { > * > * Object operations receive a pointer to raw user data as an argument. > * The macro HEADER(object) returns a pointer to the object header. > * An assertion then verifies that the first field is the MAGIC_HEADER. > */ > > /* PKIX_PL_Object Structure Definition */ > struct PKIX_PL_ObjectStruct { > PKIX_UInt32 magicHeader; > PKIX_UInt32 type; >- PKIX_UInt32 references; >+ PKIX_Int32 references; > PRLock *lock; > PKIX_PL_String *stringRep; > PKIX_UInt32 hashcode; > PKIX_Boolean hashcodeCached; > }; > > /* see source file for function documentation */ > > PKIX_Error * > pkix_pl_Object_RetrieveEqualsCallback( >@@ -95,16 +95,20 @@ PKIX_PL_Object_RegisterSystemType( > PKIX_PL_EqualsCallback equalsFunction, > PKIX_PL_HashcodeCallback hashcodeFunction, > PKIX_PL_ToStringCallback toStringFunction, > PKIX_PL_ComparatorCallback comparator, > PKIX_PL_DuplicateCallback duplicateFunction, > void *plContext); > > extern PKIX_Boolean initializing; > extern PKIX_Boolean initialized; > extern PRLock *classTableLock; >+extern pkix_ClassTable_Entry systemClasses[PKIX_NUMTYPES]; >+ >+PKIX_Error * >+pkix_pl_Object_RegisterSelf(void *plContext); > > #ifdef __cplusplus > } > #endif > > #endif /* _PKIX_PL_OBJECT_H */
Attachment #284379 - Flags: review?(nelson) → review+
Code related to "user-defined-types" will be removed in the patch for bug 397825.
Patch V3 is integrated.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
This checkin caused all tinderboxes to go orange. The reason was nto esy to spot. It is assertion failures. Assertion failure: numLeakedObjects == 0, at pkix_pl_lifecycle.c:266 ./all.sh: line 912: 11292 Aborted ${PROFTOOL} selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} \ ${SERVER_OPTIONS} ${ECC_OPTIONS} -w nss ${sparam} -i ${R_SERVERPID} $verbose Assertion failure: numLeakedObjects == 0, at pkix_pl_lifecycle.c:266 ./all.sh: line 1423: 20363 Aborted tstclnt -d $trgDir -S -h $host -p $IOPR_DOWNLOAD_PORT -w ${R_PWFILE} \ -o <$req >$file As a stopgap measure, I propose that we remove that assertion failure until the known leaks are fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Where is the log output that reports which objects are leaked, and how many of each kind?
We decided to enable printing of object only when PR log is enabled. The log is called "pkix". I've commented out the assertion that makes tests to fail. Will uncomment the code after bug 397832 is fixed.
Alexei, Please write a patch to enable the PKIX object leak logging, and cause the log output to be copied into the build log so we can see it in the Tinderbox logs.
Attachment #286327 - Flags: review?(nelson)
Comment on attachment 286327 [details] [diff] [review] Ifdef user defined object type code. r- I emailed review comments to Alexei
Attachment #286327 - Flags: review?(nelson) → review-
fixed. The last attachment will be added into bug 403538
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: