RDK-60291 Telemetry coverity fix by madhubabutt · Pull Request #244 · rdkcentral/telemetry

madhubabutt · 2026-01-24T12:54:15Z

No description provided.

Copilot

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.

Copilot · 2026-01-24T13:01:08Z

source/commonlib/telemetry_busmessage_sender.c

 static bool isRFCT2Enable = false ;
 static bool isT2Ready = false;
 static bool isRbusEnabled = false ;
+static int count = 0;


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)

Suggested change

static int count = 0;

static _Thread_local int count = 0;

Copilot · 2026-01-24T13:01:08Z

source/commonlib/telemetry_busmessage_sender.c

+    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);


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.

Copilot · 2026-01-24T13:01:09Z

source/commonlib/telemetry_busmessage_sender.c

+        fgets(path, 100, fs);
+        count = atoi ( path );
+        pclose(fs);
    }


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.

Copilot · 2026-01-24T13:01:09Z

source/utils/persistence.c

+    }
+    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);


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).

Suggested change

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);

Copilot · 2026-01-24T13:01:09Z

source/bulkdata/datamodel.c

        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);


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.

Copilot · 2026-01-24T13:01:09Z

source/commonlib/telemetry_busmessage_sender.c

-        snprintf(timeBuffer + strlen(timeBuffer), sizeof(timeBuffer) - strlen(timeBuffer), ".%03ld", msecs);
+        time(&rawtime);
+        timeinfo = localtime(&rawtime);
+        static char timeBuffer[20] = { '\0' };


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.

Suggested change

static char timeBuffer[20] = { '\0' };

char timeBuffer[20] = { '\0' };

Upvoting this comment

rdkcmf-jenkins · 2026-01-24T13:06:40Z

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

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
source/bulkdata/profile.c:339

rdkcmf-jenkins · 2026-01-24T13:06:40Z

source/commonlib/telemetry_busmessage_sender.c

-        fs = NULL;
+        fgets(path, 100, fs);
+        count = atoi ( path );
+        pclose(fs);


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

shibu-kv · 2026-02-12T22:39:02Z

source/commonlib/telemetry_busmessage_sender.c

-
-    fs = fopen(T2_CACHE_FILE, "r");
-    if (fs != NULL)
+    fs = popen ("cat /tmp/t2_caching_file | wc -l", "r");


The code needs to be rebased. This seems to be bringing back an older version of implementation for caching logic.

shibu-kv · 2026-02-12T22:39:28Z

source/commonlib/telemetry_busmessage_sender.c

-        snprintf(timeBuffer + strlen(timeBuffer), sizeof(timeBuffer) - strlen(timeBuffer), ".%03ld", msecs);
+        time(&rawtime);
+        timeinfo = localtime(&rawtime);
+        static char timeBuffer[20] = { '\0' };


Upvoting this comment

RDK-60291 Telemetry coverity fix

1a8bf25

madhubabutt requested a review from a team as a code owner January 24, 2026 12:54

Copilot AI review requested due to automatic review settings January 24, 2026 12:54

Merge branch 'develop' into test/RDK-60291

207c93e

Copilot started reviewing on behalf of madhubabutt January 24, 2026 12:54 View session

Copilot AI reviewed Jan 24, 2026

View reviewed changes

rdkcmf-jenkins reviewed Jan 24, 2026

View reviewed changes

shibu-kv requested changes Feb 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

RDK-60291 Telemetry coverity fix#244

RDK-60291 Telemetry coverity fix#244
madhubabutt wants to merge 2 commits intodevelopfrom
test/RDK-60291

madhubabutt commented Jan 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

shibu-kv Feb 12, 2026

Uh oh!

rdkcmf-jenkins commented Jan 24, 2026

Uh oh!

rdkcmf-jenkins Jan 24, 2026

Uh oh!

shibu-kv Feb 12, 2026

Uh oh!

shibu-kv Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	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);

	static char timeBuffer[20] = { '\0' };
	char timeBuffer[20] = { '\0' };

Comments

Conversation

madhubabutt commented Jan 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

shibu-kv Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

rdkcmf-jenkins commented Jan 24, 2026

Coverity Issue - Data race condition

Issue location

Uh oh!

rdkcmf-jenkins Jan 24, 2026

Choose a reason for hiding this comment

Coverity Issue - Unchecked return value

Uh oh!

shibu-kv Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

shibu-kv Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants