Last Comment Bug 72291 - PK11_ListCerts skips duplicated keys/certs between tokens
: PK11_ListCerts skips duplicated keys/certs between tokens
Status: RESOLVED FIXED
[3.8.2]
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.2
: Sun Solaris
: P2 critical (vote)
: 3.9
Assigned To: Julien Pierre
: Sonja Mirtitsch
Mentors:
Depends on:
Blocks: 215214
  Show dependency treegraph
 
Reported: 2001-03-16 17:38 PST by Julien Pierre
Modified: 2003-09-21 18:05 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch to allow all cert instances to show up in list returned by PK11_ListCerts(PK11CertListUser, ...) (8.46 KB, patch)
2003-07-28 20:41 PDT, Julien Pierre
no flags Details | Diff | Splinter Review
new patch (5.78 KB, patch)
2003-08-04 21:25 PDT, Julien Pierre
no flags Details | Diff | Splinter Review
new patch v1.1 (with wtc's edits) (6.23 KB, patch)
2003-08-21 22:21 PDT, Wan-Teh Chang
rrelyea: review+
bugz: superreview+
Details | Diff | Splinter Review
incremental patch to fix memory leak of nicknames (1.31 KB, patch)
2003-09-03 16:21 PDT, Julien Pierre
rrelyea: review+
Details | Diff | Splinter Review

Description Julien Pierre 2001-03-16 17:38:51 PST
I have a system configured with two hardware tokens - nCipher and a Rainbow.
I successfully imported the same cert/key into both tokens using pk12util.
Now, when I use the iWS admin, if I enter the password for both tokens, the list 
only shows "ncipher:Server-Cert" . The Rainbow cert - which should be "ISG 2.0 
Cryptoki Interface:Server-Cert" is not shown.

Our admin is flexible and allows only specifying password for certain tokens. If 
I specify the password for "ISG 2.0 Cryptoki Interface" but not for "ncipher", I 
correctly see "ISG 2.0 Cryptoki Interface:Server-Cert" listed.

I believe the problem is that PK11_ListCerts tries to eliminate duplicate certs. 
However, it eliminates duplicates on a global basis, inclusive of all tokens, 
which does not work for this case.

PK11_ListCerts works fine and shows certs on both Rainbow and nCipher token if 
the certs are generated from different cert requests, and not the same cert/key 
imported via pk12util.
Comment 1 Julien Pierre 2001-03-16 17:51:22 PST
FYI, this happens when I call PK11_ListCerts with PK11CertListUnique as the 
first argument. My code then shows the certs with the CERTDB_USER flag first, 
and all the other certs thereafter.

I think this problem is a side effect of the code to eliminate duplicate certs 
between the root cert module and the internal database. Perhaps the check could 
be performed only for root certs and not for user certs ? Maybe an extra flag 
for PK11_ListCerts is needed (or one already exists ?).

FYI, on that system with the two hardware tokens, I also have the internal DB 
token configured, and have imported the identical cert/key into it.
The DB cert - "Server-Cert" (without a token prefix) is only shown if I go on 
"Manage certs" without authenticating to either hardware token. It stops being 
shown as soon as I authenticate to either one. This indicates that the 
PK11_ListCerts duplicate check includes the internal DB token.

So the behavior is
1) if I don't authenticate to hardware tokens, only "Server-Cert" is returned by 
PK11_ListCerts. This is the correct behavior.

2) if I authenticate to "ISG 2.0 Cryptoki Interface", only "ISG 2.0 Cryptoki 
Interface:Server-Cert" is returned by PK11_ListCerts . This is incorrect. 
"Server-Cert" from the internal DB should also be returned.

3) if I authenticate to "ncipher", only "ncipher:Server-Cert" is returned by 
PK11_ListCerts. This is incorrect. "Server-Cert" from the internal DB should 
also be returned.

4) if I authenticate to both "ISG 2.0 Cryptoki Interface" and "ncipher", only 
"ncipher:Server-Cert" is shown. This is incorrect. "Server-Cert" from the 
internal DB, and "ISG 2.0 Cryptoki Interface:Server-Cert" from Rainbow should 
also be returned.
Comment 2 Robert Relyea 2001-03-19 07:52:47 PST
Wait. Julian, when I wrote PK11_ListCerts you specifically asked me to give you
this semantic!

