Conversation
RDKECMF-213 Add CODEOWNERS file
Signed-off-by: apatel859 <amit_patel5@comcast.com>
RDKEMW-254: libsoup3 support
DELIA-67407: Code syncup main to develop
Rebase with Develop Branch
…p_set_friendlyname Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
RDKTV-35185: Add sync between ssdp_http_server_callback and gdial_ssdp_set_friendlyname
|
Sanity tests for ['BCM 7216OTT Refboard VA'] added. |
|
Sanity tests for ['BCM 7216OTT Refboard VA'] added. |
|
Sanity tests for ['RPI4 IPSTB'] added. |
|
Sanity tests for ['RPI4 IPSTB'] added. |
Reason for change: Fix issues identified within xcast Test Procedure: Risks: low Priority: P1 Signed-off-by:Hayden Gfeller <Hayden_Gfeller@comcast.com>
…ialserver into feature/RDKEMW-2033
Reason for change: Implemented setManufacturerName and setModelName APIs for DIAL Server name configuration maintained the additional data url to specific app Test Procedure: DIAL should work Risks: None Priority: P1 Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…4_Dial_Args_1 RDK-55044: Implement DIAL requirement to use on EU product
|
Sanity tests for ['BCM 7216OTT Refboard VA'] added. |
|
Sanity tests for ['RPI4 IPSTB'] added. |
RDKEMW-2033 : Coverity
|
Sanity tests for ['RPI4 IPSTB'] added. |
|
Sanity tests for ['BCM 7216OTT Refboard VA'] added. |
Reason for change: Added DISABLE_SECURITY_TOKEN Flag to disable the WPEFrameworkSecurity Token generation changes Test Procedure: please referred from the ticket Risks: Medium Signed-off-by: Thamim Razith <ThamimRazith_AbbasAli@comcast.com>
RDKEMW-2278: Removal of WPEFrameworkSecurity Agent Utility
|
Sanity tests for ['RPI4 IPSTB'] added. |
|
Sanity tests for ['BCM 7216OTT Refboard VA'] added. |
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…129_Coverity RDKEMW-4129: Prepare native build environment
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…129_CoverityTest RDKEMW-4129: Test
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…alserver into topic/RDKEMW-4129_CoverityTest Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…129_CoverityTest RDKEMW-4129: Prepare native build environment for Coverity
Signed-off-by: apatel859 <amit_patel5@comcast.com>
RDKEMW-4129: Prepare native build environment for Coverity
|
Sanity tests for ['RPI4 IPSTB'] added. |
RDKEMW-2854 : Fix the double free issue on call to onApplicationStateChanged api
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
* RDKEMW-6891: Coverity errors fix for xdial * Update gdial.cpp Fixed review comments
* RDKEMW-9964: Removing onStopped GDial notification handling Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com> * RDKEMW-9964: Fixed coverity issues Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com> * RDKEMW-9964: Fixed coverity issues Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com> * RDKEMW-9964: Fixed coverity issues Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com> --------- Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…s issues in xdial (#182) * RDKEMW-11024 - Using copilot identify and fix the static code analysis issues in xdial Reason for Change: Resolving the static code issues scanned by copilot Test Procedure: Compiled and Verified Risks: Low Priority: P1 version: minor Signed-off-by: smohap466 <srinibas_mohapatra@comcast.com> --------- Signed-off-by: smohap466 <srinibas_mohapatra@comcast.com> Co-authored-by: smohap466 <srinibas_mohapatra@comcast.com> Co-authored-by: dkumar798 <dinesh_kumar2@comcast.com>
|
I have read the CLA Document and I hereby sign the CLA 6 out of 7 committers have signed the CLA. |
* RDKEMW-12059: Fix Coverity identified issues
* RDKEMW-12555 : Fix coveirty workflow scan in xdialserver repo Reason for Change: Fix coveirty scan workflow failure in xdialserver repo Test Procedure: Verify coveirty workflow Risks: Low Priority: P1 version: minor Signed-off-by:AkshayKumar_Gampa AkshayKumar_Gampa@comcast.com * RDKEMW-12555 : Fix coveirty workflow scan in xdialserver repo Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request contains a rebase from the develop branch that introduces significant new features and improvements to the xdialserver component, including manufacturer/model name configuration support, UUID-based app routing, security token handling improvements, and thread safety enhancements.
Changes:
- Added manufacturer and model name dynamic configuration support with callback mechanisms
- Implemented UUID-based application routing per boot cycle for improved security
- Enhanced thread safety with mutex protection for SSDP server operations
- Added security token disable option and libsoup-3.0 support in build system
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| stubs/securityagent/SecurityTokenUtil.* | New stub implementation for security token retrieval |
| stubs/iarm_stubs.cpp | New IARM bus stub implementations for testing |
| server/plat/gdialappcache.* | Added null pointer initialization and move semantics improvements |
| server/plat/gdial_app_registry.c | Implemented UUID generation for application registry per boot |
| server/plat/gdial.* | Major refactoring adding manufacturer/model name callbacks and security token handling |
| server/plat/gdial-plat-. | Platform layer updates for new manufacturer/model name features |
| server/plat/gdial-ssdp.c | Added mutex protection and dynamic manufacturer/model name support |
| server/plat/gdial-rest.c | Implemented UUID-based application routing and path parsing changes |
| server/include/*.h | Interface updates for new callback types and function signatures |
| server/gdialservice.* | Service layer implementation of manufacturer/model name configuration |
| server/gdialserver_ut.cpp | Unit test updates for interface changes |
| server/plat/CMakeLists.txt | Added DISABLE_SECURITY_TOKEN build option and libsoup-3.0 support |
| .github/workflows/native_full_build.yml | New CI/CD workflow for native builds |
| build_dependencies.sh | Build script for setting up dependencies |
| cov_build.sh | Coverage build script |
| .github/CODEOWNERS | Code ownership configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(!memcpy(buffer,payload.c_str(),len)) | ||
| return -1; | ||
| return 0; |
There was a problem hiding this comment.
The memcpy return value check is incorrect. memcpy always returns the destination pointer (buffer), which will be non-null if the buffer parameter is valid. This check will always be false. Additionally, the function should return the length of the token on success (the actual length copied), not 0. Returning 0 violates the documented post-condition that "return value 0 should not occur".
| if(ret>0) | ||
| { | ||
| controllerRemoteObject = new JSONRPC::LinkType<Core::JSON::IElement>(std::string()); | ||
| } else { | ||
| string sToken = (char*)buffer; | ||
| string query = "token=" + sToken; | ||
| GDIAL_LOGINFO("Security token = %s ",query.c_str()); | ||
| controllerRemoteObject = new JSONRPC::LinkType<Core::JSON::IElement>(std::string(), false, query); | ||
| sToken = (char*)buffer; | ||
| query = "token=" + sToken; | ||
| GDIAL_LOGINFO("Security token[%s] ",query.c_str()); | ||
| } |
There was a problem hiding this comment.
The security token logic has been inverted incorrectly. The condition now checks if ret > 0 (success) when it should check if ret < 0 (failure) based on the GetSecurityToken API contract. According to the SecurityTokenUtil.h documentation, the return value should be less than 0 on failure and greater than 0 on success. The original check for ret < 0 was correct for detecting failures.
| g_free(remote_address_str); | ||
| GDialRestServer *gdial_rest_server = (GDIAL_REST_SERVER(user_data)); | ||
| gchar **elements = g_strsplit(&path[1], "/", 4); | ||
| gchar **elements = g_strsplit(&path[1], "/", 3); |
There was a problem hiding this comment.
The split count has been changed from 4 to 3, but this may break existing functionality. The code previously handled 4 path components and now only handles 3. This change affects how the path elements are parsed and indexed in the subsequent logic (lines 719-737), where indices have been shifted. This needs careful verification that all path parsing scenarios are still handled correctly.
| gchar **elements = g_strsplit(&path[1], "/", 3); | |
| gchar **elements = g_strsplit(&path[1], "/", 0); |
| } | ||
| else { | ||
| g_object_unref(app); | ||
| // FIX(Copilot): Set app to NULL after unref to prevent use-after-free |
There was a problem hiding this comment.
The comment "FIX(Copilot): Set app to NULL after unref to prevent use-after-free" should be removed before merging. Such development-process comments don't belong in production code. The actual code change (setting app = NULL) is good practice, but the comment format is inappropriate.
| // FIX(Copilot): Set app to NULL after unref to prevent use-after-free |
| g_printerr("Warn: app_name too long\r\n"); | ||
| if (j == 0) { | ||
| ret = g_strlcpy(base, elements[i], sizeof(base)); | ||
| if (ret >= sizeof(base)) { |
There was a problem hiding this comment.
Case-inconsistent indentation with tab character. Line 721 uses a tab character while surrounding code uses spaces, creating inconsistent formatting.
| if (ret >= sizeof(base)) { | |
| if (ret >= sizeof(base)) { |
| add_library(gdial-plat SHARED ${GDIAL_PLAT_LIB_SOURCE_FILES}) | ||
| target_link_Libraries(gdial-plat PRIVATE ${GLIB_LIBRARIES} ${GOBJECT_LIBRARIES} -lpthread -lWPEFrameworkCore -lWPEFrameworkDefinitions -lWPEFrameworkCOM -lWPEFrameworkPlugins -lWPEFrameworkSecurityUtil -lIARMBus) | ||
| if(DISABLE_SECURITY_TOKEN) | ||
| target_link_Libraries(gdial-plat PRIVATE ${GLIB_LIBRARIES} ${GOBJECT_LIBRARIES} -lpthread -lWPEFrameworkCore -lWPEFrameworkDefinitions -lWPEFrameworkCOM -lWPEFrameworkPlugins -lIARMBus -luuid) |
There was a problem hiding this comment.
Incorrect case sensitivity in CMake function names. Line 78 uses 'target_link_Libraries' (capital L) while line 81 uses 'target_link_libraries' (lowercase l). CMake function names are case-insensitive, but this inconsistency indicates a typo. For consistency with CMake conventions and the rest of the file, both should use lowercase 'target_link_libraries'.
| target_link_Libraries(gdial-plat PRIVATE ${GLIB_LIBRARIES} ${GOBJECT_LIBRARIES} -lpthread -lWPEFrameworkCore -lWPEFrameworkDefinitions -lWPEFrameworkCOM -lWPEFrameworkPlugins -lIARMBus -luuid) | |
| target_link_libraries(gdial-plat PRIVATE ${GLIB_LIBRARIES} ${GOBJECT_LIBRARIES} -lpthread -lWPEFrameworkCore -lWPEFrameworkDefinitions -lWPEFrameworkCOM -lWPEFrameworkPlugins -lIARMBus -luuid) |
| * | ||
| * Parameters | ||
| * maxLength - holds the maximum uint8_t length of the buffer | ||
| * Id - Buffer to hold the token. |
There was a problem hiding this comment.
The documentation comment for the 'buffer' parameter is inconsistent between the header and implementation files. The header file states "Buffer to hold the token" while the implementation incorrectly states "Id - Buffer to hold the token". The parameter name should be 'buffer', not 'Id'.
| GDIAL_LOGTRACE("Entering ..."); | ||
| if ((nullptr == GDialObjHandle)||(nullptr == model)) | ||
| { | ||
| GDIAL_LOGERROR("NULL GDialObjHandle[%p] manufacturer[%p]",GDialObjHandle,model); |
There was a problem hiding this comment.
The parameter name in the error log message is incorrect. The function parameter is named 'model' but the error message refers to it as 'manufacturer'. This should be 'model' to match the parameter name and avoid confusion during debugging.
| GDIAL_LOGERROR("NULL GDialObjHandle[%p] manufacturer[%p]",GDialObjHandle,model); | |
| GDIAL_LOGERROR("NULL GDialObjHandle[%p] model[%p]", GDialObjHandle, model); |
| void gdial_register_friendlyname_cb(gdial_friendlyname_cb cb); | ||
| void gdial_register_registerapps_cb(gdial_registerapps_cb cb); | ||
| void gdial_register_manufacturername_cb(gdial_manufacturername_cb cb); | ||
| void gdial_register_modelname_cb(gdial_manufacturername_cb cb); |
There was a problem hiding this comment.
The callback typedef 'gdial_modelname_cb' is defined to use 'gdial_manufacturername_cb' type instead of its own distinct type. While they may have the same signature, this creates confusion and should be properly defined as its own typedef for clarity and future maintainability.
| void gdial_register_modelname_cb(gdial_manufacturername_cb cb); | |
| void gdial_register_modelname_cb(gdial_modelname_cb cb); |
| else | ||
| { | ||
| GDIAL_LOGERROR("Failed to allocate memory for response_headers"); | ||
| soup_message_set_status(msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
Potential deadlock in error path. If application_url_str allocation fails (line 146), the function sets an error status but the mutex acquired at line 93 is not released before returning. This will cause a deadlock on the next call to this function.
| soup_message_set_status(msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); | |
| soup_message_set_status(msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); | |
| pthread_mutex_unlock(&ssdpServerEventSync); | |
| return; |
No description provided.