Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1278965 - tsan race in CERTCertificate, r=wtc,ttaubert
--HG--
extra : rebase_source : 284827ba896a18c7268a530c1ad83e19a30ec24d
extra : amend_source : 828edfcd42c93ff935904cda040bcbbe46d81c4a
  • Loading branch information
franziskuskiefer committed Feb 8, 2017
1 parent 74ac2a7 commit 903787c
Show file tree
Hide file tree
Showing 8 changed files with 134 additions and 21 deletions.
21 changes: 7 additions & 14 deletions lib/certdb/cert.h
Expand Up @@ -1405,23 +1405,10 @@ void CERT_SetStatusConfig(CERTCertDBHandle *handle, CERTStatusConfig *config);
void CERT_LockCertRefCount(CERTCertificate *cert);

/*
* Free the cert reference count lock
* Release the cert reference count lock
*/
void CERT_UnlockCertRefCount(CERTCertificate *cert);

/*
* Acquire the cert trust lock
* There is currently one global lock for all certs, but I'm putting a cert
* arg here so that it will be easy to make it per-cert in the future if
* that turns out to be necessary.
*/
void CERT_LockCertTrust(const CERTCertificate *cert);

/*
* Free the cert trust lock
*/
void CERT_UnlockCertTrust(const CERTCertificate *cert);

/*
* Digest the cert's subject public key using the specified algorithm.
* NOTE: this digests the value of the BIT STRING subjectPublicKey (excluding
Expand Down Expand Up @@ -1579,6 +1566,12 @@ extern CERTRevocationFlags *CERT_AllocCERTRevocationFlags(
*/
extern void CERT_DestroyCERTRevocationFlags(CERTRevocationFlags *flags);

/*
* Get istemp and isperm fields from a cert in a thread safe way.
*/
extern SECStatus CERT_GetCertIsTemp(const CERTCertificate *cert, PRBool *istemp);
extern SECStatus CERT_GetCertIsPerm(const CERTCertificate *cert, PRBool *isperm);

SEC_END_PROTOS

#endif /* _CERT_H_ */
50 changes: 49 additions & 1 deletion lib/certdb/certdb.c
Expand Up @@ -2865,7 +2865,18 @@ CERT_LockCertTrust(const CERTCertificate *cert)
{
PORT_Assert(certTrustLock != NULL);
PZ_Lock(certTrustLock);
return;
}

static PZLock *certTempPermLock = NULL;

/*
* Acquire the cert temp/perm lock
*/
void
CERT_LockCertTempPerm(const CERTCertificate *cert)
{
PORT_Assert(certTempPermLock != NULL);
PZ_Lock(certTempPermLock);
}

SECStatus
Expand All @@ -2889,6 +2900,18 @@ cert_InitLocks(void)
}
}

if (certTempPermLock == NULL) {
certTempPermLock = PZ_NewLock(nssILockCertDB);
PORT_Assert(certTempPermLock != NULL);
if (!certTempPermLock) {
PZ_DestroyLock(certTrustLock);
PZ_DestroyLock(certRefCountLock);
certRefCountLock = NULL;
certTrustLock = NULL;
return SECFailure;
}
}

return SECSuccess;
}

Expand All @@ -2912,6 +2935,14 @@ cert_DestroyLocks(void)
} else {
rv = SECFailure;
}

PORT_Assert(certTempPermLock != NULL);
if (certTempPermLock) {
PZ_DestroyLock(certTempPermLock);
certTempPermLock = NULL;
} else {
rv = SECFailure;
}
return rv;
}

Expand All @@ -2933,6 +2964,23 @@ CERT_UnlockCertTrust(const CERTCertificate *cert)
#endif
}

/*
* Free the temp/perm lock
*/
void
CERT_UnlockCertTempPerm(const CERTCertificate *cert)
{
PORT_Assert(certTempPermLock != NULL);
#ifdef DEBUG
{
PRStatus prstat = PZ_Unlock(certTempPermLock);
PORT_Assert(prstat == PR_SUCCESS);
}
#else
(void)PZ_Unlock(certTempPermLock);
#endif
}

/*
* Get the StatusConfig data for this handle
*/
Expand Down
23 changes: 23 additions & 0 deletions lib/certdb/certi.h
Expand Up @@ -378,4 +378,27 @@ PRUint32 cert_CountDNSPatterns(CERTGeneralName* firstName);
SECStatus cert_CheckLeafTrust(CERTCertificate* cert, SECCertUsage usage,
unsigned int* failedFlags, PRBool* isTrusted);

/*
* Acquire the cert temp/perm lock
*/
void CERT_LockCertTempPerm(const CERTCertificate* cert);

/*
* Release the temp/perm lock
*/
void CERT_UnlockCertTempPerm(const CERTCertificate* cert);

/*
* Acquire the cert trust lock
* There is currently one global lock for all certs, but I'm putting a cert
* arg here so that it will be easy to make it per-cert in the future if
* that turns out to be necessary.
*/
void CERT_LockCertTrust(const CERTCertificate* cert);

/*
* Release the cert trust lock
*/
void CERT_UnlockCertTrust(const CERTCertificate* cert);

