RDK-60291 [telemetry] RDK Coverity Defect Resolution for Device Management #235
Conversation
madhubabutt
force-pushed
the
feature/RDK-60291
branch
3 times, most recently
from
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: |
| // Release reuseThreadMutex before acquiring reportInProgressMutex to maintain lock order | ||
| pthread_mutex_unlock(&profile->reuseThreadMutex); | ||
| pthread_mutex_lock(&profile->reportInProgressMutex); | ||
| profile->reportInProgress = true; |
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
source/xconf-client/xconfclient.c
Outdated
| pthread_mutex_lock(&xcThreadMutex); | ||
| stopFetchRemoteConfiguration = true; | ||
| T2Debug("%s while Loop -- END; wait for restart event\n", __FUNCTION__); | ||
| pthread_cond_wait(&xcThreadCond, &xcThreadMutex); |
There was a problem hiding this comment.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Data race condition
A wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied.
Medium Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Check the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait.
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
b9475ed to
e1a0e6a
Compare
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
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.
source/xconf-client/xconfclient.c
Outdated
| do { | ||
| n = pthread_cond_timedwait(&xcCond, &xcMutex, &_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.
| hashmap_count = hash_map_count(profileHashMap); | ||
| if (hashmap_count == 0 || hashmap_count == UINT32_MAX) | ||
| { | ||
| count = -1; | ||
| } | ||
| else | ||
| { | ||
| count = (int)(hashmap_count - 1); | ||
| } |
There was a problem hiding this comment.
The check for hashmap_count == UINT32_MAX is checking for an edge case that is unlikely to occur in practice and may not be the correct way to detect an error from hash_map_count. If hash_map_count returns UINT32_MAX to indicate an error (which would be unusual), this should be documented or verified. More importantly, when hashmap_count == 0, setting count = -1 is correct for the while loop condition while(count >= 0) to immediately exit, but the check should ensure this is the intended behavior. Consider adding a comment explaining why UINT32_MAX is checked.
source/scheduler/scheduler.c
Outdated
| break; | ||
| } | ||
|
|
||
| if( clock_gettime(CLOCK_MONOTONIC, &_now) == -1 ) |
There was a problem hiding this comment.
There is inconsistent indentation here. The line uses a tab character instead of spaces. This should be indented with spaces to match the project's coding style.
| if( clock_gettime(CLOCK_MONOTONIC, &_now) == -1 ) | |
| if( clock_gettime(CLOCK_MONOTONIC, &_now) == -1 ) |
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; | ||
| } | ||
|
|
||
| int count = 0; |
There was a problem hiding this comment.
There is inconsistent indentation here. Lines 666-674 use tab characters instead of spaces. This should be indented with spaces to match the project's coding style.
| // Unlock scMutex before sleeping to avoid holding lock during sleep | |
| if(pthread_mutex_unlock(&scMutex) != 0) | |
| { | |
| T2Error("scMutex unlock failed\n"); | |
| return T2ERROR_FAILURE; | |
| } | |
| int count = 0; | |
| // Unlock scMutex before sleeping to avoid holding lock during sleep | |
| if(pthread_mutex_unlock(&scMutex) != 0) | |
| { | |
| T2Error("scMutex unlock failed\n"); | |
| return T2ERROR_FAILURE; | |
| } | |
| int count = 0; |
| 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/xconf-client/xconfclient.c
Outdated
| stopFetchRemoteConfiguration = false; | ||
| pthread_cond_signal(&xcThreadCond); | ||
| pthread_mutex_unlock(&xcThreadMutex); | ||
| //pthread_create(&xcrThread, NULL, getUpdatedConfigurationThread, NULL); |
There was a problem hiding this comment.
There is inconsistent indentation here. The line uses a tab character instead of spaces. This should be indented with spaces to match the project's coding style.
| //pthread_create(&xcrThread, NULL, getUpdatedConfigurationThread, NULL); | |
| //pthread_create(&xcrThread, NULL, getUpdatedConfigurationThread, NULL); |
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.
| while(!stopProcessing) | ||
| while(!shouldContinue) | ||
| { | ||
| pthread_mutex_lock(&tmpRpMutex); |
There was a problem hiding this comment.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Logically dead code
Execution cannot reach this statement: "pthread_mutex_lock(&tmpRpMu...".
Medium Impact, CWE-561
DEADCODE
| 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
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
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
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
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
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
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
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
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
madhubabutt
force-pushed
the
feature/RDK-60291
branch
2 times, most recently
from
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
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
0b1354a to
185d4f0
Compare
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
cc5fd6c to
e684017
Compare
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| profile->maxlatencyTime.tv_sec += maxuploadinSec; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| profile->maxlatencyTime.tv_sec += maxuploadinSec; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
a15a7bd to
fa54c51
Compare
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.
| pthread_mutex_lock(&xcThreadMutex); | ||
| stopFetchRemoteConfiguration = false ; | ||
| pthread_mutex_unlock(&xcThreadMutex); |
There was a problem hiding this comment.
The mutex is unlocked immediately after being locked at line 1300, before the do-while loop begins. This creates a window where stopFetchRemoteConfiguration could be modified by another thread between the unlock (line 1302) and the lock at line 1307, leading to a race condition. The unlock should be removed here since line 1307 locks the mutex again at the start of the while loop.
| pthread_mutex_lock(&xcThreadMutex); | |
| stopFetchRemoteConfiguration = false ; | |
| pthread_mutex_unlock(&xcThreadMutex); |
| } | ||
| pthread_mutex_unlock(&xcMutex); | ||
| } | ||
| pthread_mutex_unlock(&xcThreadMutex); |
There was a problem hiding this comment.
At line 1323, xcMutex is locked while xcThreadMutex is already held (from line 1312). This creates a nested lock acquisition pattern. However, at lines 1347 and 1349, xcThreadMutex is unlocked followed immediately by locking it again at line 1351. This unlock-relock sequence at lines 1349-1351 serves no purpose and adds unnecessary overhead. The unlock at 1349 should be removed.
| pthread_mutex_unlock(&xcThreadMutex); |
| else | ||
| { | ||
| pthread_mutex_unlock(&xcThreadMutex); |
There was a problem hiding this comment.
After destroying xcThreadMutex at line 1541, the else branch at lines 1552-1555 attempts to unlock it at line 1554. If the code reaches line 1554, it would be unlocking a destroyed mutex, which is undefined behavior. The mutex should be unlocked before being destroyed, or the else branch should not attempt to unlock it.
| 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); |
There was a problem hiding this comment.
The condition checks if filestat.st_size is less than 0, but st_size is of type off_t which on most systems is a signed type. However, a negative file size is not a valid scenario from stat() - it would indicate a serious system error. The more important check is the upper bound. Additionally, the error message at line 475 references "sizeof(data) - 1" but the condition at line 473 checks against "sizeof(data)" which is inconsistent. The error message should match the actual condition being checked.
| 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); | |
| if ((size_t)filestat.st_size >= sizeof(data)) | |
| { | |
| T2Error("File size %ld exceeds buffer capacity %zu for file : %s\n", filestat.st_size, sizeof(data), filePath); |
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| profile->maxlatencyTime.tv_sec += maxuploadinSec; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| profile->maxlatencyTime.tv_sec += maxuploadinSec; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
9056871 to
f9ed2c6
Compare
source/bulkdata/profile.c
Outdated
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); | ||
| pthread_cond_init(&profile->reportcond, NULL); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportcond" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportcond" is written to with "_Profile.reuseThreadMutex" held 5 out of 5 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->currentTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.currentTime" is written to with "_Profile.reuseThreadMutex" held 2 out of 2 times.
Medium Impact, CWE-366
MISSING_LOCK
| if(profile->maxUploadLatency > 0) | ||
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
source/bulkdata/profile.c
Outdated
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); | ||
| pthread_cond_init(&profile->reportcond, NULL); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportcond" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportcond" is written to with "_Profile.reuseThreadMutex" held 5 out of 5 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->currentTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.currentTime" is written to with "_Profile.reuseThreadMutex" held 2 out of 2 times.
Medium Impact, CWE-366
MISSING_LOCK
| if(profile->maxUploadLatency > 0) | ||
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
b1d3007 to
5dd87d7
Compare
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.
|
|
||
| if (!isProcessing) | ||
| { | ||
| free(msgpack->msgpack_blob); |
There was a problem hiding this comment.
The msgpack structure owns the msgpack_blob pointer (which is actually the str parameter passed to this function). When this function returns T2ERROR_SUCCESS at line 335 in the error path, it frees msgpack_blob at line 332. However, the caller of this function (who allocated str) may not expect this pointer to be freed here, potentially leading to a double-free if the caller also tries to free it. The responsibility for freeing str should be clearly defined.
| free(msgpack->msgpack_blob); | |
| /* Do not free msgpack_blob here; ownership of 'str' remains with caller | |
| * since the message is not queued. | |
| */ |
| 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); | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
In T2ER_PushDataWithDelim, when pthread_cond_signal is called at line 146, the erMutex is still held (acquired at line 108, not released until line 168). However, the erCond condition variable is associated with erMutex based on the T2ER_EventDispatchThread waiting pattern. This creates a potential issue where the signal is sent while still holding erMutex, and if the waiting thread wakes up, it needs to reacquire erMutex which is still held by this thread. This differs from T2ER_Push which releases erMutex before signaling. Consider releasing erMutex before acquiring sTDMutex and signaling, similar to the pattern in T2ER_Push.
source/bulkdata/profile.c
Outdated
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); | ||
| pthread_cond_init(&profile->reportcond, NULL); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportcond" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportcond" is written to with "_Profile.reuseThreadMutex" held 5 out of 5 times.
Medium Impact, CWE-366
MISSING_LOCK
| pthread_mutex_init(&profile->reportMutex, NULL); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->currentTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.currentTime" is written to with "_Profile.reuseThreadMutex" held 2 out of 2 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_init(&profile->reportMutex, NULL); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
source/bulkdata/profile.c
Outdated
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); | ||
| pthread_cond_init(&profile->reportcond, NULL); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->reportcond" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.reportcond" is written to with "_Profile.reuseThreadMutex" held 5 out of 5 times.
Medium Impact, CWE-366
MISSING_LOCK
| pthread_mutex_init(&profile->reportMutex, NULL); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); | ||
| memset(&profile->currentTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->currentTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.currentTime" is written to with "_Profile.reuseThreadMutex" held 2 out of 2 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_init(&profile->reportMutex, NULL); | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| memset(&profile->maxlatencyTime, 0, sizeof(struct timespec)); |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
madhubabutt
force-pushed
the
feature/RDK-60291
branch
from
6627b15 to
f4a5889
Compare
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| profile->maxlatencyTime.tv_sec += maxuploadinSec; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| profile->maxlatencyTime.tv_sec += maxuploadinSec; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
| { | ||
| pthread_mutex_lock(&profile->reportMutex); | ||
| T2Info("waiting for %ld sec of macUploadLatency\n", (long) maxuploadinSec); | ||
| profile->maxlatencyTime.tv_sec += maxuploadinSec; |
There was a problem hiding this comment.
Coverity Issue - Data race condition
Accessing "profile->maxlatencyTime" without holding lock "_Profile.reuseThreadMutex". Elsewhere, "_Profile.maxlatencyTime" is written to with "_Profile.reuseThreadMutex" held 1 out of 1 times.
Medium Impact, CWE-366
MISSING_LOCK
No description provided.