Closed
Bug 353572
Opened 18 years ago
Closed 18 years ago
leak in sftk_OpenCertDB
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.4
People
(Reporter: julien.pierre, Assigned: julien.pierre)
References
Details
(Whiteboard: FIPS)
Attachments
(1 file, 1 obsolete file)
764 bytes,
patch
|
alvolkov.bgs
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
This leak occurs for each C_Initialize . Below is relevant libumem output from Anton . Patch forthcoming. umem_alloc_24 leak: 85068 buffers, 24 bytes each, 2041632 bytes total ADDR BUFADDR TIMESTAMP THREAD CACHE LASTLOG CONTENTS 80f5f28 80f2a08 119d8542eba8fb 1 807b510 80741f8 0 libumem.so.1`umem_cache_alloc_debug+0x14f libumem.so.1`umem_cache_alloc+0x144 libumem.so.1`umem_alloc+0xcc libumem.so.1`malloc+0x27 libumem.so.1`calloc+0x47 libnspr4.so`PR_Calloc+0x79 libsoftokn3.so`PORT_ZAlloc+0x52 libsoftokn3.so`sftk_OpenCertDB+0xa3 libsoftokn3.so`sftk_DBInit+0x51 libsoftokn3.so`SFTK_SlotReInit+0x1b1 libsoftokn3.so`SFTK_SlotInit+0x2f3 libsoftokn3.so`nsc_CommonInitialize+0x341 libsoftokn3.so`NSC_Initialize+0x53 libnss3.so`secmod_ModuleInit+0xf4 libnss3.so`SECMOD_LoadPKCS11Module+0x2cd libnss3.so`SECMOD_LoadModule+0x147 libnss3.so`SECMOD_LoadModule+0x1cd libnss3.so`nss_Init+0x2da libnss3.so`NSS_Initialize+0x114 libssldap50.so`ldapssl_basic_init+0x1ac libssldap50.so`ldapssl_clientauth_init+0x4b libssldap50.so`ldapssl_client_init+0x34 main+0x2b _start+0x7a
Assignee | ||
Comment 1•18 years ago
|
||
Attachment #239431 -
Flags: superreview?(nelson)
Attachment #239431 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Updated•18 years ago
|
Priority: -- → P2
Target Milestone: --- → 3.11.4
Assignee | ||
Comment 3•18 years ago
|
||
Hopefully, the validation doesn't depend on the presence of this leak. And AFAIK the module that will be certified is 3.11.5, and we still have an opportunity to get some fixes, correct ? Anyway, if this fix can't go in to 3.11.x, at least it should go into the tip for 3.12 .
Comment 4•18 years ago
|
||
Comment on attachment 239431 [details] [diff] [review] Free certdb handle after refcount reaches zero Looking at this patch, you'll see this patch create an asymmetry between sftk_freeCertDB and sftk_freeKeyDB: one frees the handle but the other doesn't. It turns out that nsslowkey_CloseKeyDB frees the handle. So I think it is better to free the handle in nsslowcert_ClosePermCertDB than in sftk_freeCertDB. You'd need to remove the PORT_Free call after the nsslowcert_ClosePermCertDB call in dbck.c. If you fix this bug on Thursday we can include it in the FIPS crypto module. We may have another opportunity to pick up new bug fixes next week, but it's safer to check in the fix on Thursday.
Comment 5•18 years ago
|
||
Attachment #239442 -
Flags: superreview?(julien.pierre.bugs)
Attachment #239442 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Comment 6•18 years ago
|
||
Comment on attachment 239442 [details] [diff] [review] Free certdb handle in nsslowcert_ClosePermCertDB Wan-Teh, I noticed the assymetry between the cert and key DB, and considered doing my patch, but decided against making the change in your patch, because I thought the handle should be freed in the same layer in which it was allocated. The keydb handle is allocated by the nsslowcert layer, so it is correctly being freed there. But the certdb handle is allocated by the sftk layer, so IMO it needs to be freed in that same layer, which is what my patch does. If we wanted to make things consistent, we could change the certdb handle to be allocated in the lowcert layer, and then we could free it there too; but I didn't want to make the bigger code changes required to do that.
Comment 7•18 years ago
|
||
Comment on attachment 239442 [details] [diff] [review] Free certdb handle in nsslowcert_ClosePermCertDB Julien, I'll let you decide what's the right fix.
Attachment #239442 -
Attachment is obsolete: true
Attachment #239442 -
Flags: superreview?(julien.pierre.bugs)
Attachment #239442 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Comment 8•18 years ago
|
||
Wan-Teh, I prefer attachment 239431 [details] [diff] [review] ;).
Comment 9•18 years ago
|
||
Comment on attachment 239431 [details] [diff] [review] Free certdb handle after refcount reaches zero This patch will (er, may) cause function DestroyCertificate http://lxr.mozilla.org/security/ident?i=DestroyCertificate to crash. It frees certHandle, but the caller still has that pointer and will attempt to continue to use it, IINM. 5032 if (handle) { 5033 sftk_freeCertDB(handle); 5034 } 5037 } 5038 if ( lockdb && handle ) { 5039 nsslowcert_UnlockDB(handle); 5040 sftk_freeCertDB(handle); 5041 } But maybe that's a flaw in DestroyCertificate, that should be fixed in DestroyCertificate by setting handle to NULL after each call to sftk_freeCertDB. Either way, this patch won't fly as-is.
Attachment #239431 -
Flags: superreview?(nelson)
Attachment #239431 -
Flags: superreview-
Attachment #239431 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Comment 10•18 years ago
|
||
Comment on attachment 239431 [details] [diff] [review] Free certdb handle after refcount reaches zero Nelson, That double-free is a different problem, which Alexei is tracking in bug 353584 . Due to the current implementation of nsslowcert_ClosePermCertDB, the double-free couldn't manifest itself without this patch. But I ran all.sh before submitting my patch, and didn't trip over the double-free somehow. Maybe that code is untested, or the Solaris allocator I use didn't warn crash on double-free ... Since there are now 2 bugs for the 2 problems - leak and crash - please re-review. I will add bug 353584 as a dependency for this one to ensure both fixes are checked in at the same time.
Attachment #239431 -
Flags: superreview?(nelson)
Attachment #239431 -
Flags: superreview-
Attachment #239431 -
Flags: review?(alexei.volkov.bugs)
Comment 11•18 years ago
|
||
Julien, I went and looked at the list of callers of sftk_freeCertDB, expecting that list to be tiny. To my surprise, it was HUGE. http://lxr.mozilla.org/security/ident?i=sftk_freeCertDB I went and looked at one of the items on that list, and the very first one was clearly going to cause a double-free. I didn't look at any others, and I gave that patch r-. Now, I think *ALL* those callers need to be examined before we can give this patch an r+. Most malloc libraries do not crash immediately on a double-free. Instead they corrupt the heap in ways that are exploitable later, and lead to much delayed crashes, whose cause (the double free) is very difficult to detect later. So, I'm not persuaded that this patch is OK by the mere fact that a test run didn't crash in a way that obviously pointed to a double free.
Assignee | ||
Comment 12•18 years ago
|
||
Nelson, Any double-free in the callers of sftk_freeCertDB are separate, pre-existing - if previously inconsequential - problems that should be treated as part of bug 353584, which is now a dependency of this bug .
Comment 13•18 years ago
|
||
Julien, I now believe that bug 353584 is invalid, and that there is no "double free" case here. I'm still concerned that all the callers of sftk_freeCertDB must be checked before this patch is applied, but I'm now satisfied that the one case described in bug 353584 is NOT a bug. I will shortly add an explanation to that bug.
Assignee | ||
Comment 14•18 years ago
|
||
Nelson, Since bug 353584 turned out to be invalid, there is no evidence of any bug in the callers of sftk_OpenCertDB / sftk_GetCertDB / sftk_FreeCertDB at this point. While these functions have no explicit documentation, it is obvious from reviewing just a few of the callers' code (let alone all 27) that the existing semantics of sftk_FreeCertDB was that it should free everything associated with the handle when the refcount reaches zero. The evidence is that : a) sftk_OpenCertDB is the only code path that ever allocates an NSSLOWCERTCertDBHandle in the entire softoken b) none of the callers of sftk_freeCertDB ever try to free the handle themselves afterwards . Clearly, sftk_freeCertDB is expected to do it c) no callers of sftk_freeCertDB try to dereference the handle except for sftk_DBShutdown which checks the refcount as follows : if (certHandle) { PORT_Assert(certHandle->ref == 1 || slot->slotID > FIPS_SLOT_ID); sftk_freeCertDB(certHandle); } This is the last thing that's done with the handle. We know that the refcount will be zero after that sftk_freeCertDB call . It is not followed by a PORT_Free(certHandle) . I don't believe the intention was to leak it the handle. So, my conclusion is that my patch doesn't change the semantics of the sftk_freeCertDB function, but merely corrects an implementation bug where it forgot to free part of the handle, and once we know that's the case, it should be a fairly simple review .
Comment 15•18 years ago
|
||
Comment on attachment 239431 [details] [diff] [review] Free certdb handle after refcount reaches zero I've checked the callers of sftk_freeCertDB. Didn't find any issues.
Attachment #239431 -
Flags: review?(alexei.volkov.bugs) → review+
Comment 16•18 years ago
|
||
Comment on attachment 239431 [details] [diff] [review] Free certdb handle after refcount reaches zero ok, on the strength of the preceeding two comments and my own review, r=nelson
Attachment #239431 -
Flags: superreview?(nelson) → superreview+
Comment 17•18 years ago
|
||
I reviewed all the callers of sftk_freeCertDB. DestroyCertificate is the only one that may call sftk_freeCertDB twice on the same handle, and it only does that when it has two references to the handle. So this patch is correct. For code reviewer's viewing pleasure, it would be nice to use tabs where tabs are used to keep the new code and original code aligned in diffs. Just a suggestion.
Assignee | ||
Comment 18•18 years ago
|
||
Thanks for the reviews, everyone. I checked this in with the indentation corrected : Tip : Checking in dbinit.c; /cvsroot/mozilla/security/nss/lib/softoken/dbinit.c,v <-- dbinit.c new revision: 1.30; previous revision: 1.29 done NSS_3_11_BRANCH : Checking in dbinit.c; /cvsroot/mozilla/security/nss/lib/softoken/dbinit.c,v <-- dbinit.c new revision: 1.28.2.1; previous revision: 1.28 done
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•18 years ago
|
||
Christophe, Now that this memory leak is fixed, please remove the stacks that include sftk_OpenCertDB from the "ignore" list in the memory leak tests. This was an init-time leak that we had to chosen to previously ignore.
You need to log in
before you can comment on or make changes to this bug.
Description
•