Skip to content

RDK-60307 [RRD] RDK Coverity Defect Resolution for Device Management #175

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 1 commit into
base: develop
Choose a base branch
from
Open

RDK-60307 [RRD] RDK Coverity Defect Resolution for Device Management #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
135 changes: 116 additions & 19 deletions src/rrdEventProcess.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,31 +164,126 @@ static void processIssueType(data_buf *rbuf)
if (staticprofiledata == NULL)
{
RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: Static Command Info not found for IssueType!!! \n", __FUNCTION__, __LINE__);
// Free dynamicprofiledata since we can't proceed
if (dynamicprofiledata != NULL)
{
if (dynamicprofiledata->rfcvalue != NULL)
{
free(dynamicprofiledata->rfcvalue);
}
if (dynamicprofiledata->command != NULL)
{
free(dynamicprofiledata->command);
}
free(dynamicprofiledata);
}
}
else
{
RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: Read complete for Static Profile: RFCValue: %s, Command: %s, Timeout: %d... \n", __FUNCTION__, __LINE__, staticprofiledata->rfcvalue, staticprofiledata->command, staticprofiledata->timeout);
//Remove the double quotes
size_t staticstrlen = strlen(staticprofiledata->command);
size_t dynamicstrlen = strlen(dynamicprofiledata->command);
if (staticstrlen > 0 && staticprofiledata->command[staticstrlen - 1] == '"') {
staticprofiledata->command[staticstrlen - 1] = '\0';
}
if (dynamicprofiledata->command[0] == '"') {
dynamicprofiledata->command[0] = COMMAND_DELIM;

// Check if commands are NULL before using them
if (dynamicprofiledata->command == NULL || staticprofiledata->command == NULL)
{
RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Command is NULL in dynamic or static profile... \n", __FUNCTION__, __LINE__);
// Free dynamicprofiledata
if (dynamicprofiledata != NULL)
{
if (dynamicprofiledata->rfcvalue != NULL)
{
free(dynamicprofiledata->rfcvalue);
}
if (dynamicprofiledata->command != NULL)
{
free(dynamicprofiledata->command);
}
free(dynamicprofiledata);
}
// Free staticprofiledata
if (staticprofiledata != NULL)
{
if (staticprofiledata->rfcvalue != NULL)
{
free(staticprofiledata->rfcvalue);
}
if (staticprofiledata->command != NULL)
Copy link
Contributor

@rdkcmf-jenkins rdkcmf-jenkins Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Coverity issue no longer present as of: undefined

Show issue

Coverity Issue - Dereference before null check

Null-checking "staticprofiledata->command" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

Medium Impact, CWE-476
REVERSE_INULL

{
free(staticprofiledata->command);
}
free(staticprofiledata);
}
}
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Static Profile Commands: %s, Dynamic Profile Commands: %s\n", __FUNCTION__, __LINE__, staticprofiledata->command, dynamicprofiledata->command);

size_t appendstrlen = ((staticstrlen - 1) + dynamicstrlen + 1);
char *appendcommandstr = (char *)realloc(staticprofiledata->command, appendstrlen);
if (appendcommandstr == NULL) {
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Memory Allocation Failed... \n", __FUNCTION__, __LINE__);
else
{
//Remove the double quotes
size_t staticstrlen = strlen(staticprofiledata->command);
size_t dynamicstrlen = strlen(dynamicprofiledata->command);
if (staticstrlen > 0 && staticprofiledata->command[staticstrlen - 1] == '"') {
staticprofiledata->command[staticstrlen - 1] = '\0';
staticstrlen--; // Update length after removing trailing quote
}
if (dynamicprofiledata->command[0] == '"') {
dynamicprofiledata->command[0] = COMMAND_DELIM;
}
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Static Profile Commands: %s, Dynamic Profile Commands: %s\n", __FUNCTION__, __LINE__, staticprofiledata->command, dynamicprofiledata->command);

size_t appendstrlen = (staticstrlen + dynamicstrlen + 1);
char *appendcommandstr = (char *)realloc(staticprofiledata->command, appendstrlen);
if (appendcommandstr == NULL) {
RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Memory Allocation Failed... \n", __FUNCTION__, __LINE__);
// Free staticprofiledata on realloc failure
if (staticprofiledata != NULL)
{
if (staticprofiledata->rfcvalue != NULL)
{
free(staticprofiledata->rfcvalue);
}
if (staticprofiledata->command != NULL)
{
free(staticprofiledata->command);
}
free(staticprofiledata);
staticprofiledata = NULL; // Set to NULL to prevent double-free
}
// Free dynamicprofiledata on realloc failure
if (dynamicprofiledata != NULL)
{
if (dynamicprofiledata->rfcvalue != NULL)
{
free(dynamicprofiledata->rfcvalue);
}
if (dynamicprofiledata->command != NULL)
{
free(dynamicprofiledata->command);
}
free(dynamicprofiledata);
dynamicprofiledata = NULL; // Set to NULL to prevent double-free
}
}
else
{
strcat(appendcommandstr, dynamicprofiledata->command);
staticprofiledata->command = appendcommandstr;
RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: Updated command after append from dynamic and static profile: %s \n", __FUNCTION__, __LINE__, staticprofiledata->command);
RDK_LOG(RDK_LOG_DEBUG,LOG_REMDEBUG,"[%s:%d]: Executing Commands in Runtime Service... \n",__FUNCTION__,__LINE__);
checkIssueNodeInfo(pIssueNode, NULL, rbuf, false, staticprofiledata);
// NOTE: staticprofiledata is freed by executeCommands() via checkIssueNodeInfo()
// Do NOT free staticprofiledata here to avoid double-free
Comment on lines +270 to +271
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

In the append-mode path, checkIssueNodeInfo(pIssueNode, ...) frees pIssueNode->Node/subNode but pIssueNode itself is never freed in this function, so each message leaks the issueNodeData allocation. Also, the comment that staticprofiledata “is freed by executeCommands()” is only true in non-GTEST builds (the frees are under #if !defined(GTEST_ENABLE) in executeCommands()), which makes the ownership expectation ambiguous and can hide leaks in test builds. Please make ownership explicit and free pIssueNode after checkIssueNodeInfo() returns.

Suggested change
// NOTE: staticprofiledata is freed by executeCommands() via checkIssueNodeInfo()
// Do NOT free staticprofiledata here to avoid double-free
/* In append mode, checkIssueNodeInfo() frees pIssueNode->Node/subNode,
* but not the issueNodeData container itself. Free it here to avoid
* leaking the allocation.
*/
if (pIssueNode != NULL)
{
free(pIssueNode);
pIssueNode = NULL;
}
/* Ownership of staticprofiledata:
* - In non-GTEST builds, staticprofiledata is freed by executeCommands()
* (invoked via checkIssueNodeInfo()).
* - In GTEST builds, executeCommands() does not free it, so clean it up here.
*/
#ifdef GTEST_ENABLE
if (staticprofiledata != NULL)
{
if (staticprofiledata->rfcvalue != NULL)
{
free(staticprofiledata->rfcvalue);
}
if (staticprofiledata->command != NULL)
{
free(staticprofiledata->command);
}
free(staticprofiledata);
staticprofiledata = NULL;
}
#endif

Copilot uses AI. Check for mistakes.
}
// Free dynamicprofiledata after use
if (dynamicprofiledata != NULL)
{
if (dynamicprofiledata->rfcvalue != NULL)
{
free(dynamicprofiledata->rfcvalue);
}
if (dynamicprofiledata->command != NULL)
Copy link
Contributor

@rdkcmf-jenkins rdkcmf-jenkins Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Coverity issue no longer present as of: undefined

Show issue

Coverity Issue - Dereference before null check

Null-checking "dynamicprofiledata->command" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

Medium Impact, CWE-476
REVERSE_INULL

{
free(dynamicprofiledata->command);
}
free(dynamicprofiledata);
}
}
strcat(appendcommandstr, dynamicprofiledata->command);
staticprofiledata->command = appendcommandstr;
RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: Updated command after append from dynamic and static profile: %s \n", __FUNCTION__, __LINE__, staticprofiledata->command);
RDK_LOG(RDK_LOG_DEBUG,LOG_REMDEBUG,"[%s:%d]: Executing Commands in Runtime Service... \n",__FUNCTION__,__LINE__);
checkIssueNodeInfo(pIssueNode, NULL, rbuf, false, staticprofiledata);
}
}
}
Expand Down Expand Up @@ -303,6 +398,8 @@ static void processIssueTypeInStaticProfile(data_buf *rbuf, issueNodeData *pIssu
{ // Static Profile JSON Parsing or Read Fail
RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: Static Profile Parse/Read failed... %s\n", __FUNCTION__, __LINE__, RRD_JSON_FILE);
processIssueTypeInInstalledPackage(rbuf, pIssueNode);
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: ...Exiting...\n", __FUNCTION__, __LINE__);
Copy link
Contributor

@nhanasi nhanasi Feb 9, 2026

Choose a reason for hiding this comment

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

Not needed as we go for dynamic download

return;
}
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Static Profile Parse And Read Success... %s\n", __FUNCTION__, __LINE__, RRD_JSON_FILE);
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Check if Issue in Parsed Static JSON... %s\n", __FUNCTION__, __LINE__, RRD_JSON_FILE);
Expand Down
1 change: 1 addition & 0 deletions src/rrdIarmEvents.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ void _rdmManagerEventHandler(const char *owner, IARM_EventId_t eventId, void *da
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Copying Message Received to the queue.. \n", __FUNCTION__, __LINE__);
RRDMsgDeliver(msqid, sendbuf);
RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: SUCCESS: Message sending Done, ID=%d MSG=%s Size=%d Type=%u AppendMode=%d! \n", __FUNCTION__, __LINE__, msqid, sendbuf->mdata, strlen(sendbuf->mdata), sendbuf->mtype, sendbuf->appendMode);
/* coverity[leaked_storage] */
}
Comment on lines 353 to 356
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

This Coverity suppression appears to be masking that sendbuf (and its mdata/jsonPath) is heap-allocated and queued via RRDMsgDeliver() without a clear/freeing path afterward. Please fix the ownership end-to-end (free the message buffer after it is consumed) rather than suppressing, otherwise the daemon leaks on every completed install event.

Copilot uses AI. Check for mistakes.
else
{
Expand Down
6 changes: 5 additions & 1 deletion src/rrdInterface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ void _rdmDownloadEventHandler(rbusHandle_t handle, rbusEvent_t const* event, rbu
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Copying Message Received to the queue.. \n", __FUNCTION__, __LINE__);
RRDMsgDeliver(msqid, sendbuf);
RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: SUCCESS: Message sending Done, ID=%d MSG=%s Size=%d Type=%u AppendMode=%d! \n", __FUNCTION__, __LINE__, msqid, sendbuf->mdata, strlen(sendbuf->mdata), sendbuf->mtype, sendbuf->appendMode);
/* coverity[leaked_storage] */
remove_item(cache);
}
else
Expand Down Expand Up @@ -338,10 +339,12 @@ void _remoteDebuggerEventHandler(rbusHandle_t handle, rbusEvent_t const* event,
if (dataMsg[0] == '\0' || len <= 0 )
{
RDK_LOG(RDK_LOG_DEBUG,LOG_REMDEBUG,"[%s:%d]: Message Received is empty, Exit Processing!!! \n", __FUNCTION__, __LINE__);
free(dataMsg);
}
else
{
pushIssueTypesToMsgQueue(dataMsg, EVENT_MSG);
/* coverity[leaked_storage] */
}
Comment on lines 345 to 348
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The added Coverity suppression here is masking a real leak/ownership issue: dataMsg is heap-allocated and passed into the queue, but the receiver path does not consistently deallocate the queued data_buf and its mdata. Please fix the ownership end-to-end (free after consumption) instead of suppressing.

Copilot uses AI. Check for mistakes.
Comment on lines 344 to 348
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

In _remoteDebuggerEventHandler, dataMsg is heap-allocated and passed to pushIssueTypesToMsgQueue() with a /* coverity[leaked_storage] */ suppression. This is only safe if the message-queue consumer reliably frees the data_buf (and its mdata) after processing; otherwise this masks a real leak. Please ensure there is a single clear owner for dataMsg/data_buf and that the consumer deallocates it (or copy the string and free here).

Copilot uses AI. Check for mistakes.

RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: ...Exiting...\n", __FUNCTION__, __LINE__);
Expand Down Expand Up @@ -393,6 +396,7 @@ void pushIssueTypesToMsgQueue(char *issueTypeList, message_type_et sndtype)
}
RRDMsgDeliver(msqid, sbuf);
RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: SUCCESS: Message sending Done, ID=%d MSG=%s Size=%d Type=%u AppendMode=%d! \n", __FUNCTION__, __LINE__, msqid, sbuf->mdata, strlen(sbuf->mdata), sbuf->mtype, sbuf->appendMode);
/* coverity[leaked_storage] */
}
Comment on lines 396 to 400
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

pushIssueTypesToMsgQueue() allocates sbuf and enqueues it, but the consumer side currently doesn’t free the data_buf after processing. The Coverity suppression here hides a growing leak in a long-running daemon; ensure the message buffer is freed after it is consumed (and clarify/transfer ownership of mdata/jsonPath safely).

Copilot uses AI. Check for mistakes.
}

Expand All @@ -417,7 +421,7 @@ int RRD_unsubscribe()
RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: SUCCESS: IARM_Bus Unsubscribe done!\n", __FUNCTION__, __LINE__);
#endif
#if !defined(GTEST_ENABLE)
rbusEvent_UnsubscribeEx(rrdRbusHandle, subscriptions, 3);
ret = rbusEvent_UnsubscribeEx(rrdRbusHandle, subscriptions, 3);
if (ret != 0)
{
RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s:%d]: RBUS Unsubscribe EventHandler for RRD failed!!! \n", __FUNCTION__, __LINE__);
Expand Down
42 changes: 40 additions & 2 deletions src/rrdJsonParser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,22 @@ char * readJsonFile(char *jsonfile)
}
fseek(fp, 0, SEEK_SET);
jsonfile_content = (char *) malloc(sizeof(char) * (ch_count + 1));
fread(jsonfile_content, 1, ch_count,fp);
if (jsonfile_content == NULL)
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: Memory allocation failed for json file %s \n",__FUNCTION__,__LINE__,jsonfile);
fclose(fp);
return NULL;
}

