Skip to content

RDK-60308-[tr69hostif, RFC] RDK Coverity Defect Resolution for Device Management #143

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
Vismalskumar0 wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

RDK-60308-[tr69hostif, RFC] RDK Coverity Defect Resolution for Device Management #143

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a33fd4b
Merge pull request #142 from rdkcentral/develop
Vismalskumar0 Jan 9, 2026
901bc15
Fix multiple code quality issues in RFC xconf handler
Vismalskumar0 Jan 9, 2026
2baaf23
Update rfc_xconf_handler.h
Vismalskumar0 Jan 9, 2026
e2360e6
Update xconf_handler.h
Vismalskumar0 Jan 9, 2026
e03568e
Update rfc_xconf_handler.h
Vismalskumar0 Jan 9, 2026
7bf89e2
Update xconf_handler.h
Vismalskumar0 Jan 9, 2026
ad72760
Merge pull request #144 from rdkcentral/develop
Vismalskumar0 Jan 12, 2026
aa456bd
Merge pull request #151 from rdkcentral/develop
Vismalskumar0 Jan 14, 2026
7206ff9
Fix Coverity USE_AFTER_MOVE defects in rfc_xconf_handler.cpp
Vismalskumar0 Jan 14, 2026
83950ae
Remove unnecessary whitespace in xconf_handler.h
Vismalskumar0 Jan 14, 2026
cb05e91
Update rfcMgr/rfc_xconf_handler.cpp
Vismalskumar0 Jan 22, 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
60 changes: 36 additions & 24 deletions rfcMgr/rfc_xconf_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1295,12 +1295,12 @@ void RuntimeFeatureControlProcessor::clearDB(void)
std::ofstream touch_file(TR181STOREFILE);
touch_file.close();

set_RFCProperty(name, ClearDB, clearValue);
set_RFCProperty(name, BootstrapClearDB, clearValue);
set_RFCProperty(name, ConfigChangeTimeKey, ConfigChangeTime);
set_RFCProperty(name, std::move(ClearDB), clearValue);
set_RFCProperty(name, std::move(BootstrapClearDB), clearValue);
set_RFCProperty(name, std::move(ConfigChangeTimeKey), std::move(ConfigChangeTime));

RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Clearing DB Value: %s\n", __FUNCTION__,__LINE__,ClearDB.c_str());
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Bootstrap Clearing DB Value: %s\n", __FUNCTION__,__LINE__,BootstrapClearDB.c_str());
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Clearing DB Value\n", __FUNCTION__,__LINE__);
Copy link
Contributor

@nhanasi nhanasi Jan 9, 2026

Choose a reason for hiding this comment

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

why we removing the log?

RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Bootstrap Clearing DB Value\n", __FUNCTION__,__LINE__);
Comment on lines +1302 to +1303
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The logging statements were updated to remove the string values because the variables were moved on lines 1298-1299. However, these log statements are now less informative. Consider either: 1) keeping the original code without std::move() for these variables since the performance gain is minimal for short-lived local variables, or 2) logging the values before the move operations.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@Vismalskumar0 Vismalskumar0 Jan 22, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link
Contributor Author

@Vismalskumar0 Vismalskumar0 Jan 22, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] ConfigChangeTime: %s\n", __FUNCTION__,__LINE__,ConfigChangeTime.c_str());
Copy link
Contributor

@rdkcmf-jenkins rdkcmf-jenkins Jan 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 - Using a moved object

"ConfigChangeTime" is used after it has been already moved.

High Impact, CWE-457
USE_AFTER_MOVE

Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

Use-after-move bug: The variable ConfigChangeTime is moved on line 1300, but then used again in the logging statement on line 1304 via .c_str(). After a string is moved, its contents are in an undefined state and accessing it leads to undefined behavior. Either remove std::move() from this variable or remove the logging statement that uses it.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@Vismalskumar0 Vismalskumar0 Jan 22, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback


#else
Expand Down Expand Up @@ -1342,7 +1342,7 @@ void RuntimeFeatureControlProcessor::rfcStashRetrieveParams(void)

std::string name = "rfc";

