Bug 1278965 - tsan race in CERTCertificate, r=wtc,ttaubert

4 files changed, 116 insertions, 17 deletions

diff --git a/lib/certdb/cert.h b/lib/certdb/cert.h
index e0af65ab0..4224da108 100644
--- a/lib/certdb/cert.h
+++ b/lib/certdb/cert.h
@@ -1405,24 +1405,11 @@ 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
  * the tag, length, and number of unused bits) rather than the whole
@@ -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_ */
diff --git a/lib/certdb/certdb.c b/lib/certdb/certdb.c
index 19c99ee90..7864edc08 100644
--- a/lib/certdb/certdb.c
+++ b/lib/certdb/certdb.c
@@ -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
@@ -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;
 }
 
@@ -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;
 }
 
@@ -2934,6 +2965,23 @@ CERT_UnlockCertTrust(const CERTCertificate *cert)
 }
 
 /*
+ * 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
  */
 CERTStatusConfig *
diff --git a/lib/certdb/certi.h b/lib/certdb/certi.h
index 1cdf4b8fa..456f2fc4e 100644
--- a/lib/certdb/certi.h
+++ b/lib/certdb/certi.h
@@ -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_ */
diff --git a/lib/certdb/stanpcertdb.c b/lib/certdb/stanpcertdb.c
index 3d7275df8..ac05d3c98 100644
--- a/lib/certdb/stanpcertdb.c
+++ b/lib/certdb/stanpcertdb.c
@@ -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;
@@ -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;
     }
@@ -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) */
@@ -911,6 +915,7 @@ CERT_SaveSMimeProfile(CERTCertificate *cert, SECItem *emailProfile,
 {
     const char *emailAddr;
     SECStatus rv;
+    PRBool isperm = PR_FALSE;
 
     if (!cert) {
         return SECFailure;
@@ -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;
@@ -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.
  */