From 4774200fc1409b8f4e3b6c659f37b05ca803bfe3 Mon Sep 17 00:00:00 2001 From: mtirum011 Date: Fri, 2 Jan 2026 08:37:54 +0000 Subject: [PATCH] RDK-60293 [telemetry] RDK Coverity Defect Resolution for Device Management --- source/bulkdata/datamodel.c | 84 ++++++++-- source/bulkdata/profile.c | 141 ++++++++++++++-- source/bulkdata/profile.h | 1 + source/bulkdata/profilexconf.c | 34 +++- source/bulkdata/profilexconf.h | 2 + source/bulkdata/reportprofiles.c | 29 +++- source/bulkdata/t2eventreceiver.c | 152 +++++++++++++----- source/ccspinterface/rbusInterface.c | 2 +- .../commonlib/telemetry_busmessage_sender.c | 12 +- source/dcautil/dca.c | 4 + .../protocol/rbusMethod/rbusmethodinterface.c | 4 + source/reportgen/reportgen.c | 25 ++- source/scheduler/scheduler.c | 55 ++++++- source/t2parser/t2parserxconf.c | 2 + source/utils/persistence.c | 20 ++- source/utils/t2collection.c | 8 +- source/xconf-client/xconfclient.c | 78 +++++++-- 17 files changed, 538 insertions(+), 115 deletions(-) diff --git a/source/bulkdata/datamodel.c b/source/bulkdata/datamodel.c index bf290bc0..a9f5f892 100644 --- a/source/bulkdata/datamodel.c +++ b/source/bulkdata/datamodel.c @@ -50,17 +50,28 @@ static void *process_rp_thread(void *data) { (void) data;//To fix compiler warning cJSON *reportProfiles = NULL; + bool shouldContinue = true; T2Debug("%s ++in\n", __FUNCTION__); - while(!stopProcessing) + while(shouldContinue) { pthread_mutex_lock(&rpMutex); + shouldContinue = !stopProcessing; + if(!shouldContinue) + { + pthread_mutex_unlock(&rpMutex); + break; + } T2Info("%s: Waiting for event from tr-181 \n", __FUNCTION__); - pthread_cond_wait(&rpCond, &rpMutex); + while(t2_queue_count(rpQueue) == 0 && shouldContinue) + { + pthread_cond_wait(&rpCond, &rpMutex); + shouldContinue = !stopProcessing; + } T2Debug("%s: Received wake up signal \n", __FUNCTION__); - if(t2_queue_count(rpQueue) > 0) + if(t2_queue_count(rpQueue) > 0 && shouldContinue) { reportProfiles = (cJSON *)t2_queue_pop(rpQueue); if (reportProfiles) @@ -80,17 +91,32 @@ static void *process_tmprp_thread(void *data) { (void) data;//To fix compiler warning cJSON *tmpReportProfiles = NULL; + bool shouldContinue = true; T2Debug("%s ++in\n", __FUNCTION__); - while(!stopProcessing) + while(shouldContinue) { pthread_mutex_lock(&tmpRpMutex); + pthread_mutex_lock(&rpMutex); + shouldContinue = !stopProcessing; + pthread_mutex_unlock(&rpMutex); + if(!shouldContinue) + { + pthread_mutex_unlock(&tmpRpMutex); + break; + } T2Info("%s: Waiting for event from tr-181 \n", __FUNCTION__); - pthread_cond_wait(&tmpRpCond, &tmpRpMutex); + while(t2_queue_count(tmpRpQueue) == 0 && shouldContinue) + { + pthread_cond_wait(&tmpRpCond, &tmpRpMutex); + pthread_mutex_lock(&rpMutex); + shouldContinue = !stopProcessing; + pthread_mutex_unlock(&rpMutex); + } T2Debug("%s: Received wake up signal \n", __FUNCTION__); - if(t2_queue_count(tmpRpQueue) > 0) + if(t2_queue_count(tmpRpQueue) > 0 && shouldContinue) { tmpReportProfiles = (cJSON *)t2_queue_pop(tmpRpQueue); if (tmpReportProfiles) @@ -110,11 +136,31 @@ static void *process_msg_thread(void *data) { (void) data;//To fix compiler warning struct __msgpack__ *msgpack; - while(!stopProcessing) + bool shouldContinue = true; + + while(shouldContinue) { + // Check stopProcessing first without holding rpMsgMutex to avoid + // nested mutex acquisition which could lead to deadlock if another + // thread acquires these mutexes in opposite order (e.g., rpMutex then rpMsgMutex) + pthread_mutex_lock(&rpMutex); + shouldContinue = !stopProcessing; + pthread_mutex_unlock(&rpMutex); + + if(!shouldContinue) + { + break; + } + pthread_mutex_lock(&rpMsgMutex); - pthread_cond_wait(&msg_Cond, &rpMsgMutex); - if(t2_queue_count(rpMsgPkgQueue) > 0) + while(t2_queue_count(rpMsgPkgQueue) == 0 && shouldContinue) + { + pthread_cond_wait(&msg_Cond, &rpMsgMutex); + pthread_mutex_lock(&rpMutex); + shouldContinue = !stopProcessing; + pthread_mutex_unlock(&rpMutex); + } + if(t2_queue_count(rpMsgPkgQueue) > 0 && shouldContinue) { msgpack = (struct __msgpack__ *)t2_queue_pop(rpMsgPkgQueue); if (msgpack) @@ -274,18 +320,24 @@ T2ERROR datamodel_MsgpackProcessProfile(char *str, int strSize) msgpack->msgpack_blob = str; msgpack->msgpack_blob_size = strSize; - pthread_mutex_lock(&rpMsgMutex); - if (!stopProcessing) - { - t2_queue_push(rpMsgPkgQueue, (void *)msgpack); - pthread_cond_signal(&msg_Cond); - } - else + + // Check stopProcessing without holding rpMsgMutex to avoid nested mutex + // acquisition which could lead to deadlock + pthread_mutex_lock(&rpMutex); + bool isProcessing = !stopProcessing; + pthread_mutex_unlock(&rpMutex); + + if (!isProcessing) { free(msgpack->msgpack_blob); free(msgpack); T2Error("Datamodel not initialized, dropping request \n"); + return T2ERROR_SUCCESS; } + + pthread_mutex_lock(&rpMsgMutex); + t2_queue_push(rpMsgPkgQueue, (void *)msgpack); + pthread_cond_signal(&msg_Cond); pthread_mutex_unlock(&rpMsgMutex); return T2ERROR_SUCCESS; } diff --git a/source/bulkdata/profile.c b/source/bulkdata/profile.c index f5d9bb17..25cdf92f 100644 --- a/source/bulkdata/profile.c +++ b/source/bulkdata/profile.c @@ -22,6 +22,8 @@ #include #include #include +#include +#include #include "profile.h" #include "reportprofiles.h" @@ -326,6 +328,7 @@ static void* CollectAndReport(void* data) pthread_mutex_init(&profile->reuseThreadMutex, NULL); pthread_cond_init(&profile->reuseThread, NULL); pthread_mutex_lock(&profile->reuseThreadMutex); + profile->restartRequested = false; profile->threadExists = true; //GrepSeekProfile *GPF = profile->GrepSeekProfle; do @@ -335,6 +338,7 @@ static void* CollectAndReport(void* data) profile->reportInProgress = true; pthread_cond_signal(&profile->reportInProgressCond); pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); int count = profile->grepSeekProfile->execCounter; @@ -366,7 +370,11 @@ static void* CollectAndReport(void* data) { T2Debug(" profile->triggerReportOnCondition is not set \n"); } + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); //return NULL; goto reportThreadEnd; } @@ -392,7 +400,11 @@ static void* CollectAndReport(void* data) { T2Debug(" profile->triggerReportOnCondition is not set \n"); } + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); //return NULL; goto reportThreadEnd; } @@ -405,7 +417,11 @@ static void* CollectAndReport(void* data) if(T2ERROR_SUCCESS != initJSONReportProfile(&profile->jsonReportObj, &valArray, profile->RootName)) { T2Error("Failed to initialize JSON Report\n"); + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); //pthread_mutex_unlock(&profile->triggerCondMutex); if(profile->triggerReportOnCondition) { @@ -475,7 +491,11 @@ static void* CollectAndReport(void* data) if(ret != T2ERROR_SUCCESS) { T2Error("Unable to generate report for : %s\n", profile->name); + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); if(profile->triggerReportOnCondition) { profile->triggerReportOnCondition = false ; @@ -515,7 +535,11 @@ static void* CollectAndReport(void* data) if(cJSON_GetArraySize(array) == 0) { T2Warning("Array size of Report is %d. Report is empty. Cannot send empty report\n", cJSON_GetArraySize(array)); + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); if(profile->triggerReportOnCondition) { T2Info(" Unlock trigger condition mutex and set report on condition to false \n"); @@ -544,13 +568,45 @@ static void* CollectAndReport(void* data) } if(profile->maxUploadLatency > 0) { + pthread_mutex_lock(&profile->reuseThreadMutex); memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); memset(&profile->currentTime, 0, sizeof(struct timespec)); - pthread_cond_init(&profile->reportcond, NULL); clock_gettime(CLOCK_REALTIME, &profile->currentTime); profile->maxlatencyTime.tv_sec = profile->currentTime.tv_sec; - srand(time(0)); // Initialise the random number generator - maxuploadinmilliSec = rand() % (profile->maxUploadLatency - 1); + pthread_mutex_unlock(&profile->reuseThreadMutex); + pthread_mutex_init(&profile->reportMutex, NULL); + pthread_cond_init(&profile->reportcond, NULL); + unsigned int random_value = 0; + // Ensure maxUploadLatency > 1 to avoid modulo by zero + if(profile->maxUploadLatency > 1) + { + int urandom = -1; + urandom = open("/dev/urandom", O_RDONLY); + if(urandom != -1 && read(urandom, &random_value, sizeof(random_value)) == sizeof(random_value)) + { + maxuploadinmilliSec = random_value % (profile->maxUploadLatency - 1); + if(close(urandom) == -1) + { + T2Error("Failed to close /dev/urandom: %s\n", strerror(errno)); + } + } + else + { + if(urandom != -1) + { + if(close(urandom) == -1) + { + T2Error("Failed to close /dev/urandom: %s\n", strerror(errno)); + } + } + maxuploadinmilliSec = (unsigned int)(time(0) % (profile->maxUploadLatency - 1)); + } + } + else + { + // If maxUploadLatency is 1, set maxuploadinmilliSec to 0 + maxuploadinmilliSec = 0; + } maxuploadinSec = (maxuploadinmilliSec + 1) / 1000; } if( strcmp(profile->protocol, "HTTP") == 0 || strcmp(profile->protocol, "RBUS_METHOD") == 0 ) @@ -561,15 +617,26 @@ static void* CollectAndReport(void* data) httpUrl = prepareHttpUrl(profile->t2HTTPDest); /* Append URL with http properties */ if(profile->maxUploadLatency > 0) { - pthread_mutex_lock(&profile->reportMutex); T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); + struct timespec timeout; + pthread_mutex_lock(&profile->reuseThreadMutex); profile->maxlatencyTime.tv_sec += maxuploadinSec; - n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &profile->maxlatencyTime); + timeout = profile->maxlatencyTime; + pthread_mutex_unlock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportMutex); + while (profile->enable && n != ETIMEDOUT) + { + n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); + } if(n == ETIMEDOUT) { T2Info("TIMEOUT for maxUploadLatency of profile %s\n", profile->name); ret = sendReportOverHTTP(httpUrl, jsonReport, NULL); } + else if(n == 0) + { + T2Info("Profile : %s signaled before timeout, skipping report upload\n", profile->name); + } else { T2Error("Profile : %s pthread_cond_timedwait ERROR!!!\n", profile->name); @@ -580,7 +647,11 @@ static void* CollectAndReport(void* data) free(httpUrl); httpUrl = NULL; } + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); if(profile->triggerReportOnCondition) { T2Info(" Unlock trigger condition mutex and set report on condition to false \n"); @@ -602,6 +673,7 @@ static void* CollectAndReport(void* data) } pthread_mutex_unlock(&profile->reportMutex); pthread_cond_destroy(&profile->reportcond); + pthread_mutex_destroy(&profile->reportMutex); } else { @@ -612,21 +684,36 @@ static void* CollectAndReport(void* data) { if(profile->maxUploadLatency > 0 ) { - pthread_mutex_lock(&profile->reportMutex); T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); + struct timespec timeout; + pthread_mutex_lock(&profile->reuseThreadMutex); profile->maxlatencyTime.tv_sec += maxuploadinSec; - n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &profile->maxlatencyTime); + timeout = profile->maxlatencyTime; + pthread_mutex_unlock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportMutex); + while (profile->enable && n != ETIMEDOUT) + { + n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); + } if(n == ETIMEDOUT) { T2Info("TIMEOUT for maxUploadLatency of profile %s\n", profile->name); ret = sendReportsOverRBUSMethod(profile->t2RBUSDest->rbusMethodName, profile->t2RBUSDest->rbusMethodParamList, jsonReport); } + else if(n == 0) + { + T2Info("Profile : %s signaled before timeout, skipping report upload\n", profile->name); + } else { T2Error("Profile : %s pthread_cond_timedwait ERROR!!!\n", profile->name); pthread_mutex_unlock(&profile->reportMutex); pthread_cond_destroy(&profile->reportcond); + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); if(profile->triggerReportOnCondition) { T2Info(" Unlock trigger condition mutex and set report on condition to false \n"); @@ -686,7 +773,11 @@ static void* CollectAndReport(void* data) if(profile->SendErr > 3 && !(rbusCheckMethodExists(profile->t2RBUSDest->rbusMethodName))) //to delete the profile in the next CollectAndReport or triggercondition { T2Debug("RBUS_METHOD doesn't exists after 3 retries\n"); + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); if(profile->triggerReportOnCondition) { profile->triggerReportOnCondition = false ; @@ -758,7 +849,11 @@ static void* CollectAndReport(void* data) jsonReport = NULL; } + pthread_mutex_lock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); if(profile->triggerReportOnCondition) { T2Info(" Unlock trigger condition mutex and set report on condition to false \n"); @@ -778,13 +873,22 @@ static void* CollectAndReport(void* data) reportThreadEnd : T2Info("%s while Loop -- END; wait for restart event\n", __FUNCTION__); T2Info("%s --out\n", __FUNCTION__); - pthread_cond_wait(&profile->reuseThread, &profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reuseThreadMutex); + while(profile->enable && !profile->restartRequested) + { + pthread_cond_wait(&profile->reuseThread, &profile->reuseThreadMutex); + } + profile->restartRequested = false; } while(profile->enable); + pthread_mutex_unlock(&profile->reuseThreadMutex); T2Info("%s --out Exiting collect and report Thread\n", __FUNCTION__); + pthread_mutex_lock(&profile->reuseThreadMutex); pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; pthread_mutex_unlock(&profile->reportInProgressMutex); + pthread_mutex_unlock(&profile->reuseThreadMutex); + pthread_mutex_lock(&profile->reuseThreadMutex); profile->threadExists = false; pthread_mutex_unlock(&profile->reuseThreadMutex); pthread_mutex_destroy(&profile->reuseThreadMutex); @@ -812,11 +916,16 @@ void NotifyTimeout(const char* profileName, bool isClearSeekMap) { profile->reportInProgress = true; profile->bClearSeekMap = isClearSeekMap; + // Copy threadExists while holding reportInProgressMutex to avoid holding both locks + bool threadStillExists = profile->threadExists; + pthread_mutex_unlock(&profile->reportInProgressMutex); + /* To avoid previous report thread to go into zombie state, mark it detached. */ - if (profile->threadExists) + if (threadStillExists) { T2Info("Signal Thread To restart\n"); pthread_mutex_lock(&profile->reuseThreadMutex); + profile->restartRequested = true; pthread_cond_signal(&profile->reuseThread); pthread_mutex_unlock(&profile->reuseThreadMutex); } @@ -828,12 +937,11 @@ void NotifyTimeout(const char* profileName, bool isClearSeekMap) else { T2Warning("Either profile is disabled or report generation still in progress - ignoring the request\n"); + pthread_mutex_unlock(&profile->reportInProgressMutex); } - pthread_mutex_unlock(&profile->reportInProgressMutex); T2Debug("%s --out\n", __FUNCTION__); } - T2ERROR Profile_storeMarkerEvent(const char *profileName, T2Event *eventInfo) { T2Debug("%s ++in\n", __FUNCTION__); @@ -1177,6 +1285,7 @@ T2ERROR deleteAllProfiles(bool delFromDisk) if (tempProfile->threadExists) { pthread_mutex_lock(&tempProfile->reuseThreadMutex); + tempProfile->restartRequested = true; pthread_cond_signal(&tempProfile->reuseThread); pthread_mutex_unlock(&tempProfile->reuseThreadMutex); pthread_join(tempProfile->reportThread, NULL); @@ -1266,20 +1375,27 @@ T2ERROR deleteProfile(const char *profileName) T2Info("Waiting for CollectAndReport to be complete : %s\n", profileName); pthread_mutex_lock(&plMutex); + // Avoid holding both reportInProgressMutex and reuseThreadMutex at the same time. + // This prevents potential deadlocks when other code paths acquire these locks + // in the opposite order. pthread_mutex_lock(&profile->reportInProgressMutex); while (profile->reportInProgress && !profile->threadExists) { pthread_cond_wait(&profile->reportInProgressCond, &profile->reportInProgressMutex); } + bool threadStillExists = profile->threadExists; pthread_mutex_unlock(&profile->reportInProgressMutex); - if (profile->threadExists) + if (threadStillExists) { pthread_mutex_lock(&profile->reuseThreadMutex); + profile->restartRequested = true; pthread_cond_signal(&profile->reuseThread); pthread_mutex_unlock(&profile->reuseThreadMutex); pthread_join(profile->reportThread, NULL); + pthread_mutex_lock(&profile->reuseThreadMutex); profile->threadExists = false; + pthread_mutex_unlock(&profile->reuseThreadMutex); } if(Vector_Size(profile->triggerConditionList) > 0) @@ -1809,6 +1925,7 @@ T2ERROR triggerReportOnCondtion(const char *referenceName, const char *reference if(tempProfile->isSchedulerstarted) { SendInterruptToTimeoutThread(tempProfilename); + // triggerCondMutex will be unlocked by CollectAndReport after report generation } else { diff --git a/source/bulkdata/profile.h b/source/bulkdata/profile.h index 574e6558..21bd4de4 100644 --- a/source/bulkdata/profile.h +++ b/source/bulkdata/profile.h @@ -91,6 +91,7 @@ typedef struct _Profile Vector *triggerConditionList; pthread_cond_t reuseThread; pthread_mutex_t reuseThreadMutex; + bool restartRequested; bool threadExists; GrepSeekProfile *grepSeekProfile; // To store GrepConfig } Profile; diff --git a/source/bulkdata/profilexconf.c b/source/bulkdata/profilexconf.c index 69c94d04..8ff951d3 100644 --- a/source/bulkdata/profilexconf.c +++ b/source/bulkdata/profilexconf.c @@ -150,6 +150,8 @@ static void freeProfileXConf() { freeGrepSeekProfile(singleProfile->grepSeekProfile); } + pthread_mutex_destroy(&singleProfile->reportInProgressMutex); + pthread_cond_destroy(&singleProfile->reportInProgressCond); free(singleProfile); singleProfile = NULL; } @@ -254,7 +256,9 @@ static void* CollectAndReportXconf(void* data) if(T2ERROR_SUCCESS != initJSONReportXconf(&profile->jsonReportObj, &valArray)) { T2Error("Failed to initialize JSON Report\n"); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); //pthread_mutex_unlock(&plMutex); //return NULL; goto reportXconfThreadEnd; @@ -314,7 +318,9 @@ static void* CollectAndReportXconf(void* data) if(ret != T2ERROR_SUCCESS) { T2Error("Unable to generate report for : %s\n", profile->name); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); //pthread_mutex_unlock(&plMutex); //return NULL; goto reportXconfThreadEnd; @@ -344,7 +350,9 @@ static void* CollectAndReportXconf(void* data) // Before caching the report, add "REPORT_TYPE": "CACHED" // tagReportAsCached(&jsonReport); Vector_PushBack(profile->cachedReportList, strdup(jsonReport)); + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); /* CID 187010: Dereference before null check */ free(jsonReport); jsonReport = NULL; @@ -486,7 +494,9 @@ static void* CollectAndReportXconf(void* data) isAbortTriggered = false ; } + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false; + pthread_mutex_unlock(&profile->reportInProgressMutex); //pthread_mutex_unlock(&plMutex); reportXconfThreadEnd : T2Info("%s while Loop -- END \n", __FUNCTION__); @@ -580,14 +590,19 @@ T2ERROR ProfileXConf_uninit() } initialized = false; - if(singleProfile->reportInProgress) + pthread_mutex_lock(&singleProfile->reportInProgressMutex); + bool reportInProgress = singleProfile->reportInProgress; + pthread_mutex_unlock(&singleProfile->reportInProgressMutex); + if(reportInProgress) { T2Debug("Waiting for final report before uninit\n"); pthread_mutex_lock(&plMutex); pthread_cond_signal(&reuseThread); pthread_mutex_unlock(&plMutex); 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(&plMutex); @@ -610,7 +625,9 @@ T2ERROR ProfileXConf_set(ProfileXConf *profile) if(!singleProfile) { singleProfile = profile; + pthread_mutex_lock(&singleProfile->reportInProgressMutex); singleProfile->reportInProgress = false ; + pthread_mutex_unlock(&singleProfile->reportInProgressMutex); size_t emIndex = 0; EventMarker *eMarker = NULL; for(; emIndex < Vector_Size(singleProfile->eMarkerList); emIndex++) @@ -719,7 +736,10 @@ T2ERROR ProfileXConf_delete(ProfileXConf *profile) } } - if(singleProfile->reportInProgress) + pthread_mutex_lock(&singleProfile->reportInProgressMutex); + bool reportInProgress = singleProfile->reportInProgress; + pthread_mutex_unlock(&singleProfile->reportInProgressMutex); + if(reportInProgress) { T2Info("Waiting for CollectAndReportXconf to be complete : %s\n", singleProfile->name); } @@ -733,7 +753,9 @@ T2ERROR ProfileXConf_delete(ProfileXConf *profile) if(isNameEqual) { profile->bClearSeekMap = singleProfile->bClearSeekMap ; + pthread_mutex_lock(&profile->reportInProgressMutex); profile->reportInProgress = false ; + pthread_mutex_unlock(&profile->reportInProgressMutex); if(count > 0 && profile->cachedReportList != NULL) { T2Info("There are %zu cached reports in the profile \n", count); @@ -881,10 +903,12 @@ void ProfileXConf_notifyTimeout(bool isClearSeekMap, bool isOnDemand) return ; } isOnDemandReport = isOnDemand; + pthread_mutex_lock(&singleProfile->reportInProgressMutex); if(!singleProfile->reportInProgress) { singleProfile->bClearSeekMap = isClearSeekMap; singleProfile->reportInProgress = true; + pthread_mutex_unlock(&singleProfile->reportInProgressMutex); if (reportThreadExits) { @@ -901,6 +925,7 @@ void ProfileXConf_notifyTimeout(bool isClearSeekMap, bool isOnDemand) } else { + pthread_mutex_unlock(&singleProfile->reportInProgressMutex); T2Warning("Received profileTimeoutCb while previous callback is still in progress - ignoring the request\n"); } @@ -1007,7 +1032,10 @@ T2ERROR ProfileXConf_terminateReport() } // Check whether any XconfReport is in progress - if(singleProfile->reportInProgress) + pthread_mutex_lock(&singleProfile->reportInProgressMutex); + bool reportInProgress = singleProfile->reportInProgress; + pthread_mutex_unlock(&singleProfile->reportInProgressMutex); + if(reportInProgress) { isAbortTriggered = true; // Check if a child pid is still alive diff --git a/source/bulkdata/profilexconf.h b/source/bulkdata/profilexconf.h index 8f46789c..aaba40b8 100644 --- a/source/bulkdata/profilexconf.h +++ b/source/bulkdata/profilexconf.h @@ -50,6 +50,8 @@ typedef struct _ProfileXConf { bool isUpdated; bool reportInProgress; + pthread_cond_t reportInProgressCond; + pthread_mutex_t reportInProgressMutex; bool bClearSeekMap; bool checkPreviousSeek; // To support Previous_Logs report post reboot bool saveSeekConfig; // To save the Seek config to persistant storage diff --git a/source/bulkdata/reportprofiles.c b/source/bulkdata/reportprofiles.c index dbe48be5..354e76c3 100644 --- a/source/bulkdata/reportprofiles.c +++ b/source/bulkdata/reportprofiles.c @@ -946,7 +946,20 @@ void ReportProfiles_ProcessReportProfilesBlob(cJSON *profiles_root, bool rprofil // Delete profiles not present in the new profile list char *profileNameKey = NULL; - int count = hash_map_count(profileHashMap) - 1; + uint32_t hashmap_count = hash_map_count(profileHashMap); + int count; + // Check for empty hashmap (count == 0) or error condition (UINT32_MAX). + // UINT32_MAX is the documented return value of hash_map_count when map is NULL. + // Setting count = -1 ensures the while(count >= 0) loop below will not execute. + if (hashmap_count == 0 || hashmap_count == UINT32_MAX) + { + count = -1; + } + else + { + count = (int)(hashmap_count - 1); + } + const char *DirPath = NULL; if (rprofiletypes == T2_RP) @@ -1309,6 +1322,7 @@ int __ReportProfiles_ProcessReportProfilesMsgPackBlob(void *msgpack, bool checkP return T2ERROR_PROFILE_NOT_FOUND; } hash_map_t *profileHashMap; + uint32_t hashmap_count; int count; char *profileNameKey = NULL; int profileIndex; @@ -1325,7 +1339,18 @@ int __ReportProfiles_ProcessReportProfilesMsgPackBlob(void *msgpack, bool checkP } /* Delete profiles not present in the new profile list */ - count = hash_map_count(profileHashMap) - 1; + hashmap_count = hash_map_count(profileHashMap); + // Check for empty hashmap (count == 0) or error condition (UINT32_MAX). + // UINT32_MAX is the documented return value of hash_map_count when map is NULL. + // Setting count = -1 ensures the while(count >= 0) loop below will not execute. + if (hashmap_count == 0 || hashmap_count == UINT32_MAX) + { + count = -1; + } + else + { + count = (int)(hashmap_count - 1); + } while(count >= 0) { profile_found_flag = false; diff --git a/source/bulkdata/t2eventreceiver.c b/source/bulkdata/t2eventreceiver.c index 5099e0e0..a42da073 100644 --- a/source/bulkdata/t2eventreceiver.c +++ b/source/bulkdata/t2eventreceiver.c @@ -134,12 +134,20 @@ void T2ER_PushDataWithDelim(char* eventInfo, char* user_data) { T2Debug("Adding eventName : %s eventValue : %s to t2event queue\n", event->name, event->value); t2_queue_push(eQueue, (void *) event); - if(!stopDispatchThread) + // Check stopDispatchThread with proper lock to avoid stale reads from compiler + // optimizations or CPU caching. We hold sTDMutex while reading stopDispatchThread + // to ensure proper synchronization with the thread that sets this flag. + if(pthread_mutex_lock(&sTDMutex) == 0) { - ret = pthread_cond_signal(&erCond); - if(ret != 0) // pthread cond signal failed so return after unlocking the mutex + bool shouldSignal = !stopDispatchThread; + pthread_mutex_unlock(&sTDMutex); + if(shouldSignal) { - T2Error("%s pthread_cond_signal for erCond failed with error code : %d\n", __FUNCTION__, ret); + ret = pthread_cond_signal(&erCond); + if(ret != 0) // pthread cond signal failed so return after unlocking the mutex + { + T2Error("%s pthread_cond_signal for erCond failed with error code : %d\n", __FUNCTION__, ret); + } } } @@ -204,21 +212,30 @@ void T2ER_Push(char* eventName, char* eventValue) event->value = strdup(eventValue); T2Debug("Adding eventName : %s eventValue : %s to t2event queue\n", event->name, event->value); t2_queue_push(eQueue, (void *) event); - if(!stopDispatchThread) - { - ret = pthread_cond_signal(&erCond); - if(ret != 0) // pthread_cond _signal failed so after unlocking the mutex it will return - { - T2Error("%s pthread_cond_signal for erCond failed with error code : %d\n", __FUNCTION__, ret); - } - } + } } + // Unlock erMutex before acquiring sTDMutex to avoid lock order reversal and potential deadlock if(pthread_mutex_unlock(&erMutex) != 0) { T2Error("%s pthread_mutex_unlock for erMutex failed\n", __FUNCTION__); } + // Signal dispatch thread if needed (after releasing erMutex) + bool shouldSignal = false; + if(pthread_mutex_lock(&sTDMutex) == 0) + { + shouldSignal = !stopDispatchThread; + pthread_mutex_unlock(&sTDMutex); + } + if(shouldSignal) + { + ret = pthread_cond_signal(&erCond); + if(ret != 0) + { + T2Error("%s pthread_cond_signal for erCond failed with error code : %d\n", __FUNCTION__, ret); + } + } free(eventName); free(eventValue); } @@ -235,7 +252,8 @@ void* T2ER_EventDispatchThread(void *arg) (void) arg; // To avoid compiler warning T2Debug("%s ++in\n", __FUNCTION__); Vector *profileList = NULL; - while(!stopDispatchThread) + bool shouldContinue = true; + while(shouldContinue) { if(pthread_mutex_lock(&erMutex) != 0) // mutex lock failed, without locking eQueue shouldn't be accessed { @@ -243,7 +261,40 @@ void* T2ER_EventDispatchThread(void *arg) return NULL; } T2Debug("Checking for events in event queue , event count = %d\n", t2_queue_count(eQueue)); - if(t2_queue_count(eQueue) > 0) + while(t2_queue_count(eQueue) == 0 && shouldContinue) + { + T2Debug("Event Queue size is 0, Waiting events from T2ER_Push\n"); + int ret = pthread_cond_wait(&erCond, &erMutex); + if(ret != 0) // pthread cond wait failed return after unlock + { + T2Error("%s pthread_cond_wait failed with error code: %d\n", __FUNCTION__, ret); + } + T2Debug("Received signal from T2ER_Push\n"); + // Release erMutex before acquiring sTDMutex to avoid lock order reversal and potential deadlock + if(pthread_mutex_unlock(&erMutex) != 0) + { + T2Error("%s pthread_mutex_unlock for erMutex failed\n", __FUNCTION__); + return NULL; + } + // Re-check shouldContinue after waking up from condition wait + if(pthread_mutex_lock(&sTDMutex) == 0) + { + shouldContinue = !stopDispatchThread; + pthread_mutex_unlock(&sTDMutex); + } + else + { + shouldContinue = false; // Exit on lock failure + } + // Re-acquire erMutex before checking queue count again + if(pthread_mutex_lock(&erMutex) != 0) + { + T2Error("%s pthread_mutex_lock for erMutex failed\n", __FUNCTION__); + return NULL; + } + } + + if(t2_queue_count(eQueue) > 0 && shouldContinue) { T2Event *event = (T2Event *)t2_queue_pop(eQueue); if(event == NULL) @@ -280,18 +331,21 @@ void* T2ER_EventDispatchThread(void *arg) } else { - T2Debug("Event Queue size is 0, Waiting events from T2ER_Push\n"); - int ret = pthread_cond_wait(&erCond, &erMutex); - if(ret != 0) // pthread cond wait failed return after unlock - { - T2Error("%s pthread_cond_wait failed with error code: %d\n", __FUNCTION__, ret); - } if(pthread_mutex_unlock(&erMutex) != 0) { T2Error("%s pthread_mutex_unlock for erMutex failed\n", __FUNCTION__); return NULL; } - T2Debug("Received signal from T2ER_Push\n"); + } + // Check if thread should continue + if(pthread_mutex_lock(&sTDMutex) == 0) + { + shouldContinue = !stopDispatchThread; + pthread_mutex_unlock(&sTDMutex); + } + else + { + shouldContinue = false; // Exit on lock failure } } T2Debug("%s --out\n", __FUNCTION__); @@ -345,7 +399,10 @@ T2ERROR T2ER_Init() registerForTelemetryEvents(T2ER_PushDataWithDelim); } - system("touch /tmp/.t2ReadyToReceiveEvents"); + if (system("touch /tmp/.t2ReadyToReceiveEvents") != 0) + { + T2Error("Failed to create /tmp/.t2ReadyToReceiveEvents flag file \n"); + } setT2EventReceiveState(T2_STATE_COMPONENT_READY); T2Info("T2 is now Ready to Recieve Events\n"); @@ -444,14 +501,16 @@ T2ERROR T2ER_StopDispatchThread() } stopDispatchThread = true; + // Release sTDMutex before acquiring erMutex to avoid lock order reversal and potential deadlock + if(pthread_mutex_unlock(&sTDMutex) != 0) + { + T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); + return T2ERROR_FAILURE; + } if(pthread_mutex_lock(&erMutex) != 0) { T2Error("%s pthread_mutex_lock for erMutex failed\n", __FUNCTION__); - if(pthread_mutex_unlock(&sTDMutex) != 0) - { - T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); - } return T2ERROR_FAILURE; } ret = pthread_cond_signal(&erCond); @@ -462,30 +521,25 @@ T2ERROR T2ER_StopDispatchThread() { T2Error("%s pthread_mutex_unlock for erMutex failed\n", __FUNCTION__); } - if(pthread_mutex_unlock(&sTDMutex) != 0) - { - T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); - } return T2ERROR_FAILURE; } if(pthread_mutex_unlock(&erMutex) != 0) { T2Error("%s pthread_mutex_unlock for erMutex failed\n", __FUNCTION__); - if(pthread_mutex_unlock(&sTDMutex) != 0) - { - T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); - } return T2ERROR_FAILURE; } - ret = pthread_detach(erThread); - flushCacheFromFile(); - if(pthread_mutex_unlock(&sTDMutex) != 0) + // Re-acquire sTDMutex to safely access erThread + if(pthread_mutex_lock(&sTDMutex) != 0) { - T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); + T2Error("%s pthread_mutex_lock for sTDMutex failed\n", __FUNCTION__); return T2ERROR_FAILURE; } + ret = pthread_detach(erThread); + pthread_mutex_unlock(&sTDMutex); + + flushCacheFromFile(); T2Debug("%s --out\n", __FUNCTION__); return T2ERROR_SUCCESS; } @@ -500,14 +554,16 @@ void T2ER_Uninit() } EREnabled = false; + pthread_t threadToJoin; + if(pthread_mutex_lock(&sTDMutex) != 0) // mutex lock failed so return from T2ER_Uninit + { + T2Error("%s pthread_mutex_lock for sTDMutex failed\n", __FUNCTION__); + return; + } if(!stopDispatchThread) { - if(pthread_mutex_lock(&sTDMutex) != 0) // mutex lock failed so return from T2ER_Uninit - { - T2Error("%s pthread_mutex_lock for sTDMutex failed\n", __FUNCTION__); - return; - } stopDispatchThread = true; + threadToJoin = erThread; // Save thread handle while holding lock if(pthread_mutex_unlock(&sTDMutex) != 0) //mutex unlock failed so return from T2ER_Uninit { T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); @@ -530,7 +586,7 @@ void T2ER_Uninit() return; } - if(pthread_join(erThread, NULL) != 0) + if(pthread_join(threadToJoin, NULL) != 0) { T2Error("%s erThread join failed\n", __FUNCTION__); } @@ -551,6 +607,14 @@ void T2ER_Uninit() return; } } + else + { + if(pthread_mutex_unlock(&sTDMutex) != 0) + { + T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); + return; + } + } T2Debug("T2ER Event Dispatch Thread successfully terminated\n"); t2_queue_destroy(eQueue, freeT2Event); eQueue = NULL; diff --git a/source/ccspinterface/rbusInterface.c b/source/ccspinterface/rbusInterface.c index d8cec2f7..f745cda4 100644 --- a/source/ccspinterface/rbusInterface.c +++ b/source/ccspinterface/rbusInterface.c @@ -281,7 +281,7 @@ Vector* getRbusProfileParamValues(Vector *paramList, int execcount) if(paramValues != NULL) { paramValues[0] = (tr181ValStruct_t*) malloc(sizeof(tr181ValStruct_t)); - paramValues[0]->parameterName = strdup(param); + paramValues[0]->parameterName = (param != NULL) ? strdup(param) : strdup("UNKNOWN"); paramValues[0]->parameterValue = strdup("NULL"); // If parameter doesn't exist in device we do populate with entry as NULL. // So count of populated data list has 1 entry and is not 0 diff --git a/source/commonlib/telemetry_busmessage_sender.c b/source/commonlib/telemetry_busmessage_sender.c index c5d43127..f2c76171 100644 --- a/source/commonlib/telemetry_busmessage_sender.c +++ b/source/commonlib/telemetry_busmessage_sender.c @@ -755,7 +755,7 @@ void t2_uninit(void) T2ERROR t2_event_s(const char* marker, const char* value) { - int ret; + int ret = -1; T2ERROR retStatus = T2ERROR_FAILURE ; EVENT_DEBUG("%s ++in\n", __FUNCTION__); char* strvalue = NULL; @@ -782,13 +782,13 @@ T2ERROR t2_event_s(const char* marker, const char* value) return T2ERROR_SUCCESS; } strvalue = strdup(value); - if( strvalue[strlen(strvalue) - 1] == '\n' ) - { - strvalue[strlen(strvalue) - 1] = '\0'; - } - ret = report_or_cache_data(strvalue, marker); if(strvalue != NULL) { + if( strvalue[strlen(strvalue) - 1] == '\n' ) + { + strvalue[strlen(strvalue) - 1] = '\0'; + } + ret = report_or_cache_data(strvalue, marker); free(strvalue); } if(ret != -1) diff --git a/source/dcautil/dca.c b/source/dcautil/dca.c index abbc8c78..08f98ec5 100644 --- a/source/dcautil/dca.c +++ b/source/dcautil/dca.c @@ -1017,7 +1017,9 @@ static FileDescriptor* getFileDeltaInMemMapAndSearch(const int fd, const off_t s //create a tmp file for main file fd char tmp_fdmain[] = "/tmp/dca_tmpfile_fdmainXXXXXX"; + mode_t old_umask = umask(0077); // Set secure umask (owner read/write only) int tmp_fd = mkstemp(tmp_fdmain); + umask(old_umask); // Restore original umask if (tmp_fd == -1) { T2Error("Failed to create temp file: %s\n", strerror(errno)); @@ -1044,7 +1046,9 @@ static FileDescriptor* getFileDeltaInMemMapAndSearch(const int fd, const off_t s if (rd != -1 && fstat(rd, &rb) == 0 && rb.st_size > 0) { char tmp_fdrotated[] = "/tmp/dca_tmpfile_fdrotatedXXXXXX"; + mode_t old_umask = umask(0077); // Set secure umask (owner read/write only) int tmp_rd = mkstemp(tmp_fdrotated); + umask(old_umask); // Restore original umask if (tmp_rd == -1) { T2Error("Failed to create temp file: %s\n", strerror(errno)); diff --git a/source/protocol/rbusMethod/rbusmethodinterface.c b/source/protocol/rbusMethod/rbusmethodinterface.c index 02673931..e37d25f3 100644 --- a/source/protocol/rbusMethod/rbusmethodinterface.c +++ b/source/protocol/rbusMethod/rbusmethodinterface.c @@ -50,6 +50,10 @@ static void asyncMethodHandler(rbusHandle_t handle, char const* methodName, rbus (void) params; T2Info("T2 asyncMethodHandler called: %s with return error code = %s \n", methodName, rbusError_ToString(retStatus)); + //Note: The mutex synchronization is handled by the caller (sendReportsOverRBUSMethod) + //which locks the mutex before calling rbusMethodCaller and uses pthread_mutex_trylock + //to wait for this async callback to complete. This callback updates isRbusMethod + //without additional locking as the caller manages the synchronization. if(retStatus == RBUS_ERROR_SUCCESS) { isRbusMethod = true ; diff --git a/source/reportgen/reportgen.c b/source/reportgen/reportgen.c index 88b47051..e491edfb 100644 --- a/source/reportgen/reportgen.c +++ b/source/reportgen/reportgen.c @@ -1210,6 +1210,8 @@ char *prepareHttpUrl(T2HTTP *http) unsigned int index = 0; int params_len = 0; char *url_params = NULL; + char *temp_params = NULL; + char *temp_url = NULL; for(; index < http->RequestURIparamList->count; index++) { @@ -1250,13 +1252,21 @@ char *prepareHttpUrl(T2HTTP *http) { new_params_len += strlen(httpParamVal); } - url_params = realloc(url_params, new_params_len); - if(url_params == NULL) + temp_params = realloc(url_params, new_params_len); + if(temp_params == NULL) { T2Error("Unable to allocate %d bytes of memory at Line %d on %s \n", new_params_len, __LINE__, __FILE__); curl_free(httpParamVal); + // Free url_params to avoid memory leak before continuing + if(url_params) + { + free(url_params); + url_params = NULL; + params_len = 0; // Reset params_len to match freed state + } continue; } + url_params = temp_params; params_len += snprintf(url_params + params_len, new_params_len - params_len, "%s=%s&", httpParam->HttpName, httpParamVal); curl_free(httpParamVal); @@ -1269,7 +1279,16 @@ char *prepareHttpUrl(T2HTTP *http) int url_len = strlen(httpUrl); int modified_url_len = url_len + strlen("?") + strlen(url_params) + 1; - httpUrl = realloc(httpUrl, modified_url_len); + temp_url = realloc(httpUrl, modified_url_len); + if(temp_url == NULL) + { + T2Error("Unable to allocate %d bytes of memory for URL at Line %d on %s \n", modified_url_len, __LINE__, __FILE__); + free(httpUrl); + free(url_params); + curl_easy_cleanup(curl); + return NULL; + } + httpUrl = temp_url; snprintf(httpUrl + url_len, modified_url_len - url_len, "?%s", url_params); } diff --git a/source/scheduler/scheduler.c b/source/scheduler/scheduler.c index bab6fa6a..5ade9a71 100644 --- a/source/scheduler/scheduler.c +++ b/source/scheduler/scheduler.c @@ -200,7 +200,7 @@ void* TimeoutThread(void *arg) T2Error("pthread_condattr_destroy failed \n"); } - while(tProfile->repeat && !tProfile->terminated && tProfile->name) + while(1) { memset(&_ts, 0, sizeof(struct timespec)); memset(&_now, 0, sizeof(struct timespec)); @@ -211,6 +211,16 @@ void* TimeoutThread(void *arg) return NULL; } + // Check loop conditions while holding the lock + if(!tProfile->repeat || tProfile->terminated || !tProfile->name) + { + if(pthread_mutex_unlock(&tProfile->tMutex) != 0) + { + T2Error("tProfile Mutex unlock failed\n"); + } + break; + } + if( clock_gettime(CLOCK_MONOTONIC, &_now) == -1 ) { T2Error("clock_gettime failed\n"); @@ -258,19 +268,31 @@ void* TimeoutThread(void *arg) if(tProfile->firstreportint > 0 && tProfile->firstexecution == true ) { T2Info("Waiting for %d sec for next TIMEOUT for profile as firstreporting interval is given - %s\n", tProfile->firstreportint, tProfile->name); - n = pthread_cond_timedwait(&tProfile->tCond, &tProfile->tMutex, &_ts); + do + { + n = pthread_cond_timedwait(&tProfile->tCond, &tProfile->tMutex, &_ts); + } + while(n == EINTR); } else { if(tProfile->timeOutDuration == UINT_MAX && tProfile->timeRefinSec == 0) { T2Info("Waiting for condition as reporting interval is not configured for profile - %s\n", tProfile->name); - n = pthread_cond_wait(&tProfile->tCond, &tProfile->tMutex); + do + { + n = pthread_cond_wait(&tProfile->tCond, &tProfile->tMutex); + } + while (n == EINTR); } else { T2Info("Waiting for timeref or reporting interval for the profile - %s is started\n", tProfile->name); - n = pthread_cond_timedwait(&tProfile->tCond, &tProfile->tMutex, &_ts); + do + { + n = pthread_cond_timedwait(&tProfile->tCond, &tProfile->tMutex, &_ts); + } + while (n == EINTR); } } if(n == ETIMEDOUT) @@ -633,6 +655,7 @@ T2ERROR unregisterProfileFromScheduler(const char* profileName) if(pthread_mutex_lock(&tProfile->tMutex) != 0) { T2Error("tProfile Mutex lock failed\n"); + pthread_mutex_unlock(&scMutex); return T2ERROR_FAILURE; } tProfile->terminated = true; @@ -647,9 +670,24 @@ T2ERROR unregisterProfileFromScheduler(const char* profileName) T2Info(" tProfile->tId = %d tProfile->name = %s\n", (int)tProfile->tId, tProfile->name); // pthread_join(tProfile->tId, NULL); // pthread_detach in freeSchedulerProfile will detach the thread sched_yield(); // This will give chance for the signal receiving thread to start + int count = 0; - while(signalrecived_and_executing && !is_activation_time_out) + bool is_signal_executing = true; + while(is_signal_executing && !is_activation_time_out) { + if(pthread_mutex_lock(&tProfile->tMutex) != 0) + { + T2Error("tProfile Mutex lock failed\n"); + pthread_mutex_unlock(&scMutex); + return T2ERROR_FAILURE; + } + is_signal_executing = signalrecived_and_executing; + if(pthread_mutex_unlock(&tProfile->tMutex) != 0) + { + T2Error("tProfile Mutex unlock failed\n"); + pthread_mutex_unlock(&scMutex); + return T2ERROR_FAILURE; + } if(count++ > 10) { break; @@ -657,9 +695,12 @@ T2ERROR unregisterProfileFromScheduler(const char* profileName) sleep(1); } - pthread_mutex_lock(&tProfile->tMutex); + // Keep scMutex held across the wait loop to prevent concurrent removal/free of tProfile. + // This avoids using a potentially stale/freed pointer (Coverity ATOMICITY/CWE-662). + // Don't lock tProfile->tMutex here as Vector_RemoveItem will free tProfile via + // freeSchedulerProfile callback, which would cause use-after-free when unlocking. Vector_RemoveItem(profileList, tProfile, freeSchedulerProfile); - pthread_mutex_unlock(&tProfile->tMutex); + T2Debug("%s:%d scMutex is unlocked\n", __FUNCTION__, __LINE__); if(pthread_mutex_unlock(&scMutex) != 0) { diff --git a/source/t2parser/t2parserxconf.c b/source/t2parser/t2parserxconf.c index ba121858..afa19f1a 100644 --- a/source/t2parser/t2parserxconf.c +++ b/source/t2parser/t2parserxconf.c @@ -247,6 +247,8 @@ T2ERROR processConfigurationXConf(char* configData, ProfileXConf **localProfile) ProfileXConf *profile = (ProfileXConf *)malloc(sizeof(ProfileXConf)); memset(profile, 0, sizeof(ProfileXConf)); + pthread_mutex_init(&profile->reportInProgressMutex, NULL); + pthread_cond_init(&profile->reportInProgressCond, NULL); // profile->id = strdup(jprofileID->valuestring); profile->name = strdup(jprofileName->valuestring); profile->reportingInterval = reportIntervalInSec; diff --git a/source/utils/persistence.c b/source/utils/persistence.c index 253ec412..dc0d9a95 100644 --- a/source/utils/persistence.c +++ b/source/utils/persistence.c @@ -464,8 +464,24 @@ T2ERROR getPrivacyModeFromPersistentFolder(char **privMode) T2Error("Unable to open the file : %s\n", filePath); return T2ERROR_FAILURE; } - stat(filePath, &filestat); - fread(data, sizeof(char), filestat.st_size, fp); + if (stat(filePath, &filestat) != 0) + { + T2Error("Unable to stat file : %s\n", filePath); + fclose(fp); + return T2ERROR_FAILURE; + } + if (filestat.st_size < 0 || (size_t)filestat.st_size >= sizeof(data)) + { + T2Error("File size %lld exceeds buffer capacity %zu for file : %s\n", (long long)filestat.st_size, sizeof(data) - 1, filePath); + fclose(fp); + return T2ERROR_FAILURE; + } + if (fread(data, sizeof(char), filestat.st_size, fp) != (size_t)filestat.st_size) + { + T2Error("Failed to read complete data from file : %s\n", filePath); + fclose(fp); + return T2ERROR_FAILURE; + } *privMode = strdup(data); if(fclose(fp) != 0) { diff --git a/source/utils/t2collection.c b/source/utils/t2collection.c index 901b3bfc..73075294 100644 --- a/source/utils/t2collection.c +++ b/source/utils/t2collection.c @@ -105,12 +105,14 @@ void *t2_queue_remove(queue_t *q, uint32_t index) element_t *e, *tmp = NULL; void *data; uint32_t i = 0; + uint32_t count; if(q == NULL) { return NULL; } - if (index > (t2_queue_count(q) - 1)) + count = t2_queue_count(q); + if (count == 0 || index >= count) { return NULL; } @@ -146,12 +148,14 @@ void *t2_queue_peek(queue_t *q, uint32_t index) { element_t *e; uint32_t i = 0; + uint32_t count; if(q == NULL) { return NULL; } - if (index > (t2_queue_count(q) - 1)) + count = t2_queue_count(q); + if (count == 0 || index >= count) { return NULL; } diff --git a/source/xconf-client/xconfclient.c b/source/xconf-client/xconfclient.c index 36577ad5..f6ad550a 100644 --- a/source/xconf-client/xconfclient.c +++ b/source/xconf-client/xconfclient.c @@ -222,7 +222,6 @@ static char *getTimezone () { fclose(file); free(jsonDoc); - jsonDoc = NULL; T2Debug("Failed to read Timezone value from %s file...\n", jsonpath); continue; } @@ -252,7 +251,6 @@ static char *getTimezone () } } free(jsonDoc); - jsonDoc = NULL; cJSON_Delete(root); } count++; @@ -281,7 +279,6 @@ static char *getTimezone () if(zoneValue) { free(zoneValue); - zoneValue = NULL ; } zoneValue = strdup(zone); } @@ -818,7 +815,10 @@ T2ERROR doHttpGet(char* httpsUrl, char **data) // Share data with parent close(sharedPipeFdDataLen[0]); - write(sharedPipeFdDataLen[1], &len, sizeof(size_t)); + if (write(sharedPipeFdDataLen[1], &len, sizeof(size_t)) != sizeof(size_t)) + { + T2Error("Failed to write data length to pipe\n"); + } close(sharedPipeFdDataLen[1]); FILE *httpOutput = fopen(HTTP_RESPONSE_FILE, "w+"); @@ -905,7 +905,10 @@ T2ERROR doHttpGet(char* httpsUrl, char **data) status_return : close(sharedPipeFdStatus[0]); - write(sharedPipeFdStatus[1], &ret, sizeof(T2ERROR)); + if (write(sharedPipeFdStatus[1], &ret, sizeof(T2ERROR)) != sizeof(T2ERROR)) + { + T2Error("Failed to write status to pipe\n"); + } close(sharedPipeFdStatus[1]); exit(0); @@ -1284,7 +1287,7 @@ static void* getUpdatedConfigurationThread(void *data) { (void) data; T2ERROR configFetch = T2ERROR_FAILURE; - T2ERROR urlFetchStatus; + T2ERROR urlFetchStatus = T2ERROR_FAILURE; T2Debug("%s ++in\n", __FUNCTION__); struct timespec _ts; struct timespec _now; @@ -1296,12 +1299,21 @@ static void* getUpdatedConfigurationThread(void *data) #endif pthread_mutex_lock(&xcThreadMutex); stopFetchRemoteConfiguration = false ; + pthread_mutex_unlock(&xcThreadMutex); do { T2Debug("%s while Loop -- START \n", __FUNCTION__); - while(!stopFetchRemoteConfiguration && (urlFetchStatus = getRemoteConfigURL(&configURL)) != T2ERROR_SUCCESS) + pthread_mutex_lock(&xcThreadMutex); + while(!stopFetchRemoteConfiguration) { + pthread_mutex_unlock(&xcThreadMutex); + urlFetchStatus = getRemoteConfigURL(&configURL); + pthread_mutex_lock(&xcThreadMutex); + if(urlFetchStatus == T2ERROR_SUCCESS) + { + break; + } if (urlFetchStatus == T2ERROR_INVALID_RESPONSE) { T2Info("Config URL is not set to valid value. Xconfclient shall not proceed for T1.0 settings fetch attempts \n"); @@ -1315,7 +1327,11 @@ static void* getUpdatedConfigurationThread(void *data) _ts.tv_sec = _now.tv_sec + RFC_RETRY_TIMEOUT; T2Info("Waiting for %d sec before trying getRemoteConfigURL\n", RFC_RETRY_TIMEOUT); - n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); + do + { + n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); + } + while(n == EINTR); if(n == ETIMEDOUT) { T2Info("TIMEDOUT -- trying fetchConfigURLs again\n"); @@ -1330,9 +1346,12 @@ static void* getUpdatedConfigurationThread(void *data) } pthread_mutex_unlock(&xcMutex); } + pthread_mutex_unlock(&xcThreadMutex); + pthread_mutex_lock(&xcThreadMutex); while(!stopFetchRemoteConfiguration) { + pthread_mutex_unlock(&xcThreadMutex); T2ERROR ret = T2ERROR_FAILURE ; if ( urlFetchStatus == T2ERROR_INVALID_RESPONSE ) { @@ -1392,6 +1411,7 @@ static void* getUpdatedConfigurationThread(void *data) free(configData); configData = NULL ; } + pthread_mutex_lock(&xcThreadMutex); break; } else if(ret == T2ERROR_PROFILE_NOT_SET) @@ -1402,6 +1422,7 @@ static void* getUpdatedConfigurationThread(void *data) free(configData); configData = NULL ; } + pthread_mutex_lock(&xcThreadMutex); break; } else @@ -1416,6 +1437,7 @@ static void* getUpdatedConfigurationThread(void *data) { T2Error("Reached max xconf retry counts : %d, Using saved profile if exists until next reboot\n", MAX_XCONF_RETRY_COUNT); xConfRetryCount = 0; + pthread_mutex_lock(&xcThreadMutex); break; } T2Info("Waiting for %d sec before trying fetchRemoteConfiguration, No.of tries : %d\n", XCONF_RETRY_TIMEOUT, xConfRetryCount); @@ -1427,7 +1449,11 @@ static void* getUpdatedConfigurationThread(void *data) clock_gettime(CLOCK_REALTIME, &_now); _ts.tv_sec = _now.tv_sec + XCONF_RETRY_TIMEOUT; - n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); + do + { + n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); + } + while(n == EINTR); if(n == ETIMEDOUT) { T2Info("TIMEDOUT -- trying fetchConfigurations again\n"); @@ -1442,7 +1468,9 @@ static void* getUpdatedConfigurationThread(void *data) } pthread_mutex_unlock(&xcMutex); } + pthread_mutex_lock(&xcThreadMutex); } // End of config fetch while + pthread_mutex_unlock(&xcThreadMutex); if(configFetch == T2ERROR_FAILURE && !ProfileXConf_isSet()) { T2Error("Failed to fetch updated configuration and no saved configurations on disk for XCONF, uninitializing the process\n"); @@ -1467,13 +1495,17 @@ static void* getUpdatedConfigurationThread(void *data) } } #endif + pthread_mutex_lock(&xcThreadMutex); stopFetchRemoteConfiguration = true; T2Debug("%s while Loop -- END; wait for restart event\n", __FUNCTION__); - pthread_cond_wait(&xcThreadCond, &xcThreadMutex); + while(stopFetchRemoteConfiguration && isXconfInit) + { + pthread_cond_wait(&xcThreadCond, &xcThreadMutex); + } + pthread_mutex_unlock(&xcThreadMutex); } while(isXconfInit); //End of do while loop - pthread_mutex_unlock(&xcThreadMutex); // pthread_detach(pthread_self()); commenting this line as thread will detached by stopXConfClient T2Debug("%s --out\n", __FUNCTION__); return NULL; @@ -1482,9 +1514,11 @@ static void* getUpdatedConfigurationThread(void *data) void uninitXConfClient() { T2Debug("%s ++in\n", __FUNCTION__); + pthread_mutex_lock(&xcThreadMutex); if(!stopFetchRemoteConfiguration) { stopFetchRemoteConfiguration = true; + pthread_mutex_unlock(&xcThreadMutex); T2Info("fetchRemoteConfigurationThread signalled to stop\n"); pthread_mutex_lock(&xcMutex); pthread_cond_signal(&xcCond); @@ -1493,11 +1527,12 @@ void uninitXConfClient() } else { + pthread_mutex_unlock(&xcThreadMutex); T2Debug("XConfClientThread is stopped already\n"); } + pthread_mutex_lock(&xcThreadMutex); if(isXconfInit) { - pthread_mutex_lock(&xcThreadMutex); isXconfInit = false; pthread_cond_signal(&xcThreadCond); pthread_mutex_unlock(&xcThreadMutex); @@ -1514,6 +1549,10 @@ void uninitXConfClient() xcCertSelectorFree(); #endif } + else + { + pthread_mutex_unlock(&xcThreadMutex); + } T2Debug("%s --out\n", __FUNCTION__); T2Info("Uninit XConf Client Successful\n"); } @@ -1576,7 +1615,9 @@ T2ERROR initXConfClient() pthread_mutex_init(&xcThreadMutex, NULL); pthread_cond_init(&xcCond, NULL); pthread_cond_init(&xcThreadCond, NULL); + pthread_mutex_lock(&xcThreadMutex); isXconfInit = true ; + pthread_mutex_unlock(&xcThreadMutex); #ifdef LIBRDKCERTSEL_BUILD xcCertSelectorInit(); #endif @@ -1591,8 +1632,10 @@ T2ERROR stopXConfClient() { T2Debug("%s ++in\n", __FUNCTION__); //pthread_detach(xcrThread); - pthread_mutex_lock(&xcMutex); + pthread_mutex_lock(&xcThreadMutex); stopFetchRemoteConfiguration = true; + pthread_mutex_unlock(&xcThreadMutex); + pthread_mutex_lock(&xcMutex); pthread_cond_signal(&xcCond); pthread_mutex_unlock(&xcMutex); T2Debug("%s --out\n", __FUNCTION__); @@ -1602,19 +1645,20 @@ T2ERROR stopXConfClient() T2ERROR startXConfClient() { T2Debug("%s ++in\n", __FUNCTION__); + pthread_mutex_lock(&xcThreadMutex); if (isXconfInit) { + stopFetchRemoteConfiguration = false; + pthread_cond_signal(&xcThreadCond); + pthread_mutex_unlock(&xcThreadMutex); //pthread_create(&xcrThread, NULL, getUpdatedConfigurationThread, NULL); pthread_mutex_lock(&xcMutex); pthread_cond_signal(&xcCond); pthread_mutex_unlock(&xcMutex); - pthread_mutex_lock(&xcThreadMutex); - stopFetchRemoteConfiguration = false; - pthread_cond_signal(&xcThreadCond); - pthread_mutex_unlock(&xcThreadMutex); } else { + pthread_mutex_unlock(&xcThreadMutex); T2Info("getUpdatedConfigurationThread is still active ... Ignore xconf reload \n"); } T2Debug("%s --out\n", __FUNCTION__);