Closed
Bug 72291
Opened 23 years ago
Closed 21 years ago
PK11_ListCerts skips duplicated keys/certs between tokens
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.9
People
(Reporter: julien.pierre, Assigned: julien.pierre)
References
Details
(Whiteboard: [3.8.2])
Attachments
(2 files, 2 obsolete files)
6.23 KB,
patch
|
rrelyea
:
review+
bugz
:
superreview+
|
Details | Diff | Splinter Review |
1.31 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Priority: -- → P1
Assignee | ||
Updated•23 years ago
|
Severity: major → critical
Comment 2•23 years ago
|
||
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•23 years ago
|
||
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
Assignee | ||
Comment 4•23 years ago
|
||
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•23 years ago
|
||
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 6•23 years ago
|
||
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.
Assignee | ||
Comment 7•23 years ago
|
||
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.
Assignee | ||
Comment 8•23 years ago
|
||
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•23 years ago
|
||
This should be fixed in 3.2.1
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•21 years ago
|
||
This appears to be broken again in current versions of NSS (3.7.x) as observed with CMS and NES.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•21 years ago
|
||
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•21 years ago
|
||
Ian, could you answer Julien's question at the end of comment 11? Thanks.
Assignee | ||
Comment 13•21 years ago
|
||
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.
Assignee | ||
Comment 14•21 years ago
|
||
hack
Comment 15•21 years ago
|
||
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•21 years ago
|
||
Assigned the bug to Julien. Target NSS 3.9.
Assignee: relyea → jpierre
Status: REOPENED → NEW
Priority: P1 → P2
Target Milestone: --- → 3.9
Assignee | ||
Comment 17•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 19•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 21•21 years ago
|
||
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•21 years ago
|
||
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?
Assignee | ||
Comment 23•21 years ago
|
||
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.
Assignee | ||
Comment 24•21 years ago
|
||
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 .
Assignee | ||
Comment 25•21 years ago
|
||
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).
Attachment #128759 -
Attachment is obsolete: true
Assignee | ||
Comment 26•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #129209 -
Flags: superreview?(nelsonb)
Attachment #129209 -
Flags: review?(wtc)
Comment 28•21 years ago
|
||
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.
Attachment #129209 -
Attachment is obsolete: true
Comment 29•21 years ago
|
||
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.
Attachment #130211 -
Flags: superreview?(bugz)
Attachment #130211 -
Flags: review?(jpierre)
Comment 30•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Comment on attachment 130211 [details] [diff] [review] new patch v1.1 (with wtc's edits) r=relyea
Attachment #130211 -
Flags: review?(jpierre) → review+
Comment 33•21 years ago
|
||
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.
Updated•21 years ago
|
Attachment #130211 -
Flags: superreview?(bugz) → superreview+
Assignee | ||
Comment 34•21 years ago
|
||
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.
Assignee | ||
Comment 35•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #130873 -
Flags: superreview?(wchang0222)
Attachment #130873 -
Flags: review?(relyea)
Comment 36•21 years ago
|
||
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...
Attachment #130873 -
Flags: review?(relyea) → review+
Assignee | ||
Comment 37•21 years ago
|
||
Thanks, Bob. I checked in the memory leak fix as suggested.
Comment 38•21 years ago
|
||
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.
Assignee | ||
Comment 39•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 23 years ago → 21 years ago
Resolution: --- → FIXED
Whiteboard: [3.8.2]
Comment 42•21 years ago
|
||
Comment on attachment 130873 [details] [diff] [review] incremental patch to fix memory leak of nicknames removing obsolete review requests
Attachment #130873 -
Flags: superreview?(wchang0222)
Updated•21 years ago
|
Attachment #129209 -
Flags: superreview?(MisterSSL)
Attachment #129209 -
Flags: review?(wchang0222)
You need to log in
before you can comment on or make changes to this bug.
Description
•