CPLCreateOrAcquireMutexEx(): fix TSAN/valgrind --tool=helgrind warnin…

…g about lock-order inversion (fixes OSGeo#1108)
rouault · Feb 19, 2024 · a74c1ee · a74c1ee
1 parent e7f14b7
commit a74c1ee
Showing 1 changed file with 29 additions and 31 deletions.
diff --git a/port/cpl_multiproc.cpp b/port/cpl_multiproc.cpp
@@ -318,14 +318,14 @@ CPLMutex *CPLCreateOrAcquireMasterMutex(double dfWaitInSeconds = 1000.0)
 
 #ifdef MUTEX_NONE
 
-int CPLCreateOrAcquireMutexEx(CPLMutex **phMutex, double dfWaitInSeconds,
-                              int nOptions)
+bool CPLCreateOrAcquireMutexEx(CPLMutex **phMutex, double dfWaitInSeconds,
+                               int nOptions)
 {
     return false;
 }
 #else
-int CPLCreateOrAcquireMutexEx(CPLMutex **phMutex, double dfWaitInSeconds,
-                              int nOptions)
+bool CPLCreateOrAcquireMutexEx(CPLMutex **phMutex, double dfWaitInSeconds,
+                               int nOptions)
 {
     bool bSuccess = false;
 
@@ -358,16 +358,16 @@ int CPLCreateOrAcquireMutexEx(CPLMutex **phMutex, double dfWaitInSeconds,
 /************************************************************************/
 
 #ifdef MUTEX_NONE
-static int CPLCreateOrAcquireMutexInternal(CPLLock **phLock,
-                                           double dfWaitInSeconds,
-                                           CPLLockType eType)
+static bool CPLCreateOrAcquireMutexInternal(CPLLock **phLock,
+                                            double dfWaitInSeconds,
+                                            CPLLockType eType)
 {
     return false;
 }
 #else
-static int CPLCreateOrAcquireMutexInternal(CPLLock **phLock,
-                                           double dfWaitInSeconds,
-                                           CPLLockType eType)
+static bool CPLCreateOrAcquireMutexInternal(CPLLock **phLock,
+                                            double dfWaitInSeconds,
+                                            CPLLockType eType)
 
 {
     bool bSuccess = false;
@@ -1462,35 +1462,31 @@ int CPLCreateOrAcquireMutexEx(CPLMutex **phMutex, double dfWaitInSeconds,
                               int nOptions)
 
 {
-    bool bSuccess = false;
-
     pthread_mutex_lock(&global_mutex);
     if (*phMutex == nullptr)
     {
         *phMutex = CPLCreateMutexInternal(true, nOptions);
-        bSuccess = *phMutex != nullptr;
+        const bool bSuccess = *phMutex != nullptr;
         pthread_mutex_unlock(&global_mutex);
+        if (!bSuccess)
+            return false;
     }
     else
     {
         pthread_mutex_unlock(&global_mutex);
-
-        bSuccess = CPL_TO_BOOL(CPLAcquireMutex(*phMutex, dfWaitInSeconds));
     }
 
-    return bSuccess;
+    return CPL_TO_BOOL(CPLAcquireMutex(*phMutex, dfWaitInSeconds));
 }
 
 /************************************************************************/
 /*                   CPLCreateOrAcquireMutexInternal()                  */
 /************************************************************************/
 
-static int CPLCreateOrAcquireMutexInternal(CPLLock **phLock,
-                                           double dfWaitInSeconds,
-                                           CPLLockType eType)
+static bool CPLCreateOrAcquireMutexInternal(CPLLock **phLock,
+                                            double dfWaitInSeconds,
+                                            CPLLockType eType)
 {
-    bool bSuccess = false;
-
     pthread_mutex_lock(&global_mutex);
     if (*phLock == nullptr)
     {
@@ -1507,18 +1503,17 @@ static int CPLCreateOrAcquireMutexInternal(CPLLock **phLock,
                 *phLock = nullptr;
             }
         }
-        bSuccess = *phLock != nullptr;
+        const bool bSuccess = *phLock != nullptr;
         pthread_mutex_unlock(&global_mutex);
+        if (!bSuccess)
+            return false;
     }
     else
     {
         pthread_mutex_unlock(&global_mutex);
-
-        bSuccess =
-            CPL_TO_BOOL(CPLAcquireMutex((*phLock)->u.hMutex, dfWaitInSeconds));
     }
 
-    return bSuccess;
+    return CPL_TO_BOOL(CPLAcquireMutex((*phLock)->u.hMutex, dfWaitInSeconds));
 }
 
 /************************************************************************/
@@ -1624,20 +1619,23 @@ static CPLMutex *CPLCreateMutexInternal(bool bAlreadyInGlobalLock, int nOptions)
     psItem->nOptions = nOptions;
     CPLInitMutex(psItem);
 
-    // Mutexes are implicitly acquired when created.
-    CPLAcquireMutex(reinterpret_cast<CPLMutex *>(psItem), 0.0);
-
     return reinterpret_cast<CPLMutex *>(psItem);
 }
 
 CPLMutex *CPLCreateMutex()
 {
-    return CPLCreateMutexInternal(false, CPL_MUTEX_RECURSIVE);
+    CPLMutex *mutex = CPLCreateMutexInternal(false, CPL_MUTEX_RECURSIVE);
+    if (mutex)
+        CPLAcquireMutex(mutex, 0);
+    return mutex;
 }
 
 CPLMutex *CPLCreateMutexEx(int nOptions)
 {
-    return CPLCreateMutexInternal(false, nOptions);
+    CPLMutex *mutex = CPLCreateMutexInternal(false, nOptions);
+    if (mutex)
+        CPLAcquireMutex(mutex, 0);
+    return mutex;
 }
 
 /************************************************************************/