WDMP_STATUS status = set_RFCProperty(name, RFC_ACCOUNT_ID_KEY_STR, stashAccountId);
WDMP_STATUS status = set_RFCProperty(name, RFC_ACCOUNT_ID_KEY_STR, std::move(stashAccountId));
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.

After moving stashAccountId into the set_RFCProperty function at line 1345, the variable is in a valid but unspecified state. The assignment at line 1356 (_accountId = stashAccountId) will assign this unspecified value to _accountId, which is incorrect. The std::move should be removed from line 1345, or the assignment at line 1356 should use the value before it was moved.

Suggested change
WDMP_STATUS status = set_RFCProperty(name, RFC_ACCOUNT_ID_KEY_STR, std::move(stashAccountId));
WDMP_STATUS status = set_RFCProperty(name, RFC_ACCOUNT_ID_KEY_STR, stashAccountId);

Copilot uses AI. Check for mistakes.
if (status != WDMP_SUCCESS)
{
#if !defined(RDKB_SUPPORT)
Expand Down Expand Up @@ -1371,9 +1371,9 @@ void RuntimeFeatureControlProcessor::clearDBEnd(void){
std::string BootstrapClearDBEndKey = "Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Bootstrap.Control.ClearDBEnd";
std::string reloadCacheKey = "RFC_CONTROL_RELOADCACHE";

set_RFCProperty(name, ClearDBEndKey, clearValue);
set_RFCProperty(name, BootstrapClearDBEndKey, clearValue);
set_RFCProperty(name, reloadCacheKey, clearValue);
set_RFCProperty(name, std::move(ClearDBEndKey), clearValue);
set_RFCProperty(name, std::move(BootstrapClearDBEndKey), clearValue);
set_RFCProperty(name, std::move(reloadCacheKey), clearValue);

RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Clearing DBEnd Key Value: %s\n", __FUNCTION__,__LINE__,ClearDBEndKey.c_str());
Copy link
Contributor

@rdkcmf-jenkins rdkcmf-jenkins Jan 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 - Using a moved object

"ClearDBEndKey" is used after it has been already moved.

High Impact, CWE-457
USE_AFTER_MOVE

RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Bootstrap Clearing DBEnd Key Value: %s\n", __FUNCTION__,__LINE__,BootstrapClearDBEndKey.c_str());
Copy link
Contributor

@rdkcmf-jenkins rdkcmf-jenkins Jan 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 - Using a moved object

"BootstrapClearDBEndKey" is used after it has been already moved.

High Impact, CWE-457
USE_AFTER_MOVE

Expand All @@ -1387,7 +1387,7 @@ void RuntimeFeatureControlProcessor::updateHashInDB(std::string configSetHash)
#if !defined(RDKB_SUPPORT)
std::string ConfigSetHashName = "ConfigSetHash";
std::string ConfigSetHash_key = "Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Control.ConfigSetHash";
set_RFCProperty(ConfigSetHashName, ConfigSetHash_key, configSetHash);
set_RFCProperty(std::move(ConfigSetHashName), std::move(ConfigSetHash_key), configSetHash);
#else
const std::string RFC_RAM_PATH = "/tmp/RFC";
std::string filePath = RFC_RAM_PATH + "/.hashValue";
Expand Down Expand Up @@ -1415,7 +1415,7 @@ void RuntimeFeatureControlProcessor::updateTimeInDB(std::string timestampString)
#if !defined(RDKB_SUPPORT)
std::string ConfigSetTimeName = "ConfigSetTime";
std::string ConfigSetTime_Key = "Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Control.ConfigSetTime";
set_RFCProperty(ConfigSetTimeName, ConfigSetTime_Key, timestampString);
set_RFCProperty(std::move(ConfigSetTimeName), std::move(ConfigSetTime_Key), timestampString);
#else
const std::string RFC_RAM_PATH = "/tmp/RFC";
std::string filePath = RFC_RAM_PATH + "/.timeValue";
Expand Down Expand Up @@ -1466,15 +1466,15 @@ void RuntimeFeatureControlProcessor::updateHashAndTimeInDB(char *curlHeaderResp)
// Output the value
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "configSetHash value: %s\n", configSetHashValue.c_str());

updateHashInDB(configSetHashValue);
updateHashInDB(std::move(configSetHashValue));
} else {
RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR, "configSetHash not found in httpHeader!");
}


std::time_t timestamp = std::time(nullptr); // Get current timestamp
std::string timestampString = std::to_string(timestamp);
updateTimeInDB(timestampString);
updateTimeInDB(std::move(timestampString));

std::fstream fs;
fs.open(RFC_SYNC_DONE, std::ios::out);
Expand Down Expand Up @@ -1507,10 +1507,8 @@ bool RuntimeFeatureControlProcessor::IsDirectBlocked()
else
{
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR,"[%s][%d] RFC: Last direct failed blocking has expired, removing %s, allowing direct \n", __FUNCTION__, __LINE__, DIRECT_BLOCK_FILENAME);
if (remove(DIRECT_BLOCK_FILENAME) != 0)
{
RDK_LOG(RDK_LOG_ERROR, LOG_RFCMGR,"[%s][%d]Failed to remove file %s\n", __FUNCTION__, __LINE__, DIRECT_BLOCK_FILENAME);
}
// Attempt to remove the file - if it fails (e.g., already removed), it's not critical
(void)remove(DIRECT_BLOCK_FILENAME);
Comment on lines +1510 to +1511
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The TOCTOU fix is correct in removing the race condition between stat() and remove(). However, silently ignoring all remove() failures (via void cast) may hide legitimate errors like permission issues. Consider logging at DEBUG level if remove() fails, checking errno to distinguish between "file not found" (expected race condition) and other errors like EACCES (permission denied).

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@Vismalskumar0 Vismalskumar0 Jan 22, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

}
}
#endif
Expand Down Expand Up @@ -1988,7 +1986,18 @@ int RuntimeFeatureControlProcessor::DownloadRuntimeFeatutres(DownloadData *pDwnL
}
if(file_dwnl.hashData != nullptr)
{
if(file_dwnl.hashData->hashvalue != nullptr)
{
free(file_dwnl.hashData->hashvalue);
file_dwnl.hashData->hashvalue = nullptr;
}
if(file_dwnl.hashData->hashtime != nullptr)
{
free(file_dwnl.hashData->hashtime);
file_dwnl.hashData->hashtime = nullptr;
}
free(file_dwnl.hashData);
file_dwnl.hashData = nullptr;
}

if (_url_validation_in_progress)
Expand Down Expand Up @@ -2021,7 +2030,10 @@ int RuntimeFeatureControlProcessor::DownloadRuntimeFeatutres(DownloadData *pDwnL
case 83:
case 90:
case 91:
NotifyTelemetry2ErrorCode(curl_ret_code);
NotifyTelemetry2ErrorCode(curl_ret_code);
break;
default:
break;
}

if((curl_ret_code == 0) && (httpCode == 404))
Expand Down Expand Up @@ -2133,7 +2145,7 @@ void RuntimeFeatureControlProcessor::GetValidAccountId()
}
else
{
_valid_accountId = value;
_valid_accountId = std::move(value);
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.

Using std::move on a local variable (value) to assign to a member variable (_valid_accountId) prevents copy elision and doesn't provide any benefit here since the local variable is going out of scope anyway. Additionally, if value is moved and later accessed in the logging or comparisons that follow, it would be in an undefined state. In this case, the move appears safe since value is not used after the assignment, but it's unnecessary and could be confusing. Consider removing std::move and letting the compiler optimize the assignment.

Suggested change
_valid_accountId = std::move(value);
_valid_accountId = value;

Copilot uses AI. Check for mistakes.
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR,"[%s][%d] NEW valid Account ID: %s\n", __FUNCTION__, __LINE__, _valid_accountId.c_str());

std::string unknown_str = "Unknown";
Expand Down Expand Up @@ -2185,7 +2197,7 @@ void RuntimeFeatureControlProcessor::GetValidPartnerId()
}
else
{
_valid_partnerId = value;
_valid_partnerId = std::move(value);
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.

Using std::move on a local variable (value) to assign to a member variable (_valid_partnerId) prevents copy elision and doesn't provide any benefit here since the local variable is going out of scope anyway. Additionally, if value is moved and later accessed in logging or comparisons that follow, it would be in an undefined state. In this case, the move appears safe since value is not used after the assignment, but it's unnecessary and could be confusing. Consider removing std::move and letting the compiler optimize the assignment.

Suggested change
_valid_partnerId = std::move(value);
_valid_partnerId = value;

Copilot uses AI. Check for mistakes.
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR,"[%s][%d] NEW valid Partner ID: %s\n", __FUNCTION__, __LINE__, _valid_partnerId.c_str());

std::string unknown_str = "Unknown";
Expand Down Expand Up @@ -2230,7 +2242,7 @@ void RuntimeFeatureControlProcessor::GetXconfSelect()
if(!XconfUrl.empty())
{
_xconf_server_url.clear();
_xconf_server_url = XconfUrl;
_xconf_server_url = std::move(XconfUrl);
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.

Using std::move on a local variable (XconfUrl) to assign to a member variable (_xconf_server_url) prevents copy elision and doesn't provide any benefit here since the local variable is going out of scope anyway. Consider removing std::move and letting the compiler optimize the assignment.

Suggested change
_xconf_server_url = std::move(XconfUrl);
_xconf_server_url = XconfUrl;

Copilot uses AI. Check for mistakes.
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR,"[%s][%d] NEW Xconf URL configured=%s\n", __FUNCTION__, __LINE__, _xconf_server_url.c_str());
}

Expand Down Expand Up @@ -2413,7 +2425,7 @@ void RuntimeFeatureControlProcessor::processXconfResponseConfigDataPart(JSON *fe
}
else
{
if (newValue.empty())
if (currentValue.empty())
Copy link
Contributor

@nhanasi nhanasi Jan 9, 2026

Choose a reason for hiding this comment

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

rebase the code

{
RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] EMPTY value for %s is rejected\n", __FUNCTION__, __LINE__, newKey.c_str());
continue;
Expand Down Expand Up @@ -2478,7 +2490,7 @@ void RuntimeFeatureControlProcessor::processXconfResponseConfigDataPart(JSON *fe
}
std::string data = "TR181: " + newKey + " " + newValue;

paramList.push_back(data);
paramList.push_back(std::move(data));
}

updateTR181File(TR181_FILE_LIST, paramList);
Expand Down Expand Up @@ -2883,7 +2895,7 @@ int RuntimeFeatureControlProcessor::ProcessXconfUrl(const char *XconfUrl)
std::string FQDN = _xconf_server_url;
_xconf_server_url = std::string(XconfUrl) + "/featureControl/getSettings";
std::stringstream url = CreateXconfHTTPUrl();
_xconf_server_url = FQDN;
_xconf_server_url = std::move(FQDN);
Copy link
Contributor

@nhanasi nhanasi Jan 9, 2026

Choose a reason for hiding this comment

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

why stb:move needed only for few params?


DownloadData DwnLoc, HeaderDwnLoc;
InitDownloadData(&DwnLoc);
Expand Down
1 change: 1 addition & 0 deletions rfcMgr/rfc_xconf_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

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.

An unnecessary blank line was added at the beginning of the file. This appears to be unintentional and should be removed to maintain code consistency.

Suggested change

Copilot uses AI. Check for mistakes.
/**
* If not stated otherwise in this file or this component's LICENSE
* file the following copyright and licenses apply:
Expand Down
2 changes: 1 addition & 1 deletion rfcMgr/xconf_handler.h
View file
Open in desktop
Copy link
Contributor

@nhanasi nhanasi Jan 9, 2026

Choose a reason for hiding this comment

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

why this change needed?

Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ extern "C" {
namespace xconf {
class XconfHandler {
public :
XconfHandler(){ }
XconfHandler() : _ebuild_type(RDKB_DEV) { }
Copy link
Contributor

@nhanasi nhanasi Jan 9, 2026

Choose a reason for hiding this comment

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

this is not needed

int initializeXconfHandler(void);

// We do not allow this class to be copied !!
Expand Down
Loading