diff --git a/source/bulkdata/profile.c b/source/bulkdata/profile.c index 5c814026..b5a86305 100644 --- a/source/bulkdata/profile.c +++ b/source/bulkdata/profile.c @@ -1286,7 +1286,11 @@ T2ERROR deleteAllProfiles(bool delFromDisk) int profileIndex = 0; Profile *tempProfile = NULL; - pthread_rwlock_rdlock(&profileListLock); + /* Acquire wrlock once to atomically validate, read count, and disable all + * profiles. This eliminates the TOCTOU race between the old pattern of + * rdlock-for-count then wrlock-per-profile, where the list could change + * between the two operations. */ + pthread_rwlock_wrlock(&profileListLock); if(profileList == NULL) { T2Error("profile list is not initialized yet or profileList is empty, ignoring\n"); @@ -1295,14 +1299,23 @@ T2ERROR deleteAllProfiles(bool delFromDisk) } count = Vector_Size(profileList); - pthread_rwlock_unlock(&profileListLock); - - for(; profileIndex < count; profileIndex++) + for(profileIndex = 0; profileIndex < count; profileIndex++) { - pthread_rwlock_wrlock(&profileListLock); tempProfile = (Profile *)Vector_At(profileList, profileIndex); tempProfile->enable = false; tempProfile->isSchedulerstarted = false; + } + pthread_rwlock_unlock(&profileListLock); + + /* Second pass: perform blocking operations (unregister, signal, join) without + * holding profileListLock for extended periods. The caller (uninitProfileList) + * sets initialized=false before calling, which prevents concurrent list + * modifications. We still briefly acquire rdlock for each Vector_At to + * satisfy Coverity's lock-consistency analysis. */ + for(profileIndex = 0; profileIndex < count; profileIndex++) + { + pthread_rwlock_rdlock(&profileListLock); + tempProfile = (Profile *)Vector_At(profileList, profileIndex); pthread_rwlock_unlock(&profileListLock); if(T2ERROR_SUCCESS != unregisterProfileFromScheduler(tempProfile->name)) @@ -1330,13 +1343,13 @@ T2ERROR deleteAllProfiles(bool delFromDisk) * after setting threadExists = false (see CollectAndReport cleanup). */ } - /* Re-acquire profileListLock for profile cleanup */ - pthread_rwlock_wrlock(&profileListLock); + /* grepSeekProfile cleanup is safe without profileListLock here: + * the profile's thread has been joined (or never existed), and + * initialized=false prevents concurrent access from other threads. */ if(tempProfile->grepSeekProfile) { freeGrepSeekProfile(tempProfile->grepSeekProfile); } - pthread_rwlock_unlock(&profileListLock); if(delFromDisk == true) { removeProfileFromDisk(REPORTPROFILES_PERSISTENCE_PATH, tempProfile->name); diff --git a/source/bulkdata/profilexconf.c b/source/bulkdata/profilexconf.c index db678534..5a6b7c6f 100644 --- a/source/bulkdata/profilexconf.c +++ b/source/bulkdata/profilexconf.c @@ -687,28 +687,31 @@ T2ERROR ProfileXConf_uninit() } initialized = false; - pthread_mutex_lock(&singleProfile->reportInProgressMutex); - bool reportInProgress = singleProfile->reportInProgress; - pthread_mutex_unlock(&singleProfile->reportInProgressMutex); - if(reportInProgress) + /* Wake the report thread (if it exists) and join it before cleanup. + * The thread may be in one of two states: + * (a) reportInProgress=true — actively generating a report + * (b) reportInProgress=false — idle in pthread_cond_wait(&reuseThread) + * In both cases we must signal and join; otherwise case (b) would skip + * the join, and freeProfileXConf + pthread_cond_destroy would destroy + * resources the sleeping thread still references (undefined behavior). */ + pthread_mutex_lock(&xconfProfileLock); + if(reportThreadExits) { - T2Debug("Waiting for final report before uninit\n"); - pthread_mutex_lock(&xconfProfileLock); pthread_cond_signal(&reuseThread); pthread_mutex_unlock(&xconfProfileLock); pthread_join(singleProfile->reportThread, NULL); - pthread_mutex_lock(&singleProfile->reportInProgressMutex); - singleProfile->reportInProgress = false ; - pthread_mutex_unlock(&singleProfile->reportInProgressMutex); - T2Info("Final report is completed, releasing profile memory\n"); + pthread_mutex_lock(&xconfProfileLock); + singleProfile->reportInProgress = false; + T2Info("Report thread joined, releasing profile memory\n"); } - pthread_mutex_lock(&xconfProfileLock); freeProfileXConf(); pthread_mutex_unlock(&xconfProfileLock); - /* Destroy condition variable at module uninit, after all threads are stopped */ + /* Destroy condition variable at module uninit, after all threads are stopped. + * xconfProfileLock is statically initialized (PTHREAD_MUTEX_INITIALIZER) and + * does not need to be destroyed — destroying it would leave it invalid if + * the module were ever re-initialized in the same process. */ pthread_cond_destroy(&reuseThread); - pthread_mutex_destroy(&xconfProfileLock); T2Debug("%s --out\n", __FUNCTION__); return T2ERROR_SUCCESS; } @@ -724,9 +727,8 @@ T2ERROR ProfileXConf_set(ProfileXConf *profile) if(!singleProfile) { singleProfile = profile; - pthread_mutex_lock(&singleProfile->reportInProgressMutex); - singleProfile->reportInProgress = false ; - pthread_mutex_unlock(&singleProfile->reportInProgressMutex); + /* reportInProgress is protected by xconfProfileLock (already held) */ + singleProfile->reportInProgress = false; size_t emIndex = 0; EventMarker *eMarker = NULL; for(; emIndex < Vector_Size(singleProfile->eMarkerList); emIndex++) @@ -887,9 +889,8 @@ T2ERROR ProfileXConf_delete(ProfileXConf *profile) if(isNameEqual) { profile->bClearSeekMap = singleProfile->bClearSeekMap ; - pthread_mutex_lock(&profile->reportInProgressMutex); - profile->reportInProgress = false ; - pthread_mutex_unlock(&profile->reportInProgressMutex); + /* reportInProgress is protected by xconfProfileLock (already held) */ + profile->reportInProgress = false; if(count > 0 && profile->cachedReportList != NULL) { T2Info("There are %zu cached reports in the profile \n", count); @@ -1037,12 +1038,11 @@ void ProfileXConf_notifyTimeout(bool isClearSeekMap, bool isOnDemand) return ; } isOnDemandReport = isOnDemand; - pthread_mutex_lock(&singleProfile->reportInProgressMutex); + /* reportInProgress is protected by xconfProfileLock (already held) */ if(!singleProfile->reportInProgress) { singleProfile->bClearSeekMap = isClearSeekMap; singleProfile->reportInProgress = true; - pthread_mutex_unlock(&singleProfile->reportInProgressMutex); if (reportThreadExits) { @@ -1055,11 +1055,20 @@ void ProfileXConf_notifyTimeout(bool isClearSeekMap, bool isOnDemand) { T2Error("Failed to create report thread with error code = %d !!! \n", reportThreadStatus); } + else + { + /* Set flag immediately so ProfileXConf_uninit can find and + * join this thread. Without this, there is a race window + * between pthread_create returning and the new thread + * setting reportThreadExits=true inside CollectAndReportXconf, + * during which uninit would skip the join and free resources + * the thread is about to use. */ + reportThreadExits = true; + } } } else { - pthread_mutex_unlock(&singleProfile->reportInProgressMutex); T2Warning("Received profileTimeoutCb while previous callback is still in progress - ignoring the request\n"); }