size_t bytes_read = fread(jsonfile_content, 1, ch_count, fp);
if (bytes_read != (size_t)ch_count)
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: Failed to read json file %s. Expected %d bytes, read %zu bytes \n",__FUNCTION__,__LINE__,jsonfile,ch_count,bytes_read);
free(jsonfile_content);
fclose(fp);
return NULL;
}

jsonfile_content[ch_count] ='\0';
fclose(fp);

Expand Down Expand Up @@ -312,6 +327,10 @@ issueData * getIssueCommandInfo(issueNodeData *issuestructNode, cJSON *jsoncfg,
tmpCommand = cJSON_Print(elem);
if(tmpCommand)
{
if(issuestdata->command != NULL)
{
free(issuestdata->command); // Free previous command before overwriting
}
issuestdata->command = strdup(tmpCommand); // print command info from json file
cJSON_free(tmpCommand);
}
Expand All @@ -322,6 +341,7 @@ issueData * getIssueCommandInfo(issueNodeData *issuestructNode, cJSON *jsoncfg,
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: No Commands found, exiting.. \n",__FUNCTION__,__LINE__);
free(issuestdata);
return NULL;
}
Comment on lines 341 to 345
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

getIssueCommandInfo() now returns NULL when no command is found or when the sanity check fails. There are callers in rrdEventProcess.c (e.g., the *Profileappend() helpers) that dereference the returned pointer unconditionally for logging; with these NULL returns they will now crash. Please update those call sites to handle a NULL return before dereferencing/logging.

Copilot uses AI. Check for mistakes.
else
{
Expand All @@ -337,6 +357,7 @@ issueData * getIssueCommandInfo(issueNodeData *issuestructNode, cJSON *jsoncfg,
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: Aborting Command execution due to Harmful commands!!!\n",__FUNCTION__,__LINE__);
free(issuestdata->command);
free(issuestdata);
return NULL;
}
else
{
Expand Down Expand Up @@ -415,6 +436,10 @@ bool invokeSanityandCommandExec(issueNodeData *issuestructNode, cJSON *jsoncfg,
tmpCommand = cJSON_Print(elem);
if(tmpCommand)
{
if(issuestdata->command != NULL)
{
free(issuestdata->command); // Free previous command before overwriting
}
issuestdata->command = strdup(tmpCommand); // print command info from json file
cJSON_free(tmpCommand);
}
Expand Down Expand Up @@ -486,6 +511,14 @@ void checkIssueNodeInfo(issueNodeData *issuestructNode, cJSON *jsoncfg, data_buf
struct tm *ltime;
rfcbuf = strdup(buff->mdata);

if (rfcbuf == NULL)
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: Memory allocation failed for rfcbuf\n",__FUNCTION__,__LINE__);
free(buff->mdata); // free rfc data
free(buff->jsonPath); // free rrd path info
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The early-return on rfcbuf == NULL frees buff->mdata / buff->jsonPath but does not free issuestructNode->Node / issuestructNode->subNode (and potentially the issuestructNode container, depending on ownership in the caller). Other exit paths free these members, so this new branch introduces an inconsistent cleanup path and leaks on allocation failure. Please align cleanup here with the rest of checkIssueNodeInfo()’s ownership expectations.

Suggested change
free(buff->jsonPath); // free rrd path info
free(buff->jsonPath); // free rrd path info
if (issuestructNode != NULL)
{
free(issuestructNode->Node);
issuestructNode->Node = NULL;
free(issuestructNode->subNode);
issuestructNode->subNode = NULL;
}

Copilot uses AI. Check for mistakes.
return;
}

// Creating Directory for MainNode under /tmp/rrd Folder
ctime = time (NULL);
ltime = localtime (&ctime);
Expand All @@ -500,6 +533,7 @@ void checkIssueNodeInfo(issueNodeData *issuestructNode, cJSON *jsoncfg, data_buf
if (mkdir(outdir,0777) != 0)
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: %s Directory creation failed!!!\n",__FUNCTION__,__LINE__,outdir);
free(rfcbuf); // free duplicated rfc data
free(buff->mdata); // free rfc data
free(buff->jsonPath); // free rrd path info
return;
Expand Down Expand Up @@ -552,12 +586,16 @@ void checkIssueNodeInfo(issueNodeData *issuestructNode, cJSON *jsoncfg, data_buf
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: RRD Upload Script Execution Success...\n",__FUNCTION__,__LINE__);
}
}
free(rfcbuf); // free duplicated rfc data
free(buff->mdata); // free rfc data
free(buff->jsonPath); // free rrd path info
}
else
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: No Command excuted as RRD Failed to change directory:%s\n",__FUNCTION__,__LINE__,outdir);
free(rfcbuf); // free duplicated rfc data
free(buff->mdata); // free rfc data
free(buff->jsonPath); // free rrd path info
}
}
}
Expand Down Expand Up @@ -634,7 +672,7 @@ bool processAllDebugCommand(cJSON *jsoncfg, issueNodeData *issuestructNode, char
}
}
}
free(rfcbuf); // free rfc value
// Note: rfcbuf is owned by caller and will be freed there
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