We need to talk about what you really want here. If I remove the skip, then you
will get multiple copies of the root certs again.

bob
Comment 3 Robert Relyea 2001-03-19 10:47:17 PST
Julien, what parameter are you passing to PK11_ListCerts.

Are you passing PK11CertListUser or PK11CertListUnique? PK11CertListUser should
provide all the user certs, is this one one that's not returning multiple certs?

bob
Comment 4 Julien Pierre 2001-03-19 13:37:48 PST
Bob,

I know you wrote PK11_ListCerts in response to my requests. However I was not 
aware of the existence of pk12util and the possibility of having multiple 
identical keys and certs and in separate tokens.

I am passing PK11CertListUnique , which gets me all the unique certs - user and 
root. However, I tried passing PK11CertListUser as well, to only get the user 
certs (presumable even non-unique user certs), but that also did not work 
correctly.

I think the API needs to be extended a bit to have things like "get root certs" 
or "get user certs" and "check for duplicates" or not. This is why I had 
originally suggested to you that you had bitmapped flags. I suppose you could 
extend it to by adding to your enum options. I see what PK11CertListUnique does 
- it gets all certs - user or others, and does the duplicate check. I am not 
sure what the current PK11CertListUser does though, because I couldn't tell from 
the results I got that the set of certs corresponded to anything logical. We 
might have to look at this together on my system with all the tokens.
Comment 5 Robert Relyea 2001-03-19 14:30:14 PST
User Certs do not do the unique check, which is why I was asking.

The problem here is you want root certs to come out unique, but not user certs.
That's a little too complicated a semantic to describe. What you can do is call
CertList go get all the certs, Ingore all the user certs in the list, then call
certList go get all the user certs. I'm still not sure this is going to do what
you want because you will get two listings of the same cert on the second list,
but there will only be one nickname associated with the cert.

I think we need to talk about this some more..

bob
Comment 6 Julien Pierre 2001-03-19 14:46:38 PST
Bob,

I can merge two lists together if I have the ability to extract the certs I 
want.

Eg. I could first do a lookup for all user certs, then a second lookup for root 
certs eliminating duplicates. Then I would merge the two lists and display it as 
one. This is fairly easy to do.

However I'd need the ability to do those queries with PK11_ListCerts and I have 
not yet found a proper way.
Comment 7 Julien Pierre 2001-03-23 17:55:29 PST
Bob,

I have tested your PK11_ListCerts changes. I have changed my admin code.
It used to do a single PK11_ListCerts with PK11CertListUnique . The cert 
nickname was always retrieved from the cert structure.

Based on your information about the patch, I changed it to do this : 
I now do two PK11_ListCerts calls instead of one, the first with 
PK11CertListUser and the second with 
PK11CertListRootUnique. I now always get the cert nickname from the "appData" 
field of your list element and no longer look at 
cert->nickname .

My code that retrieves the cert list is like this :

    // get all the user certs
    CERTCertList* clist = PK11_ListCerts(PK11CertListUser, NULL);
    populate(clist);

    // then get all the root certs
    clist = PK11_ListCerts(PK11CertListRootUnique, NULL);
    populate(clist);

The "populate" function looks at all the certs in the list you return, and then 
puts certs in either two of my own lists depending on whether they are 
user certs or not.

void certcb :: populate(CERTCertList* clist)
{
    if (clist)
    {
        // walk the list and add the certs to our own lists
        CERTCertListNode *cln;

        for (cln = CERT_LIST_HEAD(clist); !CERT_LIST_END(cln,clist);
                cln = CERT_LIST_NEXT(cln))
        {
            CERTCertificate* cert = cln->cert;
            const char* nickname = (const char*) cln->appData;
            SECStatus rv;
            CERTCertTrust trust;
            rv = CERT_GetCertTrust(cert, &trust);
            PRInt32 flags = 0;

            if (rv == SECSuccess)
            {
                flags = trust.sslFlags;
            }

            if (flags & CERTDB_USER)
            {
                usercertlist.insert(new RWCert(cert, nickname));
            }
            else
            {
                rootcertlist.insert(new RWCert(cert, nickname));
            };

            SECCertTimeValidity certtimestatus = CERT_CheckCertValidTimes(cert, 
PR_Now(), 
PR_FALSE);
            if (certtimestatus == secCertTimeExpired)
            {
                expired++;
            };
        };
    };
};

