RDK-60291 Telemetry coverity fix #244
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Coverity static analysis findings related to resource leaks, concurrency issues, error handling, and security vulnerabilities in the telemetry codebase.
Changes:
- Improved error handling for system calls (write, stat, fread, popen) and added proper return value checking
- Enhanced thread synchronization by adding mutex protection for shared variables and fixing lock ordering issues
- Fixed integer underflow conditions in queue operations and hash map handling
- Replaced insecure random number generation (rand/srand) with /dev/urandom for better entropy
- Added EINTR handling for pthread_cond_timedwait and pthread_cond_wait calls
- Fixed memory leak patterns in realloc usage
- Added security hardening with umask for temporary file creation
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| source/xconf-client/xconfclient.c | Removed unnecessary NULL assignments, added write() error checking, improved mutex handling and thread synchronization |
| source/utils/t2collection.c | Fixed integer underflow by checking for zero count before subtraction |
| source/utils/persistence.c | Added stat() and fread() error checking with buffer overflow protection |
| source/t2parser/t2parserxconf.c | Initialized mutex and condition variable for reportInProgress flag |
| source/scheduler/scheduler.c | Added EINTR handling loops, fixed mutex handling to prevent use-after-free, removed test functions |
| source/reportgen/reportgen.c | Fixed realloc pattern to use temporary pointers and avoid memory leaks on failure, removed test functions |
| source/protocol/rbusMethod/rbusmethodinterface.c | Added documentation comment explaining mutex synchronization approach |
| source/dcautil/dca.c | Added secure umask (0077) around mkstemp calls for temporary file security |
| source/commonlib/telemetry_busmessage_sender.c | Changed logging to use time() instead of clock_gettime, replaced line counting logic with popen, initialized ret variable, made count global |
| source/ccspinterface/rbusInterface.c | Added NULL check for param before strdup to prevent NULL dereference |
| source/bulkdata/t2eventreceiver.c | Major refactoring of thread synchronization with proper lock ordering, added thread handle caching for safe pthread_join |
| source/bulkdata/reportprofiles.c | Fixed uint32_t to int conversion with underflow protection for hash_map_count |
| source/bulkdata/profilexconf.h | Added mutex and condition variable members for reportInProgress flag |
| source/bulkdata/profilexconf.c | Added mutex protection around reportInProgress flag accesses, added mutex/cond destruction in cleanup |
| source/bulkdata/profile.h | Added restartRequested boolean flag for thread coordination |
| source/bulkdata/profile.c | Replaced rand() with /dev/urandom, added restartRequested flag for proper thread restart signaling, fixed mutex protection patterns |
| source/bulkdata/datamodel.c | Improved thread loop conditions and added proper stopProcessing checks with mutex protection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static bool isRFCT2Enable = false ; | ||
| static bool isT2Ready = false; | ||
| static bool isRbusEnabled = false ; | ||
| static int count = 0; |
There was a problem hiding this comment.
The global static variable count is now being accessed and modified in the cacheEventToFile function which runs in multiple threads (created via pthread_create at line 712). This creates a race condition where multiple threads can read and write to count simultaneously without synchronization. The FileCacheMutex only protects file operations, not the count variable itself. Consider either:
- Making count a local variable inside cacheEventToFile (as it was before), or
- Protecting count access with FileCacheMutex (read it inside the mutex-protected section)
| static int count = 0; | |
| static _Thread_local int count = 0; |
| fs = popen ("cat /tmp/t2_caching_file | wc -l", "r"); | ||
| if(fs != NULL) | ||
| { | ||
| while ((ch = fgetc(fs)) != EOF) | ||
| { | ||
| if (ch == '\n') | ||
| { | ||
| count++; | ||
| } | ||
| } | ||
|
|
||
| //If the file is not empty and does not contain a newline, call it one line | ||
| if (count == 0 && ftell(fs) > 0) | ||
| { | ||
| count++; | ||
| } | ||
| fclose(fs); | ||
| fs = NULL; | ||
| fgets(path, 100, fs); | ||
| count = atoi ( path ); | ||
| pclose(fs); |
There was a problem hiding this comment.
Using popen with shell command introduces security concerns as it spawns a shell process. The command uses a hardcoded path /tmp/t2_caching_file which should match T2_CACHE_FILE constant. However, T2_CACHE_FILE might not be defined as /tmp/t2_caching_file. Additionally, using cat | wc -l is inefficient compared to wc -l < file. More critically, this change converts a local variable to global without thread safety, and popen can fail silently leaving count uninitialized. Consider verifying T2_CACHE_FILE matches this hardcoded path and adding error handling for popen failure.
| fgets(path, 100, fs); | ||
| count = atoi ( path ); | ||
| pclose(fs); | ||
| } |
There was a problem hiding this comment.
The buffer path is declared as char path[100] but there's no bounds checking on the output from fgets. While fgets does limit reading to 100 bytes, the atoi function can silently fail if the string is not a valid number. Additionally, if popen fails (returns NULL), count will retain its previous value from the global static variable, which could be stale or 0. Add error checking: verify fs is not NULL before calling fgets, and consider initializing a local count variable to ensure it doesn't retain stale values.
| } | ||
| 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 filestat.st_size < 0 is checking if st_size is negative, but st_size is of type off_t which is typically a signed type. However, file sizes should never be negative in practice. The check is defensive but the error message on line 475 references "sizeof(data) - 1" which is 19, but the actual check is against sizeof(data) which is 20. The error message should match the actual check: either check against "sizeof(data) - 1" and leave room for null terminator explicitly, or update the error message to use sizeof(data).
| T2Error("File size %ld exceeds buffer capacity %zu for file : %s\n", filestat.st_size, sizeof(data) - 1, filePath); | |
| T2Error("File size %ld exceeds buffer capacity %zu for file : %s\n", filestat.st_size, sizeof(data), filePath); |
| 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); |
There was a problem hiding this comment.
Potential lock ordering issue: This function locks tmpRpMutex first (line 100), then rpMutex (line 101), and later locks rpMutex again while holding tmpRpMutex (line 113). This creates a nested lock acquisition pattern of tmpRpMutex -> rpMutex. If any other code path locks these mutexes in the opposite order (rpMutex -> tmpRpMutex), it could lead to a deadlock. Review all other code paths that acquire both mutexes to ensure consistent lock ordering throughout the codebase.
| snprintf(timeBuffer + strlen(timeBuffer), sizeof(timeBuffer) - strlen(timeBuffer), ".%03ld", msecs); | ||
| time(&rawtime); | ||
| timeinfo = localtime(&rawtime); | ||
| static char timeBuffer[20] = { '\0' }; |
There was a problem hiding this comment.
The timeBuffer variable has been changed from automatic (local) to static. This creates a potential issue: if multiple threads call EVENT_DEBUG concurrently (protected by loggerMutex), the static buffer will be shared and could be corrupted if one thread is still using it after unlocking the mutex. However, since the buffer is only used within the mutex-protected region and the file is written immediately, this should be safe. The more significant issue is that making it static doesn't provide any benefit here - it should remain an automatic variable for better thread safety and stack usage.
| static char timeBuffer[20] = { '\0' }; | |
| char timeBuffer[20] = { '\0' }; |
Coverity Issue - Data race conditionAccessing "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 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
| fs = NULL; | ||
| fgets(path, 100, fs); | ||
| count = atoi ( path ); | ||
| pclose(fs); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "pclose" without checking return value (as is done elsewhere 4 out of 5 times).
Medium Impact, CWE-252
CHECKED_RETURN
No description provided.