#endif /* _CERTI_H_ */
39 changes: 37 additions & 2 deletions lib/certdb/stanpcertdb.c
Expand Up @@ -91,7 +91,7 @@ CERT_GetCertTrust(const CERTCertificate *cert, CERTCertTrust *trust)
{
SECStatus rv;
CERT_LockCertTrust(cert);
if (cert->trust == NULL) {
if (!cert || cert->trust == NULL) {
rv = SECFailure;
} else {
*trust = *cert->trust;
Expand Down Expand Up @@ -304,8 +304,10 @@ __CERT_AddTempCertToPerm(CERTCertificate *cert, char *nickname,
CERT_MapStanError();
return SECFailure;
}
CERT_LockCertTempPerm(cert);
cert->istemp = PR_FALSE;
cert->isperm = PR_TRUE;
CERT_UnlockCertTempPerm(cert);
if (!trust) {
return SECSuccess;
}
Expand Down Expand Up @@ -436,8 +438,10 @@ CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert,
return NULL;
}

CERT_LockCertTempPerm(cc);
cc->istemp = PR_TRUE;
cc->isperm = PR_FALSE;
CERT_UnlockCertTempPerm(cc);
return cc;
loser:
/* Perhaps this should be nssCertificate_Destroy(c) */
Expand Down Expand Up @@ -911,6 +915,7 @@ CERT_SaveSMimeProfile(CERTCertificate *cert, SECItem *emailProfile,
{
const char *emailAddr;
SECStatus rv;
PRBool isperm = PR_FALSE;

if (!cert) {
return SECFailure;
Expand All @@ -932,7 +937,11 @@ CERT_SaveSMimeProfile(CERTCertificate *cert, SECItem *emailProfile,
}
}

if (cert->slot && cert->isperm && CERT_IsUserCert(cert) &&
rv = CERT_GetCertIsPerm(cert, &isperm);
if (rv != SECSuccess) {
return SECFailure;
}
if (cert->slot && isperm && CERT_IsUserCert(cert) &&
(!emailProfile || !emailProfile->len)) {
/* Don't clobber emailProfile for user certs. */
return SECSuccess;
Expand Down Expand Up @@ -986,6 +995,32 @@ CERT_FindSMimeProfile(CERTCertificate *cert)
return rvItem;
}

SECStatus
CERT_GetCertIsPerm(const CERTCertificate *cert, PRBool *isperm)
{
if (cert == NULL) {
return SECFailure;
}

CERT_LockCertTempPerm(cert);
*isperm = cert->isperm;
CERT_LockCertTempPerm(cert);
return SECSuccess;
}

SECStatus
CERT_GetCertIsTemp(const CERTCertificate *cert, PRBool *istemp)
{
if (cert == NULL) {
return SECFailure;
}

CERT_LockCertTempPerm(cert);
*istemp = cert->istemp;
CERT_LockCertTempPerm(cert);
return SECSuccess;
}

/*
* deprecated functions that are now just stubs.
*/
Expand Down
11 changes: 7 additions & 4 deletions lib/certhigh/certhigh.c
Expand Up @@ -11,6 +11,7 @@
#include "cert.h"
#include "certxutl.h"

#include "certi.h"
#include "nsspki.h"
#include "pki.h"
#include "pkit.h"
Expand Down Expand Up @@ -872,6 +873,7 @@ cert_ImportCAChain(SECItem *certs, int numcerts, SECCertUsage certUsage, PRBool
PRBool isca;
char *nickname;
unsigned int certtype;
PRBool istemp = PR_FALSE;

handle = CERT_GetDefaultCertDB();

Expand Down Expand Up @@ -949,7 +951,11 @@ cert_ImportCAChain(SECItem *certs, int numcerts, SECCertUsage certUsage, PRBool
}

/* if the cert is temp, make it perm; otherwise we're done */
if (cert->istemp) {
rv = CERT_GetCertIsTemp(cert, &istemp);
if (rv != SECSuccess) {
goto loser;
}
if (istemp) {
/* get a default nickname for it */
nickname = CERT_MakeCANickname(cert);

Expand All @@ -963,9 +969,6 @@ cert_ImportCAChain(SECItem *certs, int numcerts, SECCertUsage certUsage, PRBool
rv = SECSuccess;
}

CERT_DestroyCertificate(cert);
cert = NULL;

if (rv != SECSuccess) {
goto loser;
}
Expand Down
7 changes: 7 additions & 0 deletions lib/nss/nss.def
Expand Up @@ -1104,3 +1104,10 @@ PK11_HasAttributeSet;
;+ local:
;+ *;
;+};
;+NSS_3.31 { # NSS 3.31 release
;+ global:
CERT_GetCertIsPerm;
CERT_GetCertIsTemp;
;+ local:
;+ *;
;+};
2 changes: 2 additions & 0 deletions lib/pk11wrap/pk11cert.c
Expand Up @@ -974,8 +974,10 @@ PK11_ImportCert(PK11SlotInfo *slot, CERTCertificate *cert,
nssCertificateStore_RemoveCertLOCKED(cc->certStore, c);
nssCertificateStore_Unlock(cc->certStore, &lockTrace, &unlockTrace);
c->object.cryptoContext = NULL;
CERT_LockCertTempPerm(cert);
cert->istemp = PR_FALSE;
cert->isperm = PR_TRUE;
CERT_UnlockCertTempPerm(cert);
}

/* add the new instance to the cert, force an update of the
Expand Down
2 changes: 2 additions & 0 deletions lib/pki/pki3hack.c
Expand Up @@ -831,8 +831,10 @@ fill_CERTCertificateFields(NSSCertificate *c, CERTCertificate *cc, PRBool forced
cc->dbhandle = c->object.trustDomain;
/* subjectList ? */
/* istemp and isperm are supported in NSS 3.4 */
CERT_LockCertTempPerm(cc);
cc->istemp = PR_FALSE; /* CERT_NewTemp will override this */
cc->isperm = PR_TRUE; /* by default */
CERT_UnlockCertTempPerm(cc);
/* pointer back */
cc->nssCertificate = c;
if (trust) {
Expand Down

0 comments on commit 903787c

Please sign in to comment.