Since I now have a proper cert nickname, I also don't need the obfuscated 
PK11_FindCertByIssuerAndSN anymore in my "edit" CGI - a 
PK11_FindCertByNickname is adequate.

The good news first :

Your code does solve the problem for the enumeration of duplicate user certs in 
multiple tokens, including an identical key & cert in the DB + 
Rainbow + nCipher tokens. When using the PK11CertListUser option, I get all the 
user cert nicknames properly and I am able to edit and delete 
them independently.

Then the bad news :
I now have major problems with the root certs. I see them all twice with the 
same nickname, which means that both calls to PK11_ListCerts 
returned the root certs from the module.
However, if the trust bits have been changed on one of the certs, then that cert 
doesn't show as two identical nicknames. I see one with a nickname 
of "Builtin Object Token:BelSign Object Publishing CA" and the other with a 
nickname of ":BelSign Object 
Publishing CA" . I can't edit the second one since a PK11_FindCertFromNickname 
on ":BelSign Object Publishing CA" fails 
to return a cert.

To trace this problem, I just commented one of the PK11_ListCerts calls, in 
order to see which certs were coming as a result of which 
PK11_ListCerts call, since my code above separates user & root certs already in 
my own lists.
I left in place only     CERTCertList* clist = PK11_ListCerts(PK11CertListUser, 
NULL);
With this change, I still got the root certs from the root cert module displayed 
. Ie,  I see a cert with a nickname of "Builtin Object 
Token:BelSign Object Publishing CA" in my UI  . I also see my nCipher and 
Rainbow certs with proper nickname. The server-cert 
from the cert database however is not shown.

Then I tried the opposite, doing only  clist = 
PK11_ListCerts(PK11CertListRootUnique, NULL); .
With this change, I got my server-cert from the database, which is a user cert 
and should not have been returned, as it is not a root cert. BTW, I 
did not authenticate to the key db. Perhaps this causes a problem and doesn't 
let you identify it as a user cert properly. I only authenticate to the 
third party tokens; as this is in the cert management UI which doesn't require 
key database password; only the tokens do.
I also got as a result of this call a cert called
":BelSign Object Publishing CA" as well as all the other root certs whose bits 
had not been modified in the normal format of of 
"Builtin Object Token:BelSign Object Publishing CA".

In summary, there are four problems I see here :
1) PK11_ListCerts with PK11CertListUser does not return user certs from the 
database
2) PK11_ListCerts with PK11CertListUser incorrectly returns root certs from the 
root cert module. These should not be returned
3) PK11_ListCerts with PK11CertListRootUnique returns an incorrect nickname for 
root certs whose trust bits have been modified, beginning 
with a ":", that cannot be used in PK11_FindCertFromNickname
4) PK11_ListCerts with PK11CertListRootUnique incorrectly returns user certs 
from the database. These should not be returned

If you can fix these, then I think my UI will work correctly with my current 
code.
Comment 8 Julien Pierre 2001-03-26 14:57:20 PST
I have verified the fix on a non-official build. I will hold off marking this 
"verified" until there is an official build.

Comment 9 Robert Relyea 2001-04-24 14:21:02 PDT
This should be fixed in 3.2.1
Comment 10 Julien Pierre 2003-07-17 18:01:31 PDT
This appears to be broken again in current versions of NSS (3.7.x) as observed
with CMS and NES.
Comment 11 Julien Pierre 2003-07-28 19:23:26 PDT
verified broken.

When passing PK11CertListUser to PK11_ListCerts, the certlist returned only
contains one CERTCertificate, even though many instances of the same user
certificate exist on different slots. In 3.2.1, several CERTCertificate would be
returned in the list.

I have been debugging this for a long while and made several findings :

1) in pk11ListCertCallback, the stan certificate is converted to a single
CERTCertificate by calling the function STAN_GetCERTCertificate . 
It doesn't matter that the stan certificate object has several instances : there
is only a single CERTCertificate object corresponding to it, inevitably with an
arbitrarily chosen slot.

