bug 1344478 - isAlreadyShutDown should return true for nsNSSShutDownObjects created after NSS shut down r?Cykesiopka,ttaubert draft
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 13 Mar 2017 15:26:40 -0700
changeset 499537 87820b0ad51b7a1f12faac68a84b481c5cbc7381
parent 499337 8c89d1991786625a64d868798281610872a2bc26
child 549388 9d3545cda0b89f90cb3094b81c7dbecc30356ede
push id49447
push userbmo:dkeeler@mozilla.com
push dateWed, 15 Mar 2017 23:15:21 +0000
reviewersCykesiopka, ttaubert
bugs1344478
milestone55.0a1
bug 1344478 - isAlreadyShutDown should return true for nsNSSShutDownObjects created after NSS shut down r?Cykesiopka,ttaubert MozReview-Commit-ID: 5bUTLz6mGKC In general, it is possible to create a new nsNSSShutDownObject after nsNSSShutDownList::shutdown() had been called. Before this patch, at that point, isAlreadyShutDown() would incorrectly return false, which could lead to code calling NSS functions, which would probably lead to a crash (because NSS could be uninitialized at that point). This change merges nsNSSShutDownList::shutdown() with evaporateAllNSSResources() into evaporateAllNSSResourcesAndShutDown() for simplicity and makes it so isAlreadyShutDown() returns true if called after that point.
security/manager/ssl/nsNSSComponent.cpp
security/manager/ssl/nsNSSShutDown.cpp
security/manager/ssl/nsNSSShutDown.h
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -192,17 +192,16 @@ nsNSSComponent::~nsNSSComponent()
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
   // All cleanup code requiring services needs to happen in xpcom_shutdown
 
   ShutdownNSS();
   SharedSSLState::GlobalCleanup();
   RememberCertErrorsTable::Cleanup();
   --mInstanceCount;
-  nsNSSShutDownList::shutdown();
 
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsNSSComponent::dtor finished\n"));
 }
 
 NS_IMETHODIMP
 nsNSSComponent::PIPBundleFormatStringFromName(const char* name,
                                               const char16_t** params,
                                               uint32_t numParams,
@@ -1804,17 +1803,17 @@ nsNSSComponent::ShutdownNSS()
     ShutdownSmartCardThreads();
 #endif
     SSL_ClearSessionCache();
     // TLSServerSocket may be run with the session cache enabled. This ensures
     // those resources are cleaned up.
     Unused << SSL_ShutdownServerSessionIDCache();
 
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("evaporating psm resources"));
-    if (NS_FAILED(nsNSSShutDownList::evaporateAllNSSResources())) {
+    if (NS_FAILED(nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown())) {
       MOZ_LOG(gPIPNSSLog, LogLevel::Error, ("failed to evaporate resources"));
       return;
     }
     UnloadLoadableRoots();
     if (SECSuccess != ::NSS_Shutdown()) {
       MOZ_LOG(gPIPNSSLog, LogLevel::Error, ("NSS SHUTDOWN FAILURE"));
     } else {
       MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("NSS shutdown =====>> OK <<====="));
--- a/security/manager/ssl/nsNSSShutDown.cpp
+++ b/security/manager/ssl/nsNSSShutDown.cpp
@@ -45,16 +45,18 @@ nsNSSShutDownList::nsNSSShutDownList()
   : mObjects(&gSetOps, sizeof(ObjectHashEntry))
   , mPK11LogoutCancelObjects(&gSetOps, sizeof(ObjectHashEntry))
 {
 }
 
 nsNSSShutDownList::~nsNSSShutDownList()
 {
   MOZ_ASSERT(this == singleton);
+  MOZ_ASSERT(sInShutdown,
+             "evaporateAllNSSResourcesAndShutDown() should have been called");
   singleton = nullptr;
 }
 
 void nsNSSShutDownList::remember(nsNSSShutDownObject *o)
 {
   StaticMutexAutoLock lock(sListLock);
   if (!nsNSSShutDownList::construct(lock)) {
     return;
@@ -121,17 +123,17 @@ nsresult nsNSSShutDownList::doPK11Logout
     if (pklco) {
       pklco->logout();
     }
   }
 
   return NS_OK;
 }
 
-nsresult nsNSSShutDownList::evaporateAllNSSResources()
+nsresult nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   if (!NS_IsMainThread()) {
     return NS_ERROR_NOT_SAME_THREAD;
   }
 
   StaticMutexAutoLock lock(sListLock);
   // Other threads can acquire an nsNSSShutDownPreventionLock and cause this
@@ -145,16 +147,18 @@ nsresult nsNSSShutDownList::evaporateAll
   // and destroyed on the main thread, and if we similarly enforce that this
   // function is only called on the main thread, what we can do is check that
   // the singleton hasn't already gone away and then we don't actually have to
   // hold sListLock while calling restrictActivityToCurrentThread.
   if (!singleton) {
     return NS_OK;
   }
 
+  sInShutdown = true;
+
   {
     StaticMutexAutoUnlock unlock(sListLock);
     PRStatus rv = singleton->mActivityState.restrictActivityToCurrentThread();
     if (rv != PR_SUCCESS) {
       MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
               ("failed to restrict activity to current thread"));
       return NS_ERROR_FAILURE;
     }
@@ -178,16 +182,18 @@ nsresult nsNSSShutDownList::evaporateAll
     iter.Remove();
   }
 
   if (!singleton) {
     return NS_ERROR_FAILURE;
   }
 
   singleton->mActivityState.releaseCurrentThreadActivityRestriction();
