Potential deadlocks in extreme edge case - closes #609

abumq · Feb 14, 2018 · 7679ace · 7679ace
1 parent c06db0f
commit 7679ace
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Change Log
 
+## [Unreleased]
+### Fixes
+- Potential deadlocks in extreme edge case #609
+
 ## [9.95.4] - 10-02-2018
 ### Fixes
 - Fix documentation (see PR#597)

diff --git a/src/easylogging++.cc b/src/easylogging++.cc
@@ -1840,8 +1840,10 @@ bool RegisteredLoggers::remove(const std::string& id) {
   if (id == base::consts::kDefaultLoggerId) {
     return false;
   }
+  // get has internal lock
   Logger* logger = base::utils::Registry<Logger, std::string>::get(id);
   if (logger != nullptr) {
+    // unregister has internal lock
     unregister(logger);
   }
   return true;
@@ -2046,7 +2048,7 @@ Storage::~Storage(void) {
 }
 
 bool Storage::hasCustomFormatSpecifier(const char* formatSpecifier) {
-  base::threading::ScopedLock scopedLock(lock());
+  base::threading::ScopedLock scopedLock(customFormatSpecifiersLock());
   return std::find(m_customFormatSpecifiers.begin(), m_customFormatSpecifiers.end(),
                    formatSpecifier) != m_customFormatSpecifiers.end();
 }
@@ -2055,12 +2057,12 @@ void Storage::installCustomFormatSpecifier(const CustomFormatSpecifier& customFo
   if (hasCustomFormatSpecifier(customFormatSpecifier.formatSpecifier())) {
     return;
   }
-  base::threading::ScopedLock scopedLock(lock());
+  base::threading::ScopedLock scopedLock(customFormatSpecifiersLock());
   m_customFormatSpecifiers.push_back(customFormatSpecifier);
 }
 
 bool Storage::uninstallCustomFormatSpecifier(const char* formatSpecifier) {
-  base::threading::ScopedLock scopedLock(lock());
+  base::threading::ScopedLock scopedLock(customFormatSpecifiersLock());
   std::vector<CustomFormatSpecifier>::iterator it = std::find(m_customFormatSpecifiers.begin(),
       m_customFormatSpecifiers.end(), formatSpecifier);
   if (it != m_customFormatSpecifiers.end() && strcmp(formatSpecifier, it->formatSpecifier()) == 0) {
@@ -2349,7 +2351,7 @@ base::type::string_t DefaultLogBuilder::build(const LogMessage* logMessage, bool
     base::utils::Str::replaceFirstWithEscape(logLine, base::consts::kMessageFormatSpecifier, logMessage->message());
   }
 #if !defined(ELPP_DISABLE_CUSTOM_FORMAT_SPECIFIERS)
-  el::base::threading::ScopedLock lock_(ELPP->lock());
+  el::base::threading::ScopedLock lock_(ELPP->customFormatSpecifiersLock());
   ELPP_UNUSED(lock_);
   for (std::vector<CustomFormatSpecifier>::const_iterator it = ELPP->customFormatSpecifiers()->begin();
        it != ELPP->customFormatSpecifiers()->end(); ++it) {
@@ -2448,7 +2450,6 @@ void Writer::initializeLogger(const std::string& loggerId, bool lookup, bool nee
   }
   if (m_logger == nullptr) {
     {
-      base::threading::ScopedLock scopedLock(ELPP->lock());
       if (!ELPP->registeredLoggers()->has(std::string(base::consts::kDefaultLoggerId))) {
         // Somehow default logger has been unregistered. Not good! Register again
         ELPP->registeredLoggers()->get(std::string(base::consts::kDefaultLoggerId));
@@ -2824,7 +2825,6 @@ void Helpers::logCrashReason(int sig, bool stackTraceIfAvailable, Level level, c
 // Loggers
 
 Logger* Loggers::getLogger(const std::string& identity, bool registerIfNotAvailable) {
-  base::threading::ScopedLock scopedLock(ELPP->lock());
   return ELPP->registeredLoggers()->get(identity, registerIfNotAvailable);
 }
 
@@ -2833,12 +2833,10 @@ void Loggers::setDefaultLogBuilder(el::LogBuilderPtr& logBuilderPtr) {
 }
 
 bool Loggers::unregisterLogger(const std::string& identity) {
-  base::threading::ScopedLock scopedLock(ELPP->lock());
   return ELPP->registeredLoggers()->remove(identity);
 }
 
 bool Loggers::hasLogger(const std::string& identity) {
-  base::threading::ScopedLock scopedLock(ELPP->lock());
   return ELPP->registeredLoggers()->has(identity);
 }
 

diff --git a/src/easylogging++.h b/src/easylogging++.h
@@ -2601,7 +2601,7 @@ class IWorker {
 };
 #endif // ELPP_ASYNC_LOGGING
 /// @brief Easylogging++ management storage
-class Storage : base::NoCopy, public base::threading::ThreadSafe {
+class Storage : base::NoCopy {
  public:
 #if ELPP_ASYNC_LOGGING
   Storage(const LogBuilderPtr& defaultLogBuilder, base::IWorker* asyncDispatchWorker);
@@ -2685,6 +2685,10 @@ class Storage : base::NoCopy, public base::threading::ThreadSafe {
     return &m_customFormatSpecifiers;
   }
 
+  base::threading::Mutex& customFormatSpecifiersLock() {
+      return m_customFormatSpecifiersLock;
+  }
+
   inline void setLoggingLevel(Level level) {
     m_loggingLevel = level;
   }
@@ -2725,11 +2729,12 @@ class Storage : base::NoCopy, public base::threading::ThreadSafe {
   /// @brief Sets thread name for current thread. Requires std::thread
   inline void setThreadName(const std::string& name) {
     if (name.empty()) return;
-    base::threading::ScopedLock scopedLock(lock());
+    base::threading::ScopedLock scopedLock(m_threadNamesLock);
     m_threadNames[base::threading::getCurrentThreadId()] = name;
   }
 
   inline std::string getThreadName(const std::string& threadId) {
+    base::threading::ScopedLock scopedLock(m_threadNamesLock);
     std::map<std::string, std::string>::const_iterator it = m_threadNames.find(threadId);
     if (it == m_threadNames.end()) {
       return threadId;
@@ -2751,6 +2756,8 @@ class Storage : base::NoCopy, public base::threading::ThreadSafe {
   std::map<std::string, base::type::PerformanceTrackingCallbackPtr> m_performanceTrackingCallbacks;
   std::map<std::string, std::string> m_threadNames;
   std::vector<CustomFormatSpecifier> m_customFormatSpecifiers;
+  base::threading::Mutex m_customFormatSpecifiersLock;
+  base::threading::Mutex m_threadNamesLock;
   Level m_loggingLevel;
 
   friend class el::Helpers;
@@ -2793,7 +2800,7 @@ class AsyncDispatchWorker : public base::IWorker, public base::threading::Thread
   void run(void);
 
   void setContinueRunning(bool value) {
-    base::threading::ScopedLock scopedLock(m_continueRunningMutex);
+    base::threading::ScopedLock scopedLock(m_continueRunningLock);
     m_continueRunning = value;
   }
 
@@ -2803,7 +2810,7 @@ class AsyncDispatchWorker : public base::IWorker, public base::threading::Thread
  private:
   std::condition_variable cv;
   bool m_continueRunning;
-  base::threading::Mutex m_continueRunningMutex;
+  base::threading::Mutex m_continueRunningLock;
 };
 #endif  // ELPP_ASYNC_LOGGING
 }  // namespace base