Bug 936808 - Serialize calls to PK11 routines in SSLServerCertVerification. r=briansmith, a=dveditz
authorCamilo Viecco <cviecco@mozilla.com>
Mon, 09 Dec 2013 09:12:47 -0800
changeset 175193 3c88ff023416dcbba54ac3cd6fa6f6c99d53330c
parent 175192 a1acfd0cc6b978404a1955e6ccb0ce1931646dc9
child 175194 10fe7a0cd2532bfd8d5c71f639ceb18652b80151
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith, dveditz
Bug 936808 - Serialize calls to PK11 routines in SSLServerCertVerification. r=briansmith, a=dveditz
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -139,32 +139,36 @@ NSSCleanupAutoPtrClass_WithParam(PLArena
 // do not use a nsCOMPtr to avoid static initializer/destructor
 nsIThreadPool * gCertVerificationThreadPool = nullptr;
 // We avoid using a mutex for the success case to avoid lock-related
 // performance issues. However, we do use a lock in the error case to simplify
 // the code, since performance in the error case is not important.
 Mutex *gSSLVerificationTelemetryMutex = nullptr;
+// We add a mutex to serialize PKCS11 database operations
+Mutex *gSSLVerificationPK11Mutex = nullptr;
 } // unnamed namespace
 // Called when the socket transport thread starts, to initialize the SSL cert
 // verification thread pool. By tying the thread pool startup/shutdown directly
 // to the STS thread's lifetime, we ensure that they are *always* available for
 // SSL connections and that there are no races during startup and especially
 // shutdown. (Previously, we have had multiple problems with races in PSM
 // background threads, and the race-prevention/shutdown logic used there is
 // brittle. Since this service is critical to things like downloading updates,
 // we take no chances.) Also, by doing things this way, we avoid the need for
 // locks, since gCertVerificationThreadPool is only ever accessed on the socket
 // transport thread.
   gSSLVerificationTelemetryMutex = new Mutex("SSLVerificationTelemetryMutex");
+  gSSLVerificationPK11Mutex = new Mutex("SSLVerificationPK11Mutex");
   // TODO: tuning, make parameters preferences
   // XXX: instantiate nsThreadPool directly, to make this more bulletproof.
   // Currently, the nsThreadPool.h header isn't exported for us to do so.
   nsresult rv = CallCreateInstance(NS_THREADPOOL_CONTRACTID,
   if (NS_FAILED(rv)) {
     NS_WARNING("Failed to create SSL cert verification threads.");
@@ -191,16 +195,20 @@ void StopSSLServerCertVerificationThread
   if (gCertVerificationThreadPool) {
   if (gSSLVerificationTelemetryMutex) {
     delete gSSLVerificationTelemetryMutex;
     gSSLVerificationTelemetryMutex = nullptr;
+  if (gSSLVerificationPK11Mutex) {
+    delete gSSLVerificationPK11Mutex;
+    gSSLVerificationPK11Mutex = nullptr;
+  }
 namespace {
 LogInvalidCertError(TransportSecurityInfo *socketInfo, 
                     const nsACString &host, 
                     const nsACString &hostWithPort,
@@ -974,16 +982,20 @@ AuthCertificate(TransportSecurityInfo * 
           // We don't want to remember the server cert, 
           // the code that cares for displaying page info does this already.
         // We have found a signer cert that we want to remember.
         char* nickname = nsNSSCertificate::defaultServerNickname(node->cert);
         if (nickname && *nickname) {
+          // There is a suspicion that there is some thread safety issues
+          // in PK11_importCert and the mutex is a way to serialize until
+          // this issue has been cleared.
+          MutexAutoLock PK11Mutex(*gSSLVerificationPK11Mutex);
           ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
           if (slot) {
             PK11_ImportCert(slot, node->cert, CK_INVALID_HANDLE, 
                             nickname, false);