Closed Bug 353572 Opened 14 years ago Closed 14 years ago

leak in sftk_OpenCertDB


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: julien.pierre, Assigned: julien.pierre)



(Whiteboard: FIPS)


(1 file, 1 obsolete file)

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
Attachment #239431 - Flags: superreview?(nelson)
Attachment #239431 - Flags: review?(alexei.volkov.bugs)
Priority: -- → P2
Target Milestone: --- → 3.11.4
This change impacts code now under FIPS evaluation, IINM.
Whiteboard: FIPS
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 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.
Attachment #239442 - Flags: superreview?(julien.pierre.bugs)
Attachment #239442 - Flags: review?(alexei.volkov.bugs)
Comment on attachment 239442 [details] [diff] [review]
Free certdb handle in nsslowcert_ClosePermCertDB


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 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)

I prefer attachment 239431 [details] [diff] [review] ;).
Comment on attachment 239431 [details] [diff] [review]
Free certdb handle after refcount reaches zero

This patch will (er, may) cause function 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)
Comment on attachment 239431 [details] [diff] [review]
Free certdb handle after refcount reaches zero


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 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)
Depends on: 353584
I went and looked at the list of callers of sftk_freeCertDB, 
expecting that list to be tiny.  To my surprise, it was HUGE.

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.  

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 .
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.

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);

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 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 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+
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.
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


Checking in dbinit.c;
/cvsroot/mozilla/security/nss/lib/softoken/dbinit.c,v  <--  dbinit.c
new revision:; previous revision: 1.28
Closed: 14 years ago
Resolution: --- → FIXED

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.