From 3b8d4a4e6af9e96a1a2d3a4443ecd32a55e92aac Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 19 Feb 2024 21:40:07 +0100 Subject: [PATCH 1/2] CPLCreateOrAcquireMutexEx(): fix TSAN/valgrind --tool=helgrind warning about lock-order inversion (fixes #1108) --- port/cpl_multiproc.cpp | 52 ++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/port/cpl_multiproc.cpp b/port/cpl_multiproc.cpp index 2658ac1aa5de..65384486c29b 100644 --- a/port/cpl_multiproc.cpp +++ b/port/cpl_multiproc.cpp @@ -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(psItem), 0.0); - return reinterpret_cast(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; } /************************************************************************/ From a1ba66ffbb3d23a9c7a8f416a774fad6c38755ff Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 19 Feb 2024 22:13:16 +0100 Subject: [PATCH 2/2] GDALGetCacheMax64(): fix TSAN complaint (fixes #1837) --- gcore/gdalrasterblock.cpp | 116 +++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 57 deletions(-) diff --git a/gcore/gdalrasterblock.cpp b/gcore/gdalrasterblock.cpp index 48c19b0aec5a..93eea22399ee 100644 --- a/gcore/gdalrasterblock.cpp +++ b/gcore/gdalrasterblock.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include "cpl_atomic_ops.h" #include "cpl_conv.h" @@ -43,7 +44,6 @@ #include "cpl_string.h" #include "cpl_vsi.h" -static bool bCacheMaxInitialized = false; // Will later be overridden by the default 5% if GDAL_CACHEMAX not defined. static GIntBig nCacheMax = 40 * 1024 * 1024; static GIntBig nCacheUsed = 0; @@ -153,10 +153,8 @@ void CPL_STDCALL GDALSetCacheMax64(GIntBig nNewSizeInBytes) } #endif - { - INITIALIZE_LOCK; - } - bCacheMaxInitialized = true; + // To force one-time initialization of nCacheMax if not already done + GDALGetCacheMax64(); nCacheMax = nNewSizeInBytes; /* -------------------------------------------------------------------- */ @@ -237,73 +235,77 @@ int CPL_STDCALL GDALGetCacheMax() GIntBig CPL_STDCALL GDALGetCacheMax64() { - if (!bCacheMaxInitialized) - { + static std::once_flag flagSetupGDALGetCacheMax64; + std::call_once( + flagSetupGDALGetCacheMax64, + []() { - INITIALIZE_LOCK; - } - bSleepsForBockCacheDebug = - CPLTestBool(CPLGetConfigOption("GDAL_DEBUG_BLOCK_CACHE", "NO")); + { + INITIALIZE_LOCK; + } + bSleepsForBockCacheDebug = + CPLTestBool(CPLGetConfigOption("GDAL_DEBUG_BLOCK_CACHE", "NO")); - const char *pszCacheMax = CPLGetConfigOption("GDAL_CACHEMAX", "5%"); + const char *pszCacheMax = CPLGetConfigOption("GDAL_CACHEMAX", "5%"); - GIntBig nNewCacheMax; - if (strchr(pszCacheMax, '%') != nullptr) - { - GIntBig nUsablePhysicalRAM = CPLGetUsablePhysicalRAM(); - if (nUsablePhysicalRAM > 0) + GIntBig nNewCacheMax; + if (strchr(pszCacheMax, '%') != nullptr) { - // For some reason, coverity pretends that this will overflow. - // "Multiply operation overflows on operands - // static_cast( nUsablePhysicalRAM ) and - // CPLAtof(pszCacheMax). Example values for operands: CPLAtof( - // pszCacheMax ) = 2251799813685248, - // static_cast(nUsablePhysicalRAM) = - // -9223372036854775808." coverity[overflow,tainted_data] - double dfCacheMax = static_cast(nUsablePhysicalRAM) * - CPLAtof(pszCacheMax) / 100.0; - if (dfCacheMax >= 0 && dfCacheMax < 1e15) - nNewCacheMax = static_cast(dfCacheMax); + GIntBig nUsablePhysicalRAM = CPLGetUsablePhysicalRAM(); + if (nUsablePhysicalRAM > 0) + { + // For some reason, coverity pretends that this will overflow. + // "Multiply operation overflows on operands + // static_cast( nUsablePhysicalRAM ) and + // CPLAtof(pszCacheMax). Example values for operands: CPLAtof( + // pszCacheMax ) = 2251799813685248, + // static_cast(nUsablePhysicalRAM) = + // -9223372036854775808." coverity[overflow,tainted_data] + double dfCacheMax = + static_cast(nUsablePhysicalRAM) * + CPLAtof(pszCacheMax) / 100.0; + if (dfCacheMax >= 0 && dfCacheMax < 1e15) + nNewCacheMax = static_cast(dfCacheMax); + else + nNewCacheMax = nCacheMax; + } else + { + CPLDebug("GDAL", "Cannot determine usable physical RAM."); nNewCacheMax = nCacheMax; + } } else { - CPLDebug("GDAL", "Cannot determine usable physical RAM."); - nNewCacheMax = nCacheMax; - } - } - else - { - nNewCacheMax = CPLAtoGIntBig(pszCacheMax); - if (nNewCacheMax < 100000) - { - if (nNewCacheMax < 0) + nNewCacheMax = CPLAtoGIntBig(pszCacheMax); + if (nNewCacheMax < 100000) { - CPLError(CE_Failure, CPLE_NotSupported, - "Invalid value for GDAL_CACHEMAX. " - "Using default value."); - GIntBig nUsablePhysicalRAM = CPLGetUsablePhysicalRAM(); - if (nUsablePhysicalRAM) - nNewCacheMax = nUsablePhysicalRAM / 20; + if (nNewCacheMax < 0) + { + CPLError(CE_Failure, CPLE_NotSupported, + "Invalid value for GDAL_CACHEMAX. " + "Using default value."); + GIntBig nUsablePhysicalRAM = CPLGetUsablePhysicalRAM(); + if (nUsablePhysicalRAM) + nNewCacheMax = nUsablePhysicalRAM / 20; + else + { + CPLDebug("GDAL", + "Cannot determine usable physical RAM."); + nNewCacheMax = nCacheMax; + } + } else { - CPLDebug("GDAL", - "Cannot determine usable physical RAM."); - nNewCacheMax = nCacheMax; + nNewCacheMax *= 1024 * 1024; } } - else - { - nNewCacheMax *= 1024 * 1024; - } } - } - nCacheMax = nNewCacheMax; - CPLDebug("GDAL", "GDAL_CACHEMAX = " CPL_FRMT_GIB " MB", - nCacheMax / (1024 * 1024)); - bCacheMaxInitialized = true; - } + nCacheMax = nNewCacheMax; + CPLDebug("GDAL", "GDAL_CACHEMAX = " CPL_FRMT_GIB " MB", + nCacheMax / (1024 * 1024)); + }); + // coverity[overflow_sink] return nCacheMax; }