-
Notifications
You must be signed in to change notification settings - Fork 4
RDKEMW-8587: consume the config variables using dlsym() in MW. #168
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.
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
base: develop
Are you sure you want to change the base?
Conversation
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
ds/audioOutputPortConfig.cpp
Outdated
| printf("%d:%s: kAudioConfigs is defined and loaded kConfigs1 = %p\n", __LINE__, __func__, kConfigs1); | ||
| } | ||
| else { | ||
| INT_ERROR("kAudioConfigs is not defined\r\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why two seprate error line? combined to 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
ds/audioOutputPortConfig.cpp
Outdated
|
|
||
| static dsAudioTypeConfig_t *kConfigs1 = NULL; | ||
| static dsAudioPortConfig_t *kPorts1 = NULL; | ||
| int *pKConSize, *pKPortSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use consistency in names pKConSize -> pKConfigSize as most of the places you have use config. Also there is not significance of 1 as suffix, use better names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code
ds/audioOutputPortConfig.cpp
Outdated
| } | ||
| /* | ||
| * Initialize Audio portTypes (encodings, compressions etc.) | ||
| * and its port instances (db, level etc) |
There was a problem hiding this comment.
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 - Explicit null dereferenced
Dereferencing null pointer "configuration.pKConfigSize".
Medium Impact, CWE-476
FORWARD_NULL
ds/audioOutputPortConfig.cpp
Outdated
|
|
||
| void *dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | ||
| if (dllib) { | ||
| config->pKConfigs = (dsAudioTypeConfig_t *) dlsym(dllib, searchVaribles[0]); |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "&searchVaribles[2]" to format specifier "%s" was expected to have type "char *" but has type "char const **".
Medium Impact, CWE-686
PRINTF_ARGS
santoshcomcast
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed all review comments.
Can you please review?
ds/audioOutputPortConfig.cpp
Outdated
| } | ||
| else | ||
| { | ||
| INT_ERROR("Invalid kAudioConfigs or kAudioPorts, kConfig_size_local, kPort_size_local\n"); |
There was a problem hiding this comment.
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 - Out-of-bounds access
Overrunning array "searchVaribles" of 4 8-byte elements by passing it to a function which accesses it at element index 4 (byte offset 39).
High Impact, CWE-119
OVERRUN
ds/audioOutputPortConfig.cpp
Outdated
| } | ||
| /* | ||
| * Initialize Audio portTypes (encodings, compressions etc.) | ||
| * and its port instances (db, level etc) |
There was a problem hiding this comment.
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 - Explicit null dereferenced
Dereferencing null pointer "configuration.pKConfigSize".
Medium Impact, CWE-476
FORWARD_NULL
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
| #endif | ||
|
|
||
| /* | ||
| * Initialize Video Devices (supported DFCs etc.) |
There was a problem hiding this comment.
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 - Explicit null dereferenced
Dereferencing null pointer "pKVideoDeviceConfigs_size".
Medium Impact, CWE-476
FORWARD_NULL
| } | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "pKVideoDeviceConfigs[i].numSupportedDFCs" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| else | ||
| { | ||
| INT_ERROR("%d:%s: kVideoDeviceConfigs is NULL and videoDeviceConfigs_size is -1\n", __LINE__, __func__); | ||
| } |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "j" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| } | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "i" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| else | ||
| { | ||
| INT_ERROR("%d:%s: kVideoDeviceConfigs is NULL and videoDeviceConfigs_size is -1\n", __LINE__, __func__); | ||
| } |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "i" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
ds/videoOutputPortConfig.cpp
Outdated
| dsVideoResolution_t pixelResolution; ///< The resolution associated with the name | ||
| dsVideoAspectRatio_t aspectRatio; ///< The associated aspect ratio | ||
| dsVideoStereoScopicMode_t stereoScopicMode; ///< The associated stereoscopic mode | ||
| dsVideoFrameRate_t frameRate; ///< The associated frame rate |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "typeCfg->numSupportedResolutions" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
| } | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "pKVideoDeviceConfigs[i].numSupportedDFCs" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| else | ||
| { | ||
| INT_ERROR("%d:%s: kVideoDeviceConfigs is NULL and videoDeviceConfigs_size is -1\n", __LINE__, __func__); | ||
| } |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "j" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| } | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "i" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| else | ||
| { | ||
| INT_ERROR("%d:%s: kVideoDeviceConfigs is NULL and videoDeviceConfigs_size is -1\n", __LINE__, __func__); | ||
| } |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "i" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
ds/videoOutputPortConfig.cpp
Outdated
| dsVideoResolution_t pixelResolution; ///< The resolution associated with the name | ||
| dsVideoAspectRatio_t aspectRatio; ///< The associated aspect ratio | ||
| dsVideoStereoScopicMode_t stereoScopicMode; ///< The associated stereoscopic mode | ||
| dsVideoFrameRate_t frameRate; ///< The associated frame rate |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "typeCfg->numSupportedResolutions" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
ds/frontPanelConfig.cpp
Outdated
| configuration->pKTextDisplays[i].levels, | ||
| configuration->pKTextDisplays[i].maxHorizontalIterations, | ||
| configuration->pKTextDisplays[i].maxVerticalIterations, | ||
| configuration->pKTextDisplays[i].supportedCharacters, |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "configuration->pKTextDisplays[i].supportedCharacters" to format specifier "%d" was expected to have type "int" but has type "char const *".
Medium Impact, CWE-686
PRINTF_ARGS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we fix this issue.
| } | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "pKVideoDeviceConfigs[i].numSupportedDFCs" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| else | ||
| { | ||
| INT_ERROR("%d:%s: kVideoDeviceConfigs is NULL and videoDeviceConfigs_size is -1\n", __LINE__, __func__); | ||
| } |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "j" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| } | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "i" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
| else | ||
| { | ||
| INT_ERROR("%d:%s: kVideoDeviceConfigs is NULL and videoDeviceConfigs_size is -1\n", __LINE__, __func__); | ||
| } |
There was a problem hiding this comment.
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 - Invalid type in argument to printf format specifier
Argument "i" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
ds/videoOutputPortConfig.cpp
Outdated
| dsVideoResolution_t pixelResolution; ///< The resolution associated with the name | ||
| dsVideoAspectRatio_t aspectRatio; ///< The associated aspect ratio | ||
| dsVideoStereoScopicMode_t stereoScopicMode; ///< The associated stereoscopic mode | ||
| dsVideoFrameRate_t frameRate; ///< The associated frame rate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Invalid type in argument to printf format specifier
Argument "typeCfg->numSupportedResolutions" to format specifier "%d" was expected to have type "int" but has type "unsigned long".
Medium Impact, CWE-686
PRINTF_ARGS
ds/audioOutputPortConfig.cpp
Outdated
| return supportedTypes; | ||
| } | ||
|
|
||
| #if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@santoshcomcast , Can we remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| INT_INFO("%d:%s: pKPortSize = %p\n", __LINE__, __func__, config->pKPortSize); | ||
|
|
||
| INT_INFO("\n\n=========================================================================================================================\n\n"); | ||
| if(config->pKConfigs != NULL && *(config->pKConfigSize) != -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add null check for pKConfigs and pKConfigSize since it is pointer variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| { | ||
| INT_ERROR("%d:%s: kAudioConfigs is NULL and kConfig_size_local is -1\n", __LINE__, __func__); | ||
| } | ||
| if(config->pKPorts != NULL && *(config->pKPortSize) != -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add null check for pKPortSize since it is pointer variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| { | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[2]); | ||
| ret = searchConfigs((void **)&configuration.pKConfigSize, (char *)searchVaribles[2]); | ||
| if(ret == false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need fallbacks to default configs if pKConfigSize, pKPorts and pKPortSize are not present in the library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not mix the new and old configs.
ds/audioOutputPortConfig.cpp
Outdated
| const dsAudioPortConfig_t *port = &kPorts[i]; | ||
| * set up ports based on kPorts[] | ||
| */ | ||
| for (size_t i = 0; i < *(configuration.pKPortSize); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add NULL check here as well.
ds/videoOutputPortConfig.cpp
Outdated
| for (size_t i = 0; i < *(config->pKVideoPortConfigs_size); i++) | ||
| { | ||
| const dsVideoPortTypeConfig_t *typeCfg = &(config->pKConfigs[i]); | ||
| #if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this
ds/videoOutputPortConfig.cpp
Outdated
| INT_INFO("%d:%s: typeCfg->hdcpSupported = %d\n", __LINE__, __func__, typeCfg->hdcpSupported); | ||
| INT_INFO("%d:%s: typeCfg->restrictedResollution = %d\n", __LINE__, __func__, typeCfg->restrictedResollution); | ||
| INT_INFO("%d:%s: typeCfg->numSupportedResolutions= %d\n", __LINE__, __func__, typeCfg->numSupportedResolutions); | ||
| #if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this
| { | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[1]); | ||
| ret = searchConfigs((void **)&configuration.pKVideoPortConfigs_size, (char *)searchVaribles[1]); | ||
| if(ret == false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need fallbacks here as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not mix old and new configs.
ds/videoOutputPortConfig.cpp
Outdated
| for (size_t i = 0; i < numResolutions; i++) { | ||
| dsVideoPortResolution_t *resolution = &kResolutions[i]; | ||
| //size_t numResolutions = dsUTL_DIM(kResolutions); | ||
| for (size_t i = 0; i < *(configuration.pKResolutionsSettings_size); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add NULL check here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ds/videoOutputPortConfig.cpp
Outdated
| */ | ||
| for (size_t i = 0; i < dsUTL_DIM(kPorts); i++) { | ||
| const dsVideoPortPortConfig_t *port = &kPorts[i]; | ||
| for (size_t i = 0; i < *(configuration.pKVideoPortPorts_size); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add NULL check here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
apatel859
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces dynamic configuration loading using dlsym() to load device settings configuration variables from a HAL library at runtime, replacing the previous static linking approach. This change adds flexibility by allowing configuration to be sourced from external libraries while maintaining backward compatibility through a fallback mechanism to static configurations.
Key Changes
- Added
searchConfigs()function inmanager.cppthat usesdlopen()/dlsym()to dynamically load configuration symbols from the HAL library - Modified configuration loading in
videoOutputPortConfig.cpp,videoDeviceConfig.cpp,frontPanelConfig.cpp, andaudioOutputPortConfig.cppto attempt dynamic loading first, falling back to static configs if unavailable - Introduced debug dump functions in each config file to log loaded configuration details for troubleshooting
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| ds/manager.cpp | Implements searchConfigs() function for dynamic symbol lookup using dlsym() with mutex protection |
| ds/include/manager.hpp | Adds forward declaration for the searchConfigs() function |
| ds/videoOutputPortConfig.cpp | Updates video port configuration loading to use dynamic lookup with fallback, adds struct and debug dump function |
| ds/videoDeviceConfig.cpp | Updates video device configuration loading to use dynamic lookup with fallback, adds debug functionality |
| ds/frontPanelConfig.cpp | Updates front panel configuration loading to use dynamic lookup with fallback, adds debug dump function |
| ds/audioOutputPortConfig.cpp | Updates audio port configuration loading to use dynamic lookup with fallback, adds debug dump function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ds/audioOutputPortConfig.cpp
Outdated
| if ( configuration.pKConfigs != NULL || configuration.pKPorts != NULL || | ||
| configuration.pKConfigSize != NULL || configuration.pKPortSize != NULL) { |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition uses OR (||) instead of AND (&&). This means the code inside the block will execute even if only ONE pointer is non-NULL, which could lead to null pointer dereferences when accessing the other pointers. All four pointers should be checked with AND logic to ensure they're all valid before use.
| if ( configuration.pKConfigs != NULL || configuration.pKPorts != NULL || | |
| configuration.pKConfigSize != NULL || configuration.pKPortSize != NULL) { | |
| if ( configuration.pKConfigs != NULL && configuration.pKPorts != NULL && | |
| configuration.pKConfigSize != NULL && configuration.pKPortSize != NULL) { |
| #include <string.h> | ||
| using namespace std; | ||
|
|
||
| #define DEBUG 1 // Using for dumpconfig |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DEBUG macro is hardcoded to 1, which means debug code (including dumpconfig() calls) will always execute in production builds. This can impact performance and produce excessive logging.
Consider using a build-time configuration or removing the macro entirely if debug output is always desired, or setting it to 0 by default.
| #define DEBUG 1 // Using for dumpconfig | |
| #ifndef DEBUG | |
| #define DEBUG 0 | |
| #endif // DEBUG |
| int indicatorSize, indicatorColorSize, invalid_size = -1; | ||
| fpdConfigs_t configuration = {0}; | ||
|
|
||
| const char* searchVaribles[] = { |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name searchVaribles is misspelled. It should be searchVariables.
ds/frontPanelConfig.cpp
Outdated
| int indicatorSize, indicatorColorSize, invalid_size = -1; | ||
| fpdConfigs_t configuration = {0}; | ||
|
|
||
| const char* searchVaribles[] = { | ||
| "kFPDIndicatorColors", | ||
| "kFPDIndicatorColors_size", | ||
| "kIndicators", | ||
| "kIndicators_size", | ||
| "kTextDisplays", | ||
| "kTextDisplays_size" | ||
| }; | ||
| bool ret = false; | ||
|
|
||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[0]); | ||
| ret = searchConfigs((void **)&configuration.pKFPDIndicatorColors, searchVaribles[0]); | ||
| if(ret == true) | ||
| { | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[1]); | ||
| ret = searchConfigs((void **)&configuration.pKFPDIndicatorColors_size, (char *)searchVaribles[1]); | ||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[1]); | ||
| configuration.pKFPDIndicatorColors_size = &invalid_size; | ||
| } | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[2]); | ||
| ret = searchConfigs((void **)&configuration.pKIndicators, searchVaribles[2]); | ||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[2]); | ||
| } | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[3]); | ||
| ret = searchConfigs((void **)&configuration.pKIndicators_size, (char *)searchVaribles[3]); | ||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[3]); | ||
| configuration.pKIndicators_size = &invalid_size; | ||
| } | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[4]); | ||
| ret = searchConfigs((void **)&configuration.pKTextDisplays, searchVaribles[4]); | ||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[4]); | ||
| } | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[5]); | ||
| ret = searchConfigs((void **)&configuration.pKTextDisplays_size, (char *)searchVaribles[5]); | ||
| if(ret == false) | ||
| { | ||
| INT_ERROR("%s is not defined\n", searchVaribles[5]); | ||
| configuration.pKTextDisplays_size = &invalid_size; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| INT_ERROR("Read Old Configs\n"); | ||
| configuration.pKFPDIndicatorColors = kIndicatorColors; | ||
| indicatorColorSize = dsUTL_DIM(kIndicatorColors); | ||
| configuration.pKFPDIndicatorColors_size = &indicatorColorSize; | ||
| configuration.pKIndicators = kIndicators; | ||
| indicatorSize = dsUTL_DIM(kIndicators); | ||
| configuration.pKIndicators_size = &indicatorSize; | ||
| configuration.pKTextDisplays = kTextDisplays; | ||
| indicatorSize = dsUTL_DIM(kTextDisplays); | ||
| configuration.pKTextDisplays_size = &indicatorSize; |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local stack variables indicatorSize, indicatorColorSize, and invalid_size may go out of scope while their addresses are still stored in the configuration struct and used later. When the fallback path is taken (lines 463-471) or when invalid_size is assigned (lines 431, 444, 457), these local variables' addresses are stored and later dereferenced throughout the function. This results in undefined behavior.
The variables should be declared as static or the configuration struct should store the values directly instead of pointers to local variables.
|
|
||
| pthread_mutex_lock(&dsLock); | ||
|
|
||
| void *dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | ||
| if (dllib) { | ||
| *pConfigVar = (void *) dlsym(dllib, searchConfigStr); | ||
| if (*pConfigVar != NULL) { | ||
| INT_INFO("%s is defined and loaded pConfigVar= %p\r\n", searchConfigStr, *pConfigVar); | ||
| } | ||
| else { | ||
| INT_ERROR("%d:%s: %s is not defined\n", __LINE__, __func__, searchConfigStr); | ||
| } | ||
|
|
||
| dlclose(dllib); | ||
| } | ||
| else { | ||
| INT_ERROR("%d:%s: Open %s failed\n", __LINE__, __func__, RDK_DSHAL_NAME); | ||
| } |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dlopen() handle is not stored and dlclose() is called immediately after dlsym(). This means the loaded library is unloaded while the returned pointers are still being used throughout the application lifecycle. When dlclose() is called, the memory pointed to by the symbols may become invalid, leading to potential crashes or undefined behavior.
Consider either:
- Keeping the library handle open for the lifetime of the application (store it globally and never close it)
- Using
RTLD_NODELETEflag withdlopen()to prevent unloading
| pthread_mutex_lock(&dsLock); | |
| void *dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | |
| if (dllib) { | |
| *pConfigVar = (void *) dlsym(dllib, searchConfigStr); | |
| if (*pConfigVar != NULL) { | |
| INT_INFO("%s is defined and loaded pConfigVar= %p\r\n", searchConfigStr, *pConfigVar); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: %s is not defined\n", __LINE__, __func__, searchConfigStr); | |
| } | |
| dlclose(dllib); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: Open %s failed\n", __LINE__, __func__, RDK_DSHAL_NAME); | |
| } | |
| static void *dllib = nullptr; | |
| pthread_mutex_lock(&dsLock); | |
| if (!dllib) { | |
| dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY); | |
| if (!dllib) { | |
| INT_ERROR("%d:%s: Open %s failed\n", __LINE__, __func__, RDK_DSHAL_NAME); | |
| pthread_mutex_unlock(&dsLock); | |
| INT_INFO("%d:%s: Exit function\n", __LINE__, __func__); | |
| return false; | |
| } | |
| } | |
| *pConfigVar = (void *) dlsym(dllib, searchConfigStr); | |
| if (*pConfigVar != NULL) { | |
| INT_INFO("%s is defined and loaded pConfigVar= %p\r\n", searchConfigStr, *pConfigVar); | |
| } | |
| else { | |
| INT_ERROR("%d:%s: %s is not defined\n", __LINE__, __func__, searchConfigStr); | |
| } | |
| // Do NOT dlclose(dllib); keep the handle open for the lifetime of the process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check with team
| { | ||
| int configSize, invalid_size = -1; | ||
| videoDeviceConfig_t videoDeviceConfig = {0}; | ||
| const char* searchVaribles[] = { |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name searchVaribles is misspelled. It should be searchVariables.
ds/videoDeviceConfig.cpp
Outdated
|
|
||
|
|
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace at the end of the line should be removed.
| #include "dslogger.h" | ||
| #include "manager.hpp" | ||
|
|
||
| #define DEBUG 1 // Using for dumpconfig |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DEBUG macro is hardcoded to 1, which means debug code will always execute in production builds. This can impact performance and produce excessive logging.
Consider using a build-time configuration or removing the macro entirely if debug output is always desired, or setting it to 0 by default.
| #define DEBUG 1 // Using for dumpconfig | |
| #ifndef DEBUG | |
| #define DEBUG 0 | |
| #endif |
| { | ||
| int configSize, portSize; | ||
| audioConfigs_t configuration = {0}; | ||
| const char* searchVaribles[] = { |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name searchVaribles is misspelled. It should be searchVariables.
| const char* searchVaribles[] = { | |
| const char* searchVariables[] = { |
| configuration.pKConfigs = kConfigs; | ||
| configSize = dsUTL_DIM(kConfigs); | ||
| configuration.pKConfigSize = &configSize; | ||
| configuration.pKPorts = kPorts; |
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local stack variables configSize and portSize may go out of scope while their addresses are still stored in the configuration struct and used later. When the fallback path is taken (lines 226-231), these local variables' addresses are stored and later dereferenced at lines 247 and 268. This results in undefined behavior.
The variables should be declared as static or the configuration struct should store the values directly instead of pointers to local variables.
| configuration.pKConfigs = kConfigs; | |
| configSize = dsUTL_DIM(kConfigs); | |
| configuration.pKConfigSize = &configSize; | |
| configuration.pKPorts = kPorts; | |
| configuration.pKConfigs = kConfigs; | |
| static int configSize; | |
| configSize = dsUTL_DIM(kConfigs); | |
| configuration.pKConfigSize = &configSize; | |
| configuration.pKPorts = kPorts; | |
| static int portSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the static in the start of funcation and using same here.
ds/audioOutputPortConfig.cpp
Outdated
| if(ret == true) | ||
| { | ||
| INT_INFO("%d:%s: Calling searchConfigs( %s)\n", __LINE__, __func__, searchVaribles[2]); | ||
| ret = searchConfigs((void **)&configuration.pKConfigSize, (char *)searchVaribles[2]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When u call searchConfig API first it should have input variable and second out variable. it means earchVaribles[2] should be first argumenet not second
ds/audioOutputPortConfig.cpp
Outdated
| _aPortTypes.at(port->id.type).addPort(_aPorts.at(i)); | ||
| } | ||
| if ( configuration.pKConfigs != NULL || configuration.pKPorts != NULL || | ||
| configuration.pKConfigSize != NULL || configuration.pKPortSize != NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all four should be there so u should use &&
apatel859
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as code is common apply this to all file and will re review next week
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
| /* Initialize a set of supported resolutions | ||
| * | ||
| */ | ||
| size_t numResolutions = dsUTL_DIM(kResolutions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference after null check
Passing "&configuration" to "dumpconfig", which dereferences null "configuration.pKVideoPortConfigs_size".
Medium Impact, CWE-476
FORWARD_NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are validated the all structure pointers then calling dumpconfig().
ds/frontPanelConfig.cpp
Outdated
| * Create TextDisplays | ||
| * 1. Use Supported Colors created for indicators. | ||
| * 2. Create Text Displays. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference before null check
Null-checking "configuration.pKTextDisplays_size" 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| /* Initialize a set of supported resolutions | ||
| * | ||
| */ | ||
| size_t numResolutions = dsUTL_DIM(kResolutions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference after null check
Passing "&configuration" to "dumpconfig", which dereferences null "configuration.pKResolutionsSettings_size".
Medium Impact, CWE-476
FORWARD_NULL
| /* Initialize a set of supported resolutions | ||
| * | ||
| */ | ||
| size_t numResolutions = dsUTL_DIM(kResolutions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference after null check
Passing "&configuration" to "dumpconfig", which dereferences null "configuration.pKVideoPortPorts_size".
Medium Impact, CWE-476
FORWARD_NULL
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
ds/frontPanelConfig.cpp
Outdated
| * Create TextDisplays | ||
| * 1. Use Supported Colors created for indicators. | ||
| * 2. Create Text Displays. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference before null check
Null-checking "configuration.pKTextDisplays_size" 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
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
ds/frontPanelConfig.cpp
Outdated
| * Create TextDisplays | ||
| * 1. Use Supported Colors created for indicators. | ||
| * 2. Create Text Displays. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference before null check
Null-checking "configuration.pKTextDisplays_size" 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
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
| INT_INFO(" Text Display ID: %d, Max Brightness: %d, Max Cycle Rate: %d, Levels: %d, Max Horizontal Iterations: %d, Max Vertical Iterations: %d, Supported Characters: %s, Color Mode: %d\n", | ||
| configuration->pKTextDisplays[i].id, | ||
| configuration->pKTextDisplays[i].maxBrightness, | ||
| configuration->pKTextDisplays[i].maxCycleRate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference before null check
Null-checking "configuration->pKTextDisplays_size" 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
| * Create TextDisplays | ||
| * 1. Use Supported Colors created for indicators. | ||
| * 2. Create Text Displays. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference before null check
Null-checking "configuration.pKTextDisplays_size" 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
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
| configuration->pKTextDisplays[i].id, | ||
| configuration->pKTextDisplays[i].maxBrightness, | ||
| configuration->pKTextDisplays[i].maxCycleRate, | ||
| configuration->pKTextDisplays[i].levels, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity Issue - Dereference after null check
Dereferencing null pointer "configuration->pKTextDisplays_size".
Medium Impact, CWE-476
FORWARD_NULL
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
sync to develop
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587
Risks: High
Signed-off-by:gsanto722 grandhi_santoshkumar@comcast.com