diff --git a/security/manager/ssl/nsNSSShutDown.cpp b/security/manager/ssl/nsNSSShutDown.cpp index 984af62a89f4..06395e282ff3 100644 --- a/security/manager/ssl/nsNSSShutDown.cpp +++ b/security/manager/ssl/nsNSSShutDown.cpp @@ -135,12 +135,17 @@ nsresult nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown() return NS_ERROR_NOT_SAME_THREAD; } + // This makes this function idempotent and protects against reentering it + // (which really shouldn't happen anyway, but just in case). + if (sInShutdown) { + return NS_OK; + } + StaticMutexAutoLock lock(sListLock); // Other threads can acquire an nsNSSShutDownPreventionLock and cause this // thread to block when it calls restructActivityToCurrentThread, below. If - // those other threads then attempt to create an object that must be - // remembered by the shut down list, they will call - // nsNSSShutDownList::remember, which attempts to acquire sListLock. + // those other threads then attempt to create an nsNSSShutDownObject, they + // will call nsNSSShutDownList::remember, which attempts to acquire sListLock. // Consequently, holding sListLock while we're in // restrictActivityToCurrentThread would result in deadlock. sListLock // protects the singleton, so if we enforce that the singleton only be created @@ -152,6 +157,11 @@ nsresult nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown() return NS_OK; } + // Setting sInShutdown here means that threads calling + // nsNSSShutDownList::remember will return early (because + // nsNSSShutDownList::construct will return false) and not attempt to remember + // new objects. If they properly check isAlreadyShutDown(), they will also not + // attempt to call NSS functions or use NSS resources. sInShutdown = true; { @@ -165,38 +175,24 @@ nsresult nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown() } MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("now evaporating NSS resources")); - - // Never free more than one entry, because other threads might be calling - // us and remove themselves while we are iterating over the list, - // and the behaviour of changing the list while iterating is undefined. - while (singleton) { - auto iter = singleton->mObjects.Iter(); - if (iter.Done()) { - break; - } + for (auto iter = singleton->mObjects.Iter(); !iter.Done(); iter.Next()) { auto entry = static_cast(iter.Get()); - { - StaticMutexAutoUnlock unlock(sListLock); - entry->obj->shutdown(nsNSSShutDownObject::ShutdownCalledFrom::List); - } + entry->obj->shutdown(nsNSSShutDownObject::ShutdownCalledFrom::List); iter.Remove(); } - if (!singleton) { - return NS_ERROR_FAILURE; - } - singleton->mActivityState.releaseCurrentThreadActivityRestriction(); delete singleton; return NS_OK; } -void nsNSSShutDownList::enterActivityState() +void nsNSSShutDownList::enterActivityState(/*out*/ bool& enteredActivityState) { StaticMutexAutoLock lock(sListLock); if (nsNSSShutDownList::construct(lock)) { singleton->mActivityState.enter(); + enteredActivityState = true; } } @@ -210,7 +206,11 @@ void nsNSSShutDownList::leaveActivityState() bool nsNSSShutDownList::construct(const StaticMutexAutoLock& /*proofOfLock*/) { - if (!singleton && !sInShutdown && XRE_IsParentProcess()) { + if (sInShutdown) { + return false; + } + + if (!singleton && XRE_IsParentProcess()) { singleton = new nsNSSShutDownList(); } @@ -273,13 +273,16 @@ void nsNSSActivityState::releaseCurrentThreadActivityRestriction() } nsNSSShutDownPreventionLock::nsNSSShutDownPreventionLock() + : mEnteredActivityState(false) { - nsNSSShutDownList::enterActivityState(); + nsNSSShutDownList::enterActivityState(mEnteredActivityState); } nsNSSShutDownPreventionLock::~nsNSSShutDownPreventionLock() { - nsNSSShutDownList::leaveActivityState(); + if (mEnteredActivityState) { + nsNSSShutDownList::leaveActivityState(); + } } bool diff --git a/security/manager/ssl/nsNSSShutDown.h b/security/manager/ssl/nsNSSShutDown.h index 60fb437412bf..c3e164f39d64 100644 --- a/security/manager/ssl/nsNSSShutDown.h +++ b/security/manager/ssl/nsNSSShutDown.h @@ -57,6 +57,12 @@ class nsNSSShutDownPreventionLock public: nsNSSShutDownPreventionLock(); ~nsNSSShutDownPreventionLock(); +private: + // Keeps track of whether or not we actually managed to enter the NSS activity + // state. This is important because if we're attempting to shut down and we + // can't enter the NSS activity state, we need to not attempt to leave it when + // our destructor runs. + bool mEnteredActivityState; }; // Singleton, used by nsNSSComponent to track the list of PSM objects, @@ -82,7 +88,9 @@ public: static nsresult doPK11Logout(); // Signal entering/leaving a scope where shutting down NSS is prohibited. - static void enterActivityState(); + // enteredActivityState will be set to true if we actually managed to enter + // the NSS activity state. + static void enterActivityState(/*out*/ bool& enteredActivityState); static void leaveActivityState(); private: