353572 - leak in sftk_OpenCertDB

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

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

Comment 1

[Julien Pierre]

Attachment: Free certdb handle after refcount reaches zero

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This change impacts code now under FIPS evaluation, IINM.

Comment 3

[Julien Pierre]

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

[Wan-Teh Chang]

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

[Wan-Teh Chang]

Attachment: Free certdb handle in nsslowcert_ClosePermCertDB

Comment 6

[Julien Pierre]

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

[Wan-Teh Chang]

Comment on attachment 239442 [details] [diff] [review]
Free certdb handle in nsslowcert_ClosePermCertDB

Julien, I'll let you decide what's the right fix.

Comment 8

[Julien Pierre]

Wan-Teh,

I prefer attachment 239431 [details] [diff] [review] ;).

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Julien Pierre]

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.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 12

[Julien Pierre]

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

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 14

[Julien Pierre]

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

[Alexei Volkov]

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.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 17

[Wan-Teh Chang]

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.

Comment 18

[Julien Pierre]

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

Comment 19

[Julien Pierre]

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.