processAllDebugCommand no longer frees rfcbuf (to avoid a double-free from checkIssueNodeInfo), but the function’s doc comment still doesn’t state the ownership contract, and existing unit tests pass a strdup’d buffer without freeing it. Please update the function/header documentation to explicitly say the caller owns rfcbuf (and ensure tests/callers free it accordingly).

Suggested change
// Note: rfcbuf is owned by caller and will be freed there
// Note: rfcbuf is owned by the caller; this function must not free it.
// The caller is responsible for freeing rfcbuf after this function returns.

Copilot uses AI. Check for mistakes.
}
else
{
Expand Down
12 changes: 7 additions & 5 deletions src/rrdRunCmdThread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ static cacheData *cacheDataNode = NULL;
void initCache(void)
{
pthread_mutex_init(&rrdCacheMut, NULL);
pthread_mutex_lock(&rrdCacheMut);
cacheDataNode = NULL;
pthread_mutex_unlock(&rrdCacheMut);
Comment on lines +43 to +45
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

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

The mutex lock/unlock around setting cacheDataNode to NULL in initCache() is unnecessary and potentially problematic. This function is called once during initialization before any threads are created (from main), so there's no concurrent access to protect against. Additionally, locking a newly initialized mutex that no other thread can access yet serves no purpose. The assignment to NULL is already atomic and thread-safe for pointer writes.

Suggested change
pthread_mutex_lock(&rrdCacheMut);
cacheDataNode = NULL;
pthread_mutex_unlock(&rrdCacheMut);
cacheDataNode = NULL;

Copilot uses AI. Check for mistakes.
}

/*
Expand Down Expand Up @@ -375,8 +377,9 @@ bool executeCommands(issueData *cmdinfo)
/*Executing Commands using systemd-run*/
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Executing following commands using systemd-run:\n \"%s\"\n",__FUNCTION__,__LINE__,cmdData->command);

strncpy(remoteDebuggerServiceStr, remoteDebuggerPrefix, strlen(remoteDebuggerPrefix) + 1);
strncat(remoteDebuggerServiceStr, cmdData->rfcvalue, strlen(cmdData->rfcvalue));
strncpy(remoteDebuggerServiceStr, remoteDebuggerPrefix, sizeof(remoteDebuggerServiceStr) - 1);
remoteDebuggerServiceStr[sizeof(remoteDebuggerServiceStr) - 1] = '\0';
strncat(remoteDebuggerServiceStr, cmdData->rfcvalue, sizeof(remoteDebuggerServiceStr) - strlen(remoteDebuggerServiceStr) - 1);

removeQuotes(cmdData->command);

Expand All @@ -389,8 +392,8 @@ bool executeCommands(issueData *cmdinfo)
{
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Starting remote_debugger_%s service success...\n",__FUNCTION__,__LINE__,cmdData->rfcvalue);
copyDebugLogDestFile(systemdfp, filePointer);
v_secure_pclose(systemdfp);
}
v_secure_pclose(systemdfp);

/*Logging output using journalctl to Output file*/
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Using journalctl to log command output...\n",__FUNCTION__,__LINE__);
Expand All @@ -403,9 +406,8 @@ bool executeCommands(issueData *cmdinfo)
{
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: journalctl remote_debugger_%s service success...\n",__FUNCTION__,__LINE__,cmdData->rfcvalue);
copyDebugLogDestFile(journalctlfp, filePointer);
v_secure_pclose(journalctlfp);
}

