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 119969 c4caff0299e7e81ac76f604af4a5f53888841196
parent 119968 05117f42088f9aef01120e89c6058cfc7a91de24
child 119970 bdac595a4e46f347b04d90a2794280e6ee54f670
push id1105
push userryanvm@gmail.com
push dateTue, 10 Dec 2013 14:01:32 +0000
reviewersbriansmith, dveditz
bugs936808
milestone18.1
Bug 936808 - Serialize calls to PK11 routines in SSLServerCertVerification. r=briansmith, a=dveditz
security/manager/ssl/src/SSLServerCertVerification.cpp
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -126,31 +126,36 @@ namespace {
 
 NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);
 
 NSSCleanupAutoPtrClass(CERTCertificate, CERT_DestroyCertificate)
 NSSCleanupAutoPtrClass_WithParam(PRArenaPool, PORT_FreeArena, FalseParam, false)
 
 // do not use a nsCOMPtr to avoid static initializer/destructor
 nsIThreadPool * gCertVerificationThreadPool = 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.
 void
 InitializeSSLServerCertVerificationThreads()
 {
+  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,
                                    &gCertVerificationThreadPool);
   if (NS_FAILED(rv)) {
     NS_WARNING("Failed to create SSL cert verification threads.");
     return;
@@ -173,16 +178,20 @@ InitializeSSLServerCertVerificationThrea
 // shutdown of the nsNSSComponent service. We use the
 // nsNSSShutdownPreventionLock where needed (not here) to prevent that.
 void StopSSLServerCertVerificationThreads()
 {
   if (gCertVerificationThreadPool) {
     gCertVerificationThreadPool->Shutdown();
     NS_RELEASE(gCertVerificationThreadPool);
   }
+  if (gSSLVerificationPK11Mutex) {
+    delete gSSLVerificationPK11Mutex;
+    gSSLVerificationPK11Mutex = nullptr;
+  }
 }
 
 namespace {
 
 void
 LogInvalidCertError(TransportSecurityInfo *socketInfo, 
                     const nsACString &host, 
                     const nsACString &hostWithPort,
@@ -939,16 +948,20 @@ AuthCertificate(TransportSecurityInfo * 
         // We don't want to remember the server cert, 
         // the code that cares for displaying page info does this already.
         continue;
       }
 
       // 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);
         PK11SlotInfo *slot = PK11_GetInternalKeySlot();
         if (slot) {
           PK11_ImportCert(slot, node->cert, CK_INVALID_HANDLE, 
                           nickname, false);
           PK11_FreeSlot(slot);
         }
       }
       PR_FREEIF(nickname);