2) In lib/pki/pki3hack.c, there is a function called fill_CERTCertificateFields
which calls get_cert_instance to make the determination of which slot to fill in
the CERTCertificate .

It contains the following comment :

	    /* This only really works for two instances...  But 3.4 can't
	     * handle more anyway.  The logic is, if there are multiple
	     * instances, prefer the one that is not internal (e.g., on
	     * a hardware device.
	     */

From this I conclude that :

a. the regression occurred in NSS 3.4

b. the code that converts stan certificates to 3.x CERTCertificates chooses the
slot arbitrarily, and was only designed to handle the simplest cases of a single
instance, or one instance in the softoken and another in a hardware token. The
customer with the problem uses 3 instances of a certificate in 3 different
hardware tokens.

It would be very useful to know the reasons behind this design limitation before
I attempt to write a workaround for it.
Comment 12 Wan-Teh Chang 2003-07-28 19:49:40 PDT
Ian, could you answer Julien's question at the end of
comment 11?  Thanks.
Comment 13 Julien Pierre 2003-07-28 20:40:05 PDT
I wrote a patch that attempts to allow several CERTCertificate from a single
NSSCertificate . Basically the pk11listcertcallback iterates over each instance
and creates a new CERTCertificate using a new function call
STAN_GetSpecificCERTCertificate which takes an additional index argument ...
This patch seems quite a hack, but it appears to work.

Also, in that patch I had to prevent the caching of the CERTCertificate into the
NSSCertificate if a specific index is requested. How big a problem will that
create, if any ? I will attach the patch so you can get a better idea of what
I'm talking about.

Comment 14 Julien Pierre 2003-07-28 20:41:47 PDT
Created attachment 128759 [details] [diff] [review]
patch to allow all cert instances to show up in list returned by PK11_ListCerts(PK11CertListUser, ...)

hack
Comment 15 Wan-Teh Chang 2003-07-29 16:28:33 PDT
Comment on attachment 128759 [details] [diff] [review]
patch to allow all cert instances to show up in list returned by PK11_ListCerts(PK11CertListUser, ...)

The idea of this patch seems okay.  I don't know
what are the consequences of not caching the other
cert instances.

>-	c->decoding = dc;
>+        if (0 == index) {
>+	    c->decoding = dc;
>+        }

Is this how you prevent the caching of the CERTCertificate
into the NSSCertificate if a specific index is requested?
Comment 16 Wan-Teh Chang 2003-07-29 19:00:43 PDT
Assigned the bug to Julien.  Target NSS 3.9.
Comment 17 Julien Pierre 2003-07-29 19:10:28 PDT
Wan-Teh, in response to comment #15 :

Yes, that check was the crude way to prevent caching of the cert.
However I just see that there is more to it. I need to check for a non-zero
index upon entry of the function in order to avoid reusing the cached copy of
the cert.
Otherwise, I may be modifying that cached copy - ie. changing its slot.

Comment 18 Ian McGreer 2003-07-29 22:25:25 PDT
I can't really tell from the earlier comments how this was made to work in NSS
3.2.1.  The reason you see the comment about the NSSCertificate ->
CERTCertificate limitation is that a single CERTCertificate structure can only
be aware of a single token instance.  I suppose if this worked in NSS 3.2.1, it
was because multiple CERTCertificates were created, one for each token instance.
 That would surprise me...

Anyway, the ability to handle multiple token instances is a Stan "feature", the
need to convert that back to a CERTCertificate and pick only one to remember is
a 3.3.X "bug".

I've just glanced over the patch.  One thing Bob and I had discussed was to have
a 3.3.X API that would handle multiple instances by peeking into the Stan layer.
 I'm not sure if that code ever showed up, I thought it did, because I believe
PSM uses it.  If so, the idea was to have something like this:

PK11SlotInfo **CERT_GetSlots(CERTCertificate *cert)
char *CERT_GetNameForSlot(CERTCertificate *cert, PK11SlotInfo *slot)

I looked around the code a bit, and didn't see it, so maybe it isn't there... 
The Stan function nssCertificate_GetNickname would be a good place to start.  I
think working with slot pointers / nicknames would be better than some arbitrary
index.
Comment 19 Julien Pierre 2003-07-29 23:42:27 PDT
Ian,