v_secure_pclose(journalctlfp);

/* Close debug_output.txt file*/
fclose(filePointer);
Expand Down
2 changes: 1 addition & 1 deletion src/unittest/rrdUnitTestRunner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3892,7 +3892,7 @@ TEST_F(GetIssueCommandInfoTest, ReturnsValidStruct) {

char buf[] = "rfcvalue123";
issueData* result = getIssueCommandInfo(&node, root, buf);
ASSERT_NE(result, nullptr);
ASSERT_EQ(result, nullptr);

Comment on lines 3893 to 3896
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

ReturnsValidStruct now asserts result == nullptr, but the test JSON includes "kill" and the Sanity.Check.Commands list contains "kill", so getIssueCommandInfo correctly returns NULL (harmful command). Either rename the test to reflect the harmful-command case, or update the JSON to use a non-harmful command and assert result != nullptr. Also ensure root is deleted (and result freed when non-null) to avoid leaking test allocations.

Copilot uses AI. Check for mistakes.
Comment on lines 3893 to 3896
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

This test now expects getIssueCommandInfo() to return NULL, but the name ReturnsValidStruct is misleading and root isn’t cleaned up on this path. Consider renaming the test to reflect the harmful-command case and add cJSON_Delete(root) before returning.

Copilot uses AI. Check for mistakes.
}

Expand Down
Loading