RDK-60291 [telemetry] RDK Coverity Defect Resolution for Device Management#235
RDK-60291 [telemetry] RDK Coverity Defect Resolution for Device Management#235madhubabutt wants to merge 2 commits intodevelopfrom
Conversation
15ccf65 to
b9475ed
Compare
Coverity Issue - Data race conditionA wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. Medium Impact, CWE-none How to fixCheck the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait. Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
b9475ed to
e1a0e6a
Compare
e1a0e6a to
ddb6710
Compare
There was a problem hiding this comment.
Pull request overview
This pull request addresses Coverity static analysis defects in the telemetry Device Management component, focusing on improving thread safety, error handling, and security practices.
Changes:
- Enhanced thread synchronization with proper mutex lock/unlock patterns and condition variable handling
- Added error checking for system calls (write, stat, fread, system) with appropriate error logging
- Fixed potential memory leaks in realloc usage by using temporary pointers
- Improved integer overflow protection when converting uint32_t to int
- Enhanced security by setting secure umask before mkstemp calls
- Replaced insecure rand() with /dev/urandom for better random number generation
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 28 comments.
Show a summary per file
| File | Description |
|---|---|
| source/xconf-client/xconfclient.c | Improved mutex locking patterns for thread safety, added error handling for pipe write operations |
| source/utils/t2collection.c | Fixed potential integer underflow by checking for zero count before subtraction |
| source/utils/persistence.c | Added error checking for stat and fread system calls |
| source/scheduler/scheduler.c | Enhanced thread termination logic and improved mutex handling around sleep operations |
| source/reportgen/reportgen.c | Fixed potential memory leak in realloc by using temporary pointer |
| source/protocol/rbusMethod/rbusmethodinterface.c | Added missing mutex lock for thread-safe access to shared variable |
| source/dcautil/dca.c | Implemented secure temporary file creation with restrictive umask |
| source/commonlib/telemetry_busmessage_sender.c | Added null pointer check before string operations and proper variable initialization |
| source/bulkdata/t2eventreceiver.c | Improved thread synchronization and condition variable handling |
| source/bulkdata/reportprofiles.c | Added integer overflow protection when converting hashmap count to signed int |
| source/bulkdata/profile.c | Fixed lock ordering to prevent deadlocks and replaced rand() with /dev/urandom |
| source/bulkdata/datamodel.c | Enhanced thread safety with improved condition variable and mutex handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ret = report_or_cache_data(strvalue, marker); | ||
| if(strvalue != NULL) | ||
| { | ||
| { |
There was a problem hiding this comment.
There are trailing whitespace characters at the end of this line. Trailing whitespace should be removed to maintain code cleanliness.
| { | |
| { |
source/scheduler/scheduler.c
Outdated
| do { | ||
| n = pthread_cond_timedwait(&tProfile->tCond, &tProfile->tMutex, &_ts); | ||
| } while(n != ETIMEDOUT && n != 0); |
There was a problem hiding this comment.
The do-while loop here will continue retrying even after pthread_cond_timedwait succeeds (returns 0). This breaks the intended behavior where the thread should wake up when signaled via pthread_cond_signal. The loop should only retry on spurious wakeups (when the return value is neither ETIMEDOUT nor 0), not when the call succeeds. Consider using while(n != ETIMEDOUT && n != 0 && n == EINTR) if you only want to handle spurious wakeups, or remove this do-while wrapper if spurious wakeup handling isn't necessary.
source/bulkdata/profile.c
Outdated
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &profile->maxlatencyTime); | ||
| do { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &profile->maxlatencyTime); | ||
| } while(n != ETIMEDOUT && n != 0); |
There was a problem hiding this comment.
The do-while loop here will continue retrying even after pthread_cond_timedwait succeeds (returns 0). This breaks the intended behavior where the thread should wake up when signaled via pthread_cond_signal. The loop should only retry on spurious wakeups (when the return value is neither ETIMEDOUT nor 0), not when the call succeeds. Consider using while(n != ETIMEDOUT && n != 0 && n == EINTR) if you only want to handle spurious wakeups, or remove this do-while wrapper if spurious wakeup handling isn't necessary.
| } while(n != ETIMEDOUT && n != 0); | |
| } while (n == EINTR); |
source/bulkdata/t2eventreceiver.c
Outdated
| bool shouldSignal = false; | ||
| if(pthread_mutex_lock(&sTDMutex) == 0) | ||
| { | ||
| shouldSignal = !stopDispatchThread; | ||
| pthread_mutex_unlock(&sTDMutex); | ||
| } | ||
| if(shouldSignal) |
There was a problem hiding this comment.
There is inconsistent indentation here. Lines 208-214 use tab characters instead of spaces. This should be indented with spaces to match the project's coding style.
| bool shouldSignal = false; | |
| if(pthread_mutex_lock(&sTDMutex) == 0) | |
| { | |
| shouldSignal = !stopDispatchThread; | |
| pthread_mutex_unlock(&sTDMutex); | |
| } | |
| if(shouldSignal) | |
| bool shouldSignal = false; | |
| if(pthread_mutex_lock(&sTDMutex) == 0) | |
| { | |
| shouldSignal = !stopDispatchThread; | |
| pthread_mutex_unlock(&sTDMutex); | |
| } | |
| if(shouldSignal) |
source/bulkdata/profile.c
Outdated
| FILE *urandom = fopen("/dev/urandom", "r"); | ||
| if(urandom != NULL && fread(&random_value, sizeof(random_value), 1, urandom) == 1) | ||
| { | ||
| maxuploadinmilliSec = random_value % (profile->maxUploadLatency - 1); | ||
| fclose(urandom); | ||
| } | ||
| else | ||
| { | ||
| if(urandom != NULL) fclose(urandom); | ||
| maxuploadinmilliSec = (unsigned int)(time(0) % (profile->maxUploadLatency - 1)); |
There was a problem hiding this comment.
There is inconsistent indentation here. Lines 556, 557, 559, 560, 564, and 565 use tab characters instead of spaces. This should be indented with spaces to match the project's coding style.
| 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); | ||
| return NULL; |
There was a problem hiding this comment.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Resource leak
Variable "curl" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
source/bulkdata/t2eventreceiver.c
Outdated
| if(!stopDispatchThread) | ||
|
|
||
| bool shouldSignal = false; | ||
| if(pthread_mutex_lock(&sTDMutex) == 0) |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "sTDMutex" while holding lock "erMutex" (count: 1 / 2).
Medium Impact, CWE-833
ORDER_REVERSAL
ddb6710 to
0ca5ee5
Compare
Coverity Issue - Waiting while holding a lockCall to "sendCachedReportsOverRBUSMethod" might sleep while holding lock "profile->reuseThreadMutex". Medium Impact, CWE-667 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
source/bulkdata/t2eventreceiver.c
Outdated
| if(!stopDispatchThread) | ||
|
|
||
| bool shouldSignal = false; | ||
| if(pthread_mutex_lock(&sTDMutex) == 0) |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "sTDMutex" while holding lock "erMutex" (count: 1 / 2).
Medium Impact, CWE-833
ORDER_REVERSAL
0ca5ee5 to
421dbea
Compare
source/bulkdata/t2eventreceiver.c
Outdated
| if(!stopDispatchThread) | ||
|
|
||
| bool shouldSignal = false; | ||
| if(pthread_mutex_lock(&sTDMutex) == 0) |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "sTDMutex" while holding lock "erMutex" (count: 1 / 2).
Medium Impact, CWE-833
ORDER_REVERSAL
421dbea to
b7306d4
Compare
source/bulkdata/profile.c
Outdated
| T2Info("TIMEOUT for maxUploadLatency of profile %s\n", profile->name); | ||
| ret = sendReportsOverRBUSMethod(profile->t2RBUSDest->rbusMethodName, profile->t2RBUSDest->rbusMethodParamList, jsonReport); | ||
| } | ||
| else if(n == 0) |
There was a problem hiding this comment.
Inconsistent use of tabs for indentation. The code appears to use spaces for indentation elsewhere. This should be consistent with the rest of the codebase.
| else if(n == 0) | |
| else if(n == 0) |
| while(t2_queue_count(tmpRpQueue) == 0 && shouldContinue) | ||
| { | ||
| pthread_cond_wait(&tmpRpCond, &tmpRpMutex); | ||
| } |
There was a problem hiding this comment.
Similar to process_rp_thread, the condition variable wait loop checks shouldContinue but doesn't re-evaluate it after pthread_cond_wait returns. If stopProcessing is set while waiting, the thread will continue processing instead of exiting. The shouldContinue variable should be re-checked after pthread_cond_wait returns.
source/xconf-client/xconfclient.c
Outdated
| { | ||
| n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); | ||
| } | ||
| while(n == EINTR); |
There was a problem hiding this comment.
Inconsistent use of tabs and spaces for indentation. The code appears to use spaces for indentation elsewhere, but tabs are used here. This should be consistent with the rest of the codebase.
| { | |
| n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); | |
| } | |
| while(n == EINTR); | |
| { | |
| n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); | |
| } | |
| while(n == EINTR); |
source/reportgen/reportgen.c
Outdated
| 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); | ||
| return NULL; | ||
| } | ||
| httpUrl = temp_url; |
There was a problem hiding this comment.
Inconsistent use of tabs for indentation. The code appears to use spaces for indentation elsewhere. This should be consistent with the rest of the codebase.
source/bulkdata/t2eventreceiver.c
Outdated
| 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 |
There was a problem hiding this comment.
Inconsistent use of tabs for indentation. The code appears to use spaces for indentation elsewhere. This should be consistent with the rest of the codebase.
source/scheduler/scheduler.c
Outdated
|
|
||
| // Unlock scMutex before sleeping to avoid holding lock during sleep | ||
| if(pthread_mutex_unlock(&scMutex) != 0) | ||
| { | ||
| T2Error("scMutex unlock failed\n"); | ||
| return T2ERROR_FAILURE; |
There was a problem hiding this comment.
Inconsistent use of tabs for indentation. The code appears to use spaces for indentation elsewhere. This should be consistent with the rest of the codebase.
source/bulkdata/profile.c
Outdated
| FILE *urandom = fopen("/dev/urandom", "r"); | ||
| if(urandom != NULL && fread(&random_value, sizeof(random_value), 1, urandom) == 1) | ||
| { | ||
| maxuploadinmilliSec = random_value % (profile->maxUploadLatency - 1); | ||
| fclose(urandom); | ||
| } | ||
| else | ||
| { | ||
| if(urandom != NULL) fclose(urandom); | ||
| maxuploadinmilliSec = (unsigned int)(time(0) % (profile->maxUploadLatency - 1)); | ||
| } |
There was a problem hiding this comment.
Potential division by zero error if profile->maxUploadLatency is 1. The expression (profile->maxUploadLatency - 1) would be 0, causing a modulo by zero which results in undefined behavior. Similar issue exists at line 565. Add a check to ensure maxUploadLatency is greater than 1 before performing the modulo operation.
| FILE *urandom = fopen("/dev/urandom", "r"); | |
| if(urandom != NULL && fread(&random_value, sizeof(random_value), 1, urandom) == 1) | |
| { | |
| maxuploadinmilliSec = random_value % (profile->maxUploadLatency - 1); | |
| fclose(urandom); | |
| } | |
| else | |
| { | |
| if(urandom != NULL) fclose(urandom); | |
| maxuploadinmilliSec = (unsigned int)(time(0) % (profile->maxUploadLatency - 1)); | |
| } | |
| if (profile->maxUploadLatency > 1) | |
| { | |
| FILE *urandom = fopen("/dev/urandom", "r"); | |
| if(urandom != NULL && fread(&random_value, sizeof(random_value), 1, urandom) == 1) | |
| { | |
| maxuploadinmilliSec = random_value % (profile->maxUploadLatency - 1); | |
| fclose(urandom); | |
| } | |
| else | |
| { | |
| if(urandom != NULL) fclose(urandom); | |
| maxuploadinmilliSec = (unsigned int)(time(0) % (profile->maxUploadLatency - 1)); | |
| } | |
| } | |
| else | |
| { | |
| /* For maxUploadLatency == 1, avoid modulo by zero and use no additional delay */ | |
| maxuploadinmilliSec = 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"); | ||
| } |
There was a problem hiding this comment.
The condition variable wait loop checks shouldContinue but doesn't re-evaluate it after pthread_cond_wait returns. If stopDispatchThread is set while waiting on the condition, the thread will continue processing the event instead of exiting. The shouldContinue variable should be re-checked after pthread_cond_wait returns at line 257.
source/bulkdata/t2eventreceiver.c
Outdated
| if(!stopDispatchThread) | ||
|
|
||
| bool shouldSignal = false; | ||
| if(pthread_mutex_lock(&sTDMutex) == 0) |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "sTDMutex" while holding lock "erMutex" (count: 1 / 2).
Medium Impact, CWE-833
ORDER_REVERSAL
b7306d4 to
958bb14
Compare
source/bulkdata/t2eventreceiver.c
Outdated
| if(!stopDispatchThread) | ||
|
|
||
| bool shouldSignal = false; | ||
| if(pthread_mutex_lock(&sTDMutex) == 0) |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "sTDMutex" while holding lock "erMutex" (count: 1 / 2).
Medium Impact, CWE-833
ORDER_REVERSAL
28c66a1 to
ac77f39
Compare
source/bulkdata/t2eventreceiver.c
Outdated
| if(!stopDispatchThread) | ||
|
|
||
| bool shouldSignal = false; | ||
| if(pthread_mutex_lock(&sTDMutex) == 0) |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "sTDMutex" while holding lock "erMutex" (count: 1 / 2).
Medium Impact, CWE-833
ORDER_REVERSAL
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| pthread_mutex_lock(&profile->reuseThreadMutex); |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "_Profile.reuseThreadMutex" while holding lock "_Profile.reportMutex" (count: 0 / 1).
Medium Impact, CWE-833
ORDER_REVERSAL
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| pthread_mutex_lock(&profile->reuseThreadMutex); |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "_Profile.reuseThreadMutex" while holding lock "_Profile.reportMutex" (count: 0 / 1).
Medium Impact, CWE-833
ORDER_REVERSAL
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
source/xconf-client/xconfclient.c:231
- In getTimezone(), jsonDoc is allocated with (numbytes + 1) but the buffer is never NUL-terminated after fread(). cJSON_Parse expects a NUL-terminated string; if the last byte is uninitialized this can cause parse failures or out-of-bounds reads. After a successful read, explicitly set jsonDoc[numbytes] = '\0' (or memset the buffer) before calling cJSON_Parse.
fseek(file, 0, SEEK_END);
long numbytes = ftell(file);
jsonDoc = (char*)malloc(sizeof(char) * (numbytes + 1));
fseek(file, 0, SEEK_SET);
//CID 190258: Argument cannot be negative (NEGATIVE_RETURNS)
if (numbytes > 0 )
{
if (fread(jsonDoc, numbytes, 1, file) != 1)
{
fclose(file);
free(jsonDoc);
T2Debug("Failed to read Timezone value from %s file...\n", jsonpath);
continue;
}
}
fclose(file);
cJSON *root = cJSON_Parse(jsonDoc);
if (root != NULL)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| url_params = temp_params; | ||
| params_len += snprintf(url_params + params_len, new_params_len - params_len, "%s=%s&", httpParam->HttpName, httpParamVal); | ||
|
|
||
| curl_free(httpParamVal); |
There was a problem hiding this comment.
httpParamVal can be NULL if curl_easy_escape fails (e.g., OOM). The current snprintf("%s=%s&", ..., httpParamVal) will dereference a NULL pointer via %s. Add an explicit NULL check for httpParamVal and skip the param / return error before calling snprintf.
| 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; | ||
| } | ||
| 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); | ||
|
|
There was a problem hiding this comment.
unregisterProfileFromScheduler() can call Vector_RemoveItem(..., freeSchedulerProfile) while the TimeoutThread is still running. freeSchedulerProfile destroys tMutex/tCond and frees tProfile, which is undefined behavior if the thread hasn’t exited yet. The current wait loop only waits for signalrecived_and_executing to flip, which doesn’t guarantee thread termination. Ensure the timeout thread has exited (e.g., set terminated=true, signal, then pthread_join(tProfile->tId, NULL) before freeing/destroying).
| T2Error("Profile : %s pthread_cond_timedwait ERROR!!!\n", profile->name); | ||
| pthread_mutex_unlock(&profile->reportMutex); | ||
| pthread_cond_destroy(&profile->reportcond); | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; | ||
| pthread_mutex_unlock(&profile->reportInProgressMutex); |
There was a problem hiding this comment.
In the RBUS_METHOD maxUploadLatency error path (pthread_cond_timedwait returning unexpected error), the code destroys reportcond and jumps to reportThreadEnd without destroying reportMutex. Since reportMutex was initialized when maxUploadLatency > 0, it should be destroyed on this error path as well to avoid leaking the mutex / reinitializing an already-initialized mutex.
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| pthread_mutex_lock(&profile->reuseThreadMutex); |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "_Profile.reuseThreadMutex" while holding lock "_Profile.reportMutex" (count: 0 / 1).
Medium Impact, CWE-833
ORDER_REVERSAL
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| pthread_mutex_lock(&profile->reuseThreadMutex); |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "_Profile.reuseThreadMutex" while holding lock "_Profile.reportMutex" (count: 0 / 1).
Medium Impact, CWE-833
ORDER_REVERSAL
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| pthread_mutex_lock(&profile->reuseThreadMutex); |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "_Profile.reuseThreadMutex" while holding lock "_Profile.reportMutex" (count: 0 / 1).
Medium Impact, CWE-833
ORDER_REVERSAL
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| pthread_mutex_lock(&profile->reuseThreadMutex); |
There was a problem hiding this comment.
Coverity Issue - Thread deadlock
Calling "pthread_mutex_lock" acquires lock "_Profile.reuseThreadMutex" while holding lock "_Profile.reportMutex" (count: 0 / 1).
Medium Impact, CWE-833
ORDER_REVERSAL
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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]); |
There was a problem hiding this comment.
doHttpGet(): write() to the pipe can return a short count or be interrupted. Logging when write(...) != sizeof(size_t) but continuing can cause the parent to read an incomplete length and mis-handle the response. Consider looping until all bytes are written (handling EINTR) and treating failure as an error 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]); |
There was a problem hiding this comment.
doHttpGet(): same issue for the status pipe—write() may be short or interrupted. If this happens, the parent may read a corrupted/partial status. Consider a write-all helper and fail the child path if the status cannot be written reliably.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| T2Error("Profile : %s pthread_cond_timedwait ERROR!!!\n", profile->name); | ||
| pthread_mutex_unlock(&profile->reportMutex); | ||
| pthread_cond_destroy(&profile->reportcond); | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; | ||
| pthread_mutex_unlock(&profile->reportInProgressMutex); | ||
| if(profile->triggerReportOnCondition) |
There was a problem hiding this comment.
After initializing profile->reportMutex/profile->reportcond for maxUploadLatency, the RBUS_METHOD error path destroys only the condition variable and then continues/gotos without destroying profile->reportMutex. This can leak the mutex and makes subsequent pthread_mutex_init(&profile->reportMutex, ...) undefined if the loop repeats. Ensure both the cond var and mutex are destroyed on all exit paths after they’re initialized.
| else | ||
| { | ||
| if(pthread_mutex_unlock(&sTDMutex) != 0) | ||
| { | ||
| T2Error("%s pthread_mutex_unlock for sTDMutex failed\n", __FUNCTION__); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
When stopDispatchThread is already true, T2ER_Uninit only unlocks sTDMutex and skips destroying erMutex, sTDMutex, and erCond. Since EREnabled is set false and eQueue is destroyed, this leaves synchronization primitives allocated for the lifetime of the process. Consider destroying the mutexes/cond regardless of whether the dispatch thread was running, and only conditionally pthread_join when a joinable thread exists.
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (1) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (1) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (1) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (1) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (profile->enable && n != ETIMEDOUT && n != 0) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Logically dead code
Execution cannot reach this statement: "n = pthread_cond_timedwait(...".
Medium Impact, CWE-561
DEADCODE
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (profile->enable && n != ETIMEDOUT && n != 0) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Logically dead code
Execution cannot reach this statement: "n = pthread_cond_timedwait(...".
Medium Impact, CWE-561
DEADCODE
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (profile->enable && n != ETIMEDOUT && n != 0) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Logically dead code
Execution cannot reach this statement: "n = pthread_cond_timedwait(...".
Medium Impact, CWE-561
DEADCODE
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (profile->enable && n != ETIMEDOUT && n != 0) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Logically dead code
Execution cannot reach this statement: "n = pthread_cond_timedwait(...".
Medium Impact, CWE-561
DEADCODE
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 |
There was a problem hiding this comment.
Temporarily changing process umask is not thread-safe and can affect file creation in other threads. mkstemp already creates files with 0600 permissions; prefer fchmod() on the returned fd instead of modifying umask.
| 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 | |
| int tmp_rd = mkstemp(tmp_fdrotated); |
| if (filestat.st_size < 0 || (size_t)filestat.st_size >= sizeof(data)) | ||
| { | ||
| T2Error("File size %ld exceeds buffer capacity %zu for file : %s\n", filestat.st_size, sizeof(data) - 1, filePath); | ||
| fclose(fp); |
There was a problem hiding this comment.
stat.st_size is an off_t and may not match %ld on all platforms (can trigger -Wformat warnings or incorrect logging). Use an appropriate format (e.g., cast to intmax_t and print with %jd) and include <inttypes.h> if needed.
| 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) | ||
| { |
There was a problem hiding this comment.
threadExists is written under reuseThreadMutex elsewhere, but here it’s read while holding reportInProgressMutex. That does not synchronize with the writer and is still a data race. Read/write threadExists under the same mutex (reuseThreadMutex) or make it an atomic to avoid undefined behavior.
source/bulkdata/profile.c
Outdated
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); | ||
| } | ||
| while (profile->enable && n == EINTR); | ||
| 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); | ||
| } |
There was a problem hiding this comment.
pthread_cond_timedwait can return 0 due to spurious wakeups. In this codebase, profile->reportcond is never signaled, so treating n==0 as “skip report upload” can randomly drop uploads. Use a predicate-based loop (wait until timeout or an explicit cancellation flag) instead of skipping on n==0.
| 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); | ||
| } |
There was a problem hiding this comment.
pthread_cond_timedwait can return 0 due to spurious wakeups; treating n==0 as “skip report upload” can drop RBUS_METHOD uploads unexpectedly. Use a predicate-based loop (timeout or explicit cancellation condition) instead of skipping on n==0.
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
| pthread_mutex_lock(&profile->reportMutex); | ||
| do | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
There was a problem hiding this comment.
Coverity Issue - Indefinite wait
A wait is performed without ensuring that the condition is not already satisfied while holding lock "_Profile.reportMutex". This can cause a deadlock if the notification happens before the lock is acquired.
High Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Acquire the lock, then check the wait condition in a loop, without releasing with the lock before the wait. This will prevent deadlocks and failed conditions from spurious wakeups.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pthread_mutex_lock(&profile->reportMutex); | ||
| while (profile->enable && n != ETIMEDOUT) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); | ||
| } |
There was a problem hiding this comment.
Same maxUploadLatency issue as the HTTP path: the loop keeps waiting even after being signaled (n==0), so uploads won’t be skipped on early signal. Adjust the wait logic to break on n==0 using an explicit predicate, and only send when ETIMEDOUT.
| 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); |
There was a problem hiding this comment.
Same data race as in NotifyTimeout: threadExists is read under reportInProgressMutex, but updates are guarded by reuseThreadMutex. Read it under reuseThreadMutex (or make it atomic) so deleteProfile’s decision to join/signal the thread is synchronized correctly.
| while (profile->enable && n != ETIMEDOUT) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); | ||
| } |
There was a problem hiding this comment.
The maxUploadLatency wait loop ignores early signals: n starts at 0, and the loop condition is while (profile->enable && n != ETIMEDOUT), so even if pthread_cond_timedwait returns 0 (signaled), it will immediately wait again until ETIMEDOUT. This makes the else if (n == 0) branch effectively unreachable and defeats the intended “skip upload on signal” behavior. Use a predicate (e.g., restartRequested/enable) and break on n==0, or remove the loop and rely on the predicate check for spurious wakeups.
| while (profile->enable && n != ETIMEDOUT) | |
| { | |
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); | |
| } | |
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); |
| T2Debug(" profile->triggerReportOnCondition is not set \n"); | ||
| } | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportInProgress" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportInProgress" is written to with "_Profile.reuseThreadMutex" held 11 out of 11 times.
Medium Impact, CWE-366
MISSING_LOCK
| T2Debug(" profile->triggerReportOnCondition is not set \n"); | ||
| } | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportInProgress" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportInProgress" is written to with "_Profile.reuseThreadMutex" held 11 out of 11 times.
Medium Impact, CWE-366
MISSING_LOCK
| pthread_mutex_unlock(&profile->reuseThreadMutex); | ||
| T2Info("%s --out Exiting collect and report Thread\n", __FUNCTION__); | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportInProgress" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportInProgress" is written to with "_Profile.reuseThreadMutex" held 11 out of 11 times.
Medium Impact, CWE-366
MISSING_LOCK
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (profile->enable && n != ETIMEDOUT) | ||
| { | ||
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); | ||
| } |
There was a problem hiding this comment.
Same issue in the RBUS_METHOD maxUploadLatency path: the while(profile->enable && n != ETIMEDOUT) loop won't break on n==0, so a signal won't cause an early exit as the subsequent logging suggests. Use a single timedwait (retry only on EINTR) or introduce a predicate and break appropriately.
| while (profile->enable && n != ETIMEDOUT) | |
| { | |
| n = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); | |
| } | |
| int waitResult; | |
| do | |
| { | |
| waitResult = pthread_cond_timedwait(&profile->reportcond, &profile->reportMutex, &timeout); | |
| } while (waitResult == EINTR); | |
| n = waitResult; |
| if (write(sharedPipeFdStatus[1], &ret, sizeof(T2ERROR)) != sizeof(T2ERROR)) | ||
| { | ||
| T2Error("Failed to write status to pipe\n"); | ||
| } |
There was a problem hiding this comment.
The status pipe write() result is checked, but a short/failed write is only logged and the child still exits normally. This can cause the parent to read an uninitialized/partial T2ERROR value. Consider treating a short/failed write as fatal (set a fallback failure code and exit), and have the parent verify it read exactly sizeof(T2ERROR).
| if (filestat.st_size < 0 || (size_t)filestat.st_size >= sizeof(data)) | ||
| { | ||
| T2Error("File size %ld exceeds buffer capacity %zu for file : %s\n", filestat.st_size, sizeof(data) - 1, filePath); | ||
| fclose(fp); |
There was a problem hiding this comment.
filestat.st_size is off_t, but the new error message prints it with %ld. On some platforms off_t is not long (e.g., long long), which can cause incorrect logs/UB. Use a portable format (e.g., cast to intmax_t and print with %jd) or cast to long long and use %lld consistently.
| T2Debug(" profile->triggerReportOnCondition is not set \n"); | ||
| } | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportInProgress" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportInProgress" is written to with "_Profile.reuseThreadMutex" held 11 out of 11 times.
Medium Impact, CWE-366
MISSING_LOCK
| T2Debug(" profile->triggerReportOnCondition is not set \n"); | ||
| } | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportInProgress" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportInProgress" is written to with "_Profile.reuseThreadMutex" held 11 out of 11 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| T2Error("Failed to initialize JSON Report\n"); | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportInProgress" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportInProgress" is written to with "_Profile.reuseThreadMutex" held 11 out of 11 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| T2Error("Failed to initialize JSON Report\n"); | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = false; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportInProgress" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportInProgress" is written to with "_Profile.reuseThreadMutex" held 11 out of 11 times.
Medium Impact, CWE-366
MISSING_LOCK
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
source/bulkdata/profile.c:645
- In this error path, reportcond is destroyed but reportMutex (initialized earlier when maxUploadLatency > 0) is not destroyed before jumping to reportThreadEnd. This can leak resources and make subsequent pthread_mutex_init on reportMutex undefined. Ensure reportMutex is destroyed on this error path as well.
T2Error("Profile : %s pthread_cond_timedwait ERROR!!!\n", profile->name);
pthread_mutex_unlock(&profile->reportMutex);
pthread_cond_destroy(&profile->reportcond);
if(httpUrl)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); |
There was a problem hiding this comment.
In this RBUS_METHOD timedwait error path, reportcond is destroyed but reportMutex (initialized earlier when maxUploadLatency > 0) is not destroyed before jumping to reportThreadEnd. This can leak resources and make subsequent pthread_mutex_init on reportMutex undefined. Ensure reportMutex is destroyed on this error path (and consistently on all paths that destroy reportcond).
| if (write(sharedPipeFdStatus[1], &ret, sizeof(T2ERROR)) != sizeof(T2ERROR)) | ||
| { | ||
| T2Error("Failed to write status to pipe\n"); |
There was a problem hiding this comment.
If writing the status to the pipe fails (short write or -1), the parent may read an uninitialized/partial status and take the wrong code path. Consider making pipe write failure fatal (e.g., log, set an error status, and exit) rather than continuing with an unreliable IPC result.
| if (write(sharedPipeFdStatus[1], &ret, sizeof(T2ERROR)) != sizeof(T2ERROR)) | |
| { | |
| T2Error("Failed to write status to pipe\n"); | |
| { | |
| size_t totalWritten = 0; | |
| unsigned char *buf = (unsigned char *)&ret; | |
| while (totalWritten < sizeof(T2ERROR)) | |
| { | |
| ssize_t bytes = write(sharedPipeFdStatus[1], buf + totalWritten, sizeof(T2ERROR) - totalWritten); | |
| if (bytes < 0) | |
| { | |
| T2Error("Failed to write status to pipe: write error\n"); | |
| close(sharedPipeFdStatus[1]); | |
| exit(EXIT_FAILURE); | |
| } | |
| if (bytes == 0) | |
| { | |
| T2Error("Failed to write status to pipe: wrote 0 bytes\n"); | |
| close(sharedPipeFdStatus[1]); | |
| exit(EXIT_FAILURE); | |
| } | |
| totalWritten += (size_t)bytes; | |
| } |
| do | ||
| { | ||
| n = pthread_cond_timedwait(&xcCond, &xcMutex, &_ts); | ||
| } | ||
| while(n == EINTR); |
There was a problem hiding this comment.
Same pattern here: xcThreadMutex is held while doing pthread_cond_timedwait on xcCond with xcMutex. This can prevent stop/uninit from acquiring xcThreadMutex to signal the condition, causing long delays/hangs. Consider not holding xcThreadMutex across the timed wait (use a single mutex for the stop flag + condition, or unlock/relock around the wait).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
source/bulkdata/profile.c:1293
- deleteAllProfiles() writes tempProfile->threadExists = false without holding reuseThreadMutex (or any other lock consistently protecting threadExists). Since CollectAndReport() updates/destroys reuseThreadMutex based on threadExists, this unsynchronized write can race with the worker thread and lead to signaling/joining the wrong state. Update threadExists under reuseThreadMutex (or make it atomic) consistently.
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);
tempProfile->threadExists = false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pthread_mutex_init(&profile->reportMutex, NULL); | ||
| pthread_cond_init(&profile->reportcond, NULL); | ||
| unsigned int random_value = 0; |
There was a problem hiding this comment.
CollectAndReport() initializes profile->reportMutex/profile->reportcond when maxUploadLatency > 0, but teardown is not symmetric across all branches (e.g., the HTTP path destroys both, while the RBUS_METHOD maxUploadLatency path later only destroys the cond var). This can leak pthread resources across repeated reports. Ensure reportMutex is destroyed on every path that initializes it (success, error, and both protocol branches).
| params_len += snprintf(url_params + params_len, new_params_len - params_len, "%s=%s&", httpParam->HttpName, httpParamVal); | ||
|
|
There was a problem hiding this comment.
prepareHttpUrl() calls snprintf with httpParamVal using a "%s" format, but httpParamVal can be NULL if curl_easy_escape fails (OOM) or returns NULL. Passing NULL to "%s" is undefined behavior and can crash. Add an explicit check for httpParamVal == NULL and skip/abort building that parameter before calling snprintf.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| T2Error("Failed to create /tmp/.t2ReadyToReceiveEvents flag file \n"); | ||
| } | ||
| setT2EventReceiveState(T2_STATE_COMPONENT_READY); | ||
| T2Info("T2 is now Ready to Recieve Events\n"); |
There was a problem hiding this comment.
Typo in log message: "Recieve" -> "Receive".
| T2Info("T2 is now Ready to Recieve Events\n"); | |
| T2Info("T2 is now Ready to Receive Events\n"); |
| pthread_mutex_lock(&tmpRpMutex); | ||
| pthread_mutex_lock(&rpMutex); | ||
| shouldContinue = !stopProcessing; | ||
| pthread_mutex_unlock(&rpMutex); | ||
| if(!shouldContinue) |
There was a problem hiding this comment.
tmpRpQueue operations are protected by tmpRpMutex in this thread, but producers push to tmpRpQueue under rpMutex (see datamodel_processProfile using pthread_mutex_lock(&rpMutex) before t2_queue_push(tmpRpQueue, ...)). This means tmpRpQueue is accessed under different mutexes, which is a data race and can corrupt the queue. Use a single mutex consistently for tmpRpQueue (and pair tmpRpCond with that same mutex), or remove the extra mutex.
| 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); |
There was a problem hiding this comment.
This thread waits on tmpRpCond, but datamodel_unInit() never signals tmpRpCond when setting stopProcessing = true. If tmpRpQueue is empty, pthread_join(tmpRpThread, ...) can hang indefinitely. Ensure datamodel_unInit() signals/broadcasts tmpRpCond during shutdown so this wait can be released and the thread can exit.
No description provided.