Good to hear from you !

In NSS 3.2.1 the "appData" field of the CERTCertListNode was used to specify a
string indicating which instance of the certificate it was.

Eg. in the case with 3 instances, you had 3 nodes, each with a different appData
string. However, I believe the CERTCertificate structure in each node was identical.

This is how web server was able to find the certs on all tokens .

My patch is different - it creates multiple CERTCertificate structure, one per
instance. Can you tell me if this is a bad thing ?

As far as the API you propose, it certainly would be fairly simple to write, but
at this point in time I don't think it will meet our customers requirements.
Comment 20 Ian McGreer 2003-07-30 21:23:30 PDT
Having multiple CERTCertificate structures for one cert is bad.  It will confuse
the cert cache and other parts of the code which assume that will never happen.

What requirements would not be met by the proposed API?  It looked to me like
all you needed was to collect all of the names of the cert for the various token
instances.  Doing that for PSM was why Bob suggested that API (or one like it,
anyway).  I would think that API would suffice for implementing the 3.2.1
behavior you described, stuffing the names into a list in appData.
Comment 21 Julien Pierre 2003-07-31 15:56:52 PDT
Ian,

Yes, I agree it is possible for me to emulate the 3.2.1 behavior for this function.

However I will need multiple CERTCertificate structures in order to resolve bug
74822 , and if I'm going to do it for that function, I may as well do it here too.
Comment 22 Ian McGreer 2003-07-31 22:27:31 PDT
I can't see bug 74822.  But I've been told many times that there should never be
multiple CERTCertificate structures for a single cert, and I know quite a bit of
code depends on that.  As I understand it, even 3.2.1 didn't do that.

Is there any way you can summarize bug 74822 to explain why multiple structures
are needed?
Comment 23 Julien Pierre 2003-08-04 16:37:20 PDT
Ian,

I have made the bug available to you.
You say a lot of code depends on the fact that there is a single CERTCertificate
regardless of the number of instances. Can you be any more specific on what
code, and what the reasons are for this restriction ?

In the meantime, will code a new patch for this bug that doesn't create multiple
CERTCertificate but rather replicates the 3.2.1 behavior with the appData field.
Comment 24 Julien Pierre 2003-08-04 20:49:21 PDT
Ian, I see an nssCertificate_GetNickname function that I believe does something
similar to the CERT_GetNameForSlot you mentioned earlier, except it takes an
NSSToken rather a PK11Slot . I think there is a way to convert from a
PK11SlotInfo* to an NSSToken so it should be possible for me to write the char
*CERT_GetNameForSlot(CERTCertificate *cert, PK11SlotInfo *slot) public function.

I don't know if it's possible to convert NSSToken to PK11SlotInfo easily, but if
it is, I should also be able to write CERT_GetSlots .
Comment 25 Julien Pierre 2003-08-04 21:25:31 PDT
Created attachment 129209 [details] [diff] [review]
new patch

Don't return different CERTCertificate structures.
Only return a different nickname in appData for each instance.

Apps need to be aware of that field (eg. NES is, but certutil is not).
Comment 26 Julien Pierre 2003-08-04 21:26:33 PDT
Responding to myself on comment 24 :
nssCertificate_GetNickname did not work for the purpose used.
It returned instances[i].label which for some reason was constant regardless of
the token.
Comment 27 Ian McGreer 2003-08-04 21:30:12 PDT
The cert cache requires that an { issuer, serial } pair maps to a single 
certificate.  AFAIK, it has always been that way (at least, that's what Bob 
told me).  Actually, I shouldn't even say cache, it's really the in-memory 
database, that is, the collection of all certs with open references.

However that came about, various parts of the code depend on it.  For example, 
when creating a temp certificate, we first extract the { issuer, serial } and 
see if it already exists.