+  delete singleton;
+
   return NS_OK;
 }
 
 void nsNSSShutDownList::enterActivityState()
 {
   StaticMutexAutoLock lock(sListLock);
   if (nsNSSShutDownList::construct(lock)) {
     singleton->mActivityState.enter();
@@ -206,27 +212,16 @@ bool nsNSSShutDownList::construct(const 
 {
   if (!singleton && !sInShutdown && XRE_IsParentProcess()) {
     singleton = new nsNSSShutDownList();
   }
 
   return !!singleton;
 }
 
-void nsNSSShutDownList::shutdown()
-{
-  MOZ_RELEASE_ASSERT(NS_IsMainThread());
-  StaticMutexAutoLock lock(sListLock);
-  sInShutdown = true;
-
-  if (singleton) {
-    delete singleton;
-  }
-}
-
 nsNSSActivityState::nsNSSActivityState()
 :mNSSActivityStateLock("nsNSSActivityState.mNSSActivityStateLock"), 
  mNSSActivityChanged(mNSSActivityStateLock,
                      "nsNSSActivityState.mNSSActivityStateLock"),
  mNSSActivityCounter(0),
  mNSSRestrictedThread(nullptr)
 {
 }
@@ -281,8 +276,14 @@ nsNSSShutDownPreventionLock::nsNSSShutDo
 {
   nsNSSShutDownList::enterActivityState();
 }
 
 nsNSSShutDownPreventionLock::~nsNSSShutDownPreventionLock()
 {
   nsNSSShutDownList::leaveActivityState();
 }
+
+bool
+nsNSSShutDownObject::isAlreadyShutDown() const
+{
+  return mAlreadyShutDown || sInShutdown;
+}
--- a/security/manager/ssl/nsNSSShutDown.h
+++ b/security/manager/ssl/nsNSSShutDown.h
@@ -59,29 +59,28 @@ public:
   ~nsNSSShutDownPreventionLock();
 };
 
 // Singleton, used by nsNSSComponent to track the list of PSM objects,
 // which hold NSS resources and support the "early cleanup mechanism".
 class nsNSSShutDownList
 {
 public:
-  static void shutdown();
-
   // track instances that support early cleanup
   static void remember(nsNSSShutDownObject *o);
   static void forget(nsNSSShutDownObject *o);
 
   // track instances that would like notification when
   // a PK11 logout operation is performed.
   static void remember(nsOnPK11LogoutCancelObject *o);
   static void forget(nsOnPK11LogoutCancelObject *o);
 
-  // Do the "early cleanup", if possible.
-  static nsresult evaporateAllNSSResources();
+  // Release all tracked NSS resources and prevent nsNSSShutDownObjects from
+  // using NSS functions.
+  static nsresult evaporateAllNSSResourcesAndShutDown();
 
   // PSM has been asked to log out of a token.
   // Notify all registered instances that want to react to that event.
   static nsresult doPK11Logout();
 
   // Signal entering/leaving a scope where shutting down NSS is prohibited.
   static void enterActivityState();
   static void leaveActivityState();
@@ -223,17 +222,17 @@ public:
           break;
         default:
           MOZ_CRASH("shutdown() called from an unknown source");
       }
       mAlreadyShutDown = true;
     }
   }
 
-  bool isAlreadyShutDown() const { return mAlreadyShutDown; }
+  bool isAlreadyShutDown() const;
 
 protected:
   virtual void virtualDestroyNSSReference() = 0;
 private:
   volatile bool mAlreadyShutDown;
 };
 
 class nsOnPK11LogoutCancelObject