When NSSTrustDomain_FindTokenByName() returns 'tok', what guarantees that it still exists, the moment after the locks are dropped?
Aren't we supposed to use nssToken_AddRef() before dropping the locks, to ensure that it stays around long enough for our caller to actually use it? So we'd want something like the patch below? I ask because we're adding a new NSSTrustDomain_FindTokensByURI() function and won't want to make the same mistake. I've been drumming into Varun that he needs to make his new functions look at much like *existing* similar functions as possible... but perhaps *not* to the extent of preserving their bugs :) diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c index 1b9be63..e4443c5 100644 --- a/lib/pk11wrap/pk11cert.c +++ b/lib/pk11wrap/pk11cert.c @@ -731,20 +731,23 @@ find_certs_from_nickname(const char *nickname, void *wincx) if ((delimit = PORT_Strchr(nickCopy,':')) != NULL) { tokenName = nickCopy; nickname = delimit + 1; *delimit = '\0'; /* find token by name */ + /* We got a ref on the token here... */ token = NSSTrustDomain_FindTokenByName(defaultTD, (NSSUTF8 *)tokenName); if (token) { slot = PK11_ReferenceSlot(token->pk11slot); } else { PORT_SetError(SEC_ERROR_NO_TOKEN); } *delimit = ':'; } else { slot = PK11_GetInternalKeySlot(); token = PK11Slot_GetNSSToken(slot); + /* ... so we need one here too... */ + nssToken_AddRef(token); } if (token) { nssList *certList; nssCryptokiObject **instances; nssTokenSearchType tokenOnly = nssTokenSearchType_TokenOnly; @@ -803,10 +806,14 @@ find_certs_from_nickname(const char *nickname, void *wincx) } loser: if (collection) { nssPKIObjectCollection_Destroy(collection); } + if (token) { + /* ... and we drop it here */ + nssToken_Destroy(token); + } if (slot) { PK11_FreeSlot(slot); } if (nickCopy) PORT_Free(nickCopy); return certs; diff --git a/lib/pki/trustdomain.c b/lib/pki/trustdomain.c index 87ea6da..dd72347 100644 --- a/lib/pki/trustdomain.c +++ b/lib/pki/trustdomain.c @@ -294,11 +294,14 @@ NSSTrustDomain_FindTokenByName ( tok != (NSSToken *)NULL; tok = (NSSToken *)nssListIterator_Next(td->tokens)) { if (nssToken_IsPresent(tok)) { myName = nssToken_GetName(tok); - if (nssUTF8_Equal(tokenName, myName, &nssrv)) break; + if (nssUTF8_Equal(tokenName, myName, &nssrv)) { + tok = nssToken_AddRef(tok); + break; + } } } nssListIterator_Finish(td->tokens); NSSRWLock_UnlockRead(td->tokensLock); return tok; (Note that there is no find_certs_from_nickname() in HEAD at the moment; there are two almost identical copies of the same code, duplicated in PK11_FindCertsFromNickname() and PK11_FindCertFromNickname() with the occasional bug fix applied to one but not the other. I fixed that already in my tree and will submit that cleanup separately. Again, I didn't want Varun doing the same thing with the equivalent FromURI() functions.) -- David Woodhouse Open Source Technology Centre david.woodho...@intel.com Intel Corporation
smime.p7s
Description: S/MIME cryptographic signature
-- dev-tech-crypto mailing list dev-tech-crypto@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-tech-crypto