I guess I don't know the reasons for why this is.  I believe it's just because 
that was how the old temp db worked, so the behavior was preserved.
Comment 28 Wan-Teh Chang 2003-08-21 22:21:43 PDT
Created attachment 130211 [details] [diff] [review]
new patch v1.1 (with wtc's edits)

Julien, I made the following changes to your patch.

pk11cert.c

- Move some code outside the "if (isUnique)" statement
because it is common to the "if" and "else" blocks.  In
particular it doesn't need to be inside the for loop
in the "else" block.

- We need to lock c->object.lock around your for loop.
I don't know whether it is safe to lock c->object.lock
because we are calling several functions inside the
for loop.  If any of them locks c->object.lock, we get
a deadlock.  I don't have time to investigate that, so
I just avoid the issue by iterating over a copy of
c->object.instances.

- In the for loop, we should use the cert instance's slot
instead of newCert->slot.

pki3hack.c

- I pass the cert instance, instead of the cert instance's
token, to STAN_GetCERTCertificateNameForToken (which I
renamed STAN_GetCERTCertificateNameForInstance).  This
avoids the need to get the instance from the token.
Comment 29 Wan-Teh Chang 2003-08-21 22:24:29 PDT
Comment on attachment 130211 [details] [diff] [review]
new patch v1.1 (with wtc's edits)

Please review and test my changes to Julien's patch.  Thanks.
Comment 30 Wan-Teh Chang 2003-08-22 08:43:19 PDT
Comment on attachment 130211 [details] [diff] [review]
new patch v1.1 (with wtc's edits)

Ian,

1. If we want to iterate over all the instances of a
NSSCertificate, should we iterate over c->object.instances
directly (with proper locking), or should we call
nssPKIObject_GetInstances(&c->object) to get a copy of
the instances and iterate over the copy?

2. In pki3hack.h, the new function
STAN_GetCERTCertificateNameForInstance references			       
       nssCryptokiInstance as an opaque type.  Should I
have pki3hack.h include devt.h (where nssCryptokiInstance
is defined), or should I simply add
    typedef struct nssCryptokiInstanceStr nssCryptokiInstance;
to nssdevt.h (which pki3hack.h already includes)?

Seems like nssCryptokiInstance is an internal type
so maybe it shouldn't appear in nssdevt.h at all?
Comment 31 Robert Relyea 2003-08-26 13:33:09 PDT
catching up on bugs....

Ian is correct, we should only have once CERTCertificate instance. We can expose
additional API's to retrieve the additional tokens that this cert lives in (for
the most part the existing NSS interfaces handle certs on multiple tokens in 
only an ad-hoc manner). This is what I believe ian proposed, and the current
direction... review on wtc's patch forthcoming..

bob
Comment 32 Robert Relyea 2003-08-26 15:51:58 PDT
Comment on attachment 130211 [details] [diff] [review]
new patch v1.1 (with wtc's edits)

r=relyea
Comment 33 Ian McGreer 2003-08-27 11:26:31 PDT
Responding to Wan-Teh's comment #30:

1.  Either method is acceptable.  I was preferring the latter, as it avoids
holding a the object's lock.  OTOH, it requires a malloc & free.  In general, I 
think it most likely that client code will have multiple instances of objects, 
in which case holding the lock is less of an issue, and perhaps the first 
method is more appropriate.

2.  nssdevt.h is still a private header in NSS 3.X, thought it follows the Stan 
naming convention for public headers.  No Stan headers are public in 3.X.  But, 
for the sake of clarity, I would rather pki3hack.h included devt.h, since they 
both follow the naming convention for private headers.
Comment 34 Julien Pierre 2003-09-03 16:15:58 PDT
Another issue that we need to look at here is the freeing of the appData fields.
There is a memory leak with the nickname strings, because they are allocated
with PORT_Alloc and don't get freed when the CERTCertList is destroyed.
This is actually not a regression introduced by this patch, it has always been
the case even before we handled multiple instances.

Since the appData field is application specific, and some applications may use a
different type of object, we can't just to a PORT_Free on the appData field when
destroying the node. Moreover, some of the appData are not to be freed, since
they are from the CERTCertificate structure itself and we wouldn't want to free
them twice.

The fix in this case I believe is to allocate the memory for the nickname
strings on the CERTCertList's arena. This will cause it to be freed when the
list is destroyed.

However, the pki code can't do that directly. This means that after we get a
nickname string from STAN_GetCERTCertificateNameForInstance or
STAN_GetCERTCertificateName, we must make a copy into the list's arena, using
PORT_ArenaStrdup, then free the STAN string using PORT_Free.

I will attach a patch to do that.
Comment 35 Julien Pierre 2003-09-03 16:21:55 PDT
Created attachment 130873 [details] [diff] [review]
incremental patch to fix memory leak of nicknames
Comment 36 Robert Relyea 2003-09-04 15:26:23 PDT
Comment on attachment 130873 [details] [diff] [review]
incremental patch to fix memory leak of nicknames

Though I think the code would be simpler if the temp variable was used on the
result of the GetNicknameFunction()...

char *tmpnickname;
char *nickname = NULL;
.
.
.
.
tmpnickname = STAN_GetCERTCertificateName(c);
if (tmpnickname) {
    nickname = PORT_ArenaStrdup(certList->arena,tmpnickname);
    PORT_Free(tmpnickname);
}


etc...
Comment 37 Julien Pierre 2003-09-04 17:16:53 PDT
Thanks, Bob. I checked in the memory leak fix as suggested.
Comment 38 Wan-Teh Chang 2003-09-04 19:10:11 PDT
Comment on attachment 130873 [details] [diff] [review]
incremental patch to fix memory leak of nicknames

We should modify the STAN_GetCERTCertificateName and
STAN_GetCERTCertificateNameForInstance functions to
take an arenaOpt argument.  If arenaOpt is NULL, they
allocate using PORT_Alloc.  If arenaOpt is not NULL,
they allocate from that arena.

It is wasteful to allocate with PORT_Alloc and free
the memory with PORT_Free immediately.

I am also worried that some of our clients may have
discovered this memory leak, determined (by studying
NSS source code) that the memory is allocated with
PORT_Alloc, and added code to free the nicknames with
PORT_Free.  (I was going to check in this workaround
into JSS.)  This patch would break these clients'
workaround.
Comment 39 Julien Pierre 2003-09-04 19:27:33 PDT
Wan-Teh,

The only client that I know ever used this appData nickname feature of
PK11_ListCerts is NES. It doesn't try to free the appData. In fact it didn't
even free the cert list until recently. The memory leaks were only checked for
in the request code path, and this occurred during the server configuration path.

So I wouldn't be worried about breaking clients.
The only other caller of STAN_GetCERTCertificateName, in certhigh.c, was already
aware that the nickname should be freed. Only the PK11_ListCerts callback had
the memory leak.

Adding a PRArenaPool* arenaOpt argument to the two STAN function would work, but
it is a STAN function so perhaps it shouldn't be using non-Stan PRArenaPool .
Comment 40 Wan-Teh Chang 2003-09-04 21:42:56 PDT
STAN_GetCERTCertificateName and STAN_GetCERTCertificateNameForInstance
are not Stan functions, but rather functions that bridge Stan and NSS
3.x.  They already call PORT_Alloc, so they can certainly call
PORT_ArenaAlloc 
Comment 41 Wan-Teh Chang 2003-09-05 07:48:40 PDT
Julien, it just occurred to me that I already filed bug 217247
for the memory leak of the appData in the cert list nodes returned
by PK11_ListCerts.  We should move the discussion of the memory
leak to that bug.

I'd like a general solution that doesn't just fix the case where
appData is the cert's nickname.  appData is a void *.  It can
point to anything, not just memory allocated with PORT_Alloc or
PORT_ArenaAlloc/PORT_ArenaStrdup.  The destruction of appData may
require more than just freeing memory.  For example, it may need
releasing a reference count if it points to a reference-counted
object.

(Re: your comment 39: PK11_ListCerts returns the nicknames in
appData whether the caller needs them or not.  So the memory leak
is not only seen by clients that actually use the returned appData
nicknames.  But I agree that it's unlikely any NSS client has
implemented a workaround for the memory leak without reporting it
to us.)

I'm going to mark this bug fixed in NSS 3.9 and 3.8.2.
Comment 42 Matthias Versen [:Matti] 2003-09-21 18:05:48 PDT
Comment on attachment 130873 [details] [diff] [review]
incremental patch to fix memory leak of nicknames

removing obsolete review requests

Note You need to log in before you can comment on or make changes to this bug.