RDK-60308-[tr69hostif, RFC] RDK Coverity Defect Resolution for Device Management#158
RDK-60308-[tr69hostif, RFC] RDK Coverity Defect Resolution for Device Management#158Vismalskumar0 wants to merge 11 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Coverity defects in the Device Management component by fixing memory leaks, adding move semantics optimizations, and improving code quality.
Changes:
- Added proper cleanup of nested pointers in hashData structure to fix memory leaks
- Introduced std::move() optimizations to reduce string copying overhead
- Added default case to switch statement for better code coverage
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rfcMgr/rfc_xconf_handler.cpp
Outdated
| break; | ||
| default | ||
| break; |
There was a problem hiding this comment.
Inconsistent indentation: These lines use tabs while surrounding code uses spaces. Consider maintaining consistent indentation style throughout the file.
| 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; |
There was a problem hiding this comment.
Inconsistent indentation: These lines use tabs while surrounding code uses spaces. Consider maintaining consistent indentation style throughout the file.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@Vismalskumar0 I've opened a new pull request, #159, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rfcMgr/rfc_xconf_handler.cpp
Outdated
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Clearing DB Value: %s\n", __FUNCTION__,__LINE__,ClearDB.c_str()); | ||
| set_RFCProperty(name, std::move(ClearDB), clearValue); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Bootstrap Clearing DB Value: %s\n", __FUNCTION__,__LINE__,BootstrapClearDB.c_str()); | ||
| set_RFCProperty(name, std::move(BootstrapClearDB), clearValue); |
There was a problem hiding this comment.
Use-after-move error: The BootstrapClearDB string is logged at line 1300 using .c_str(), but then immediately moved at line 1301. After std::move(), BootstrapClearDB is in an unspecified state and using it is undefined behavior. The logging should occur after the set_RFCProperty call, or the second parameter should not use std::move() since it's still needed by the logging statement.
| set_RFCProperty(name, std::move(BootstrapClearDB), clearValue); | |
| set_RFCProperty(name, BootstrapClearDB, clearValue); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Bootstrap Clearing DB Value: %s\n", __FUNCTION__,__LINE__,BootstrapClearDB.c_str()); | ||
| set_RFCProperty(name, BootstrapClearDB, clearValue); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] ConfigChangeTime: %s\n", __FUNCTION__,__LINE__,ConfigChangeTime.c_str()); | ||
| set_RFCProperty(name, ConfigChangeTimeKey, ConfigChangeTime); |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"ConfigChangeTime" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""ConfigChangeTime"")" instead of "ConfigChangeTime".
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); |
There was a problem hiding this comment.
Unnecessary use of std::move on local variables that are only used once. The function signature set_RFCProperty(std::string name, std::string key, std::string value) takes all parameters by value, which means they will be copied regardless of std::move. Since ConfigSetHashName and ConfigSetHash_key are local variables that are not used after this call, the compiler's copy elision and move semantics will automatically optimize this. Using std::move here doesn't provide any benefit and makes the code less readable.
| set_RFCProperty(std::move(ConfigSetHashName), std::move(ConfigSetHash_key), configSetHash); | |
| set_RFCProperty(ConfigSetHashName, ConfigSetHash_key, configSetHash); |
| 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); |
There was a problem hiding this comment.
Unnecessary use of std::move on local variables that are only used once. The function signature set_RFCProperty(std::string name, std::string key, std::string value) takes all parameters by value, which means they will be copied regardless of std::move. Since ConfigSetTimeName and ConfigSetTime_Key are local variables that are not used after this call, the compiler's copy elision and move semantics will automatically optimize this. Using std::move here doesn't provide any benefit and makes the code less readable.
| 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; |
There was a problem hiding this comment.
Inconsistent indentation: Lines 1990, 1993, and 2001 use tabs while surrounding code uses spaces. This violates the consistent indentation pattern seen throughout the file. Consider maintaining consistent indentation style (appears to be 4 spaces based on surrounding context).
| touch_file.close(); | ||
|
|
||
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Clearing DB Value: %s\n", __FUNCTION__,__LINE__,ClearDB.c_str()); | ||
| set_RFCProperty(name, ClearDB, clearValue); |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"ClearDB" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""ClearDB"")" instead of "ClearDB".
|
|
||
| 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()); | ||
| set_RFCProperty(name, BootstrapClearDB, clearValue); |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"BootstrapClearDB" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""BootstrapClearDB"")" instead of "BootstrapClearDB".
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Reload Cache Key: %s\n", __FUNCTION__,__LINE__,reloadCacheKey.c_str()); | ||
| set_RFCProperty(name, ClearDBEndKey, clearValue); | ||
| set_RFCProperty(name, BootstrapClearDBEndKey, clearValue); | ||
| set_RFCProperty(name, reloadCacheKey, clearValue); |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"reloadCacheKey" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""reloadCacheKey"")" instead of "reloadCacheKey".
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Clearing DBEnd Key Value: %s\n", __FUNCTION__,__LINE__,ClearDBEndKey.c_str()); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Bootstrap Clearing DBEnd Key Value: %s\n", __FUNCTION__,__LINE__,BootstrapClearDBEndKey.c_str()); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Reload Cache Key: %s\n", __FUNCTION__,__LINE__,reloadCacheKey.c_str()); | ||
| set_RFCProperty(name, ClearDBEndKey, clearValue); |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"ClearDBEndKey" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""ClearDBEndKey"")" instead of "ClearDBEndKey".
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Bootstrap Clearing DBEnd Key Value: %s\n", __FUNCTION__,__LINE__,BootstrapClearDBEndKey.c_str()); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_RFCMGR, "[%s][%d] Reload Cache Key: %s\n", __FUNCTION__,__LINE__,reloadCacheKey.c_str()); | ||
| set_RFCProperty(name, ClearDBEndKey, clearValue); | ||
| set_RFCProperty(name, BootstrapClearDBEndKey, clearValue); |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"BootstrapClearDBEndKey" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""BootstrapClearDBEndKey"")" instead of "BootstrapClearDBEndKey".
|
@Vismalskumar0 -please fix the coverity failures and share the Dev test lgos |
|
@Vismalskumar0 I've opened a new pull request, #160, to work on those changes. Once the pull request is ready, I'll request review from you. |
RDK-60308-[tr69hostif, RFC] RDK Coverity Defect Resolution for Device Management