Skip to content

RDK-60291 [telemetry] RDK Coverity Defect Resolution for Device Management #235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
madhubabutt wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

RDK-60291 [telemetry] RDK Coverity Defect Resolution for Device Management #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4774200
RDK-60293 [telemetry] RDK Coverity Defect Resolution for Device Manag…
Jan 2, 2026
cb3a8c8
Merge branch 'develop' into feature/RDK-60291
madhubabutt Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 68 additions & 16 deletions source/bulkdata/datamodel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
* If not stated otherwise in this file or this component's LICENSE file the
* following copyright and licenses apply:

Check failure on line 3 in source/bulkdata/datamodel.c

View workflow job for this annotation

GitHub Actions / call-fossid-workflow / Fossid Annotate PR

FossID License Issue Detected 

Source code with 'Apache-2.0' license found in local file 'source/bulkdata/datamodel.c' (Match: rdk/components/generic/telemetry/rdk/components/generic/telemetry/1, 404 lines, url: https://code.rdkcentral.com/r/plugins/gitiles/rdk/components/generic/telemetry/+archive/RDKB-RELEASE-TEST-DUNFELL-1.tar.gz, file: source/bulkdata/datamodel.c)
*
* Copyright 2019 RDK Management
*
Expand Down Expand Up @@ -50,17 +50,28 @@
{
(void) data;//To fix compiler warning
cJSON *reportProfiles = NULL;
bool shouldContinue = true;

T2Debug("%s ++in\n", __FUNCTION__);

while(!stopProcessing)
while(shouldContinue)
{
pthread_mutex_lock(&rpMutex);
shouldContinue = !stopProcessing;
if(!shouldContinue)
{
pthread_mutex_unlock(&rpMutex);
break;
}
T2Info("%s: Waiting for event from tr-181 \n", __FUNCTION__);
pthread_cond_wait(&rpCond, &rpMutex);
while(t2_queue_count(rpQueue) == 0 && shouldContinue)
{
pthread_cond_wait(&rpCond, &rpMutex);
shouldContinue = !stopProcessing;
}

T2Debug("%s: Received wake up signal \n", __FUNCTION__);
if(t2_queue_count(rpQueue) > 0)
if(t2_queue_count(rpQueue) > 0 && shouldContinue)
{
reportProfiles = (cJSON *)t2_queue_pop(rpQueue);
if (reportProfiles)
Expand All @@ -80,17 +91,32 @@
{
(void) data;//To fix compiler warning
cJSON *tmpReportProfiles = NULL;
bool shouldContinue = true;

T2Debug("%s ++in\n", __FUNCTION__);

while(!stopProcessing)
while(shouldContinue)
{
pthread_mutex_lock(&tmpRpMutex);
pthread_mutex_lock(&rpMutex);
shouldContinue = !stopProcessing;
pthread_mutex_unlock(&rpMutex);
if(!shouldContinue)
Comment on lines 100 to +104
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
{
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);
Comment on lines 109 to +113
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
shouldContinue = !stopProcessing;
pthread_mutex_unlock(&rpMutex);
}
Comment on lines 88 to +116
Copy link

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential deadlock risk: tmpRpMutex is locked at line 100, then rpMutex is locked at line 101 while tmpRpMutex is still held. This creates a specific lock ordering (tmpRpMutex -> rpMutex). However, at line 113 inside the while loop, rpMutex is acquired again while tmpRpMutex is held via pthread_cond_wait. This nested locking pattern could lead to deadlock if other code paths acquire these mutexes in a different order. Ensure all code paths follow consistent lock ordering.

Copilot uses AI. Check for mistakes.

T2Debug("%s: Received wake up signal \n", __FUNCTION__);
if(t2_queue_count(tmpRpQueue) > 0)
if(t2_queue_count(tmpRpQueue) > 0 && shouldContinue)
{
tmpReportProfiles = (cJSON *)t2_queue_pop(tmpRpQueue);
if (tmpReportProfiles)
Expand All @@ -110,11 +136,31 @@
{
(void) data;//To fix compiler warning
struct __msgpack__ *msgpack;
while(!stopProcessing)
bool shouldContinue = true;

while(shouldContinue)
{
// Check stopProcessing first without holding rpMsgMutex to avoid
// nested mutex acquisition which could lead to deadlock if another
// thread acquires these mutexes in opposite order (e.g., rpMutex then rpMsgMutex)
pthread_mutex_lock(&rpMutex);
shouldContinue = !stopProcessing;
pthread_mutex_unlock(&rpMutex);

if(!shouldContinue)
{
break;
}

pthread_mutex_lock(&rpMsgMutex);
pthread_cond_wait(&msg_Cond, &rpMsgMutex);
if(t2_queue_count(rpMsgPkgQueue) > 0)
while(t2_queue_count(rpMsgPkgQueue) == 0 && shouldContinue)
{
pthread_cond_wait(&msg_Cond, &rpMsgMutex);
pthread_mutex_lock(&rpMutex);
shouldContinue = !stopProcessing;
pthread_mutex_unlock(&rpMutex);
Comment on lines +159 to +161
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential deadlock: Same nested locking issue as Comment 3, acquiring rpMutex while holding rpMsgMutex inside the condition wait loop.

Copilot uses AI. Check for mistakes.
}
if(t2_queue_count(rpMsgPkgQueue) > 0 && shouldContinue)
{
msgpack = (struct __msgpack__ *)t2_queue_pop(rpMsgPkgQueue);
if (msgpack)
Expand Down Expand Up @@ -274,18 +320,24 @@

msgpack->msgpack_blob = str;
msgpack->msgpack_blob_size = strSize;
pthread_mutex_lock(&rpMsgMutex);
if (!stopProcessing)
{
t2_queue_push(rpMsgPkgQueue, (void *)msgpack);
pthread_cond_signal(&msg_Cond);
}
else

// Check stopProcessing without holding rpMsgMutex to avoid nested mutex
// acquisition which could lead to deadlock
pthread_mutex_lock(&rpMutex);
bool isProcessing = !stopProcessing;
pthread_mutex_unlock(&rpMutex);

if (!isProcessing)
{
free(msgpack->msgpack_blob);
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
free(msgpack->msgpack_blob);
/* Do not free msgpack_blob here; ownership of 'str' remains with caller
* since the message is not queued.
*/

Copilot uses AI. Check for mistakes.
free(msgpack);
T2Error("Datamodel not initialized, dropping request \n");
return T2ERROR_SUCCESS;
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling at line 343 returns T2ERROR_SUCCESS even though an error condition was detected (datamodel not initialized). This should return T2ERROR_FAILURE or an appropriate error code to indicate the operation failed.

Suggested change
return T2ERROR_SUCCESS;
return T2ERROR_FAILURE;

Copilot uses AI. Check for mistakes.
}

pthread_mutex_lock(&rpMsgMutex);
t2_queue_push(rpMsgPkgQueue, (void *)msgpack);
pthread_cond_signal(&msg_Cond);
pthread_mutex_unlock(&rpMsgMutex);
Comment on lines +326 to 341
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a race condition here. Between unlocking rpMutex at line 336 and locking rpMsgMutex at line 346, another thread could set stopProcessing to false. This means the msgpack data could be pushed to the queue even though the processing threads might be shutting down. Consider holding rpMutex until after pushing to the queue, or use a local copy of rpMsgPkgQueue under rpMutex to ensure atomic checking and queueing.

Copilot uses AI. Check for mistakes.
return T2ERROR_SUCCESS;
}
Expand Down
Loading