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
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
RDKEMW-2033 : Coverity
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
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
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>
| name: Build xdialserver component in github rdkcentral | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: native build | ||
| run: | | ||
| sh -x build_dependencies.sh | ||
| sh -x cov_build.sh | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, add an explicit permissions: block to the affected job, specifying the minimum required permissions. For this build job, only contents: read is necessary to allow code checkout from the repository. Edit .github/workflows/native_full_build.yml, and under the build-entservices-on-pr: job definition (line 12, or following the job name: key), insert:
permissions:
contents: readNo other keys or permission types are needed unless subsequent steps explicitly warrant them. This fix only changes the YAML file, does not affect build or release flow, and adheres to the principle of least privilege.
| @@ -9,6 +9,8 @@ | ||
| jobs: | ||
| build-entservices-on-pr: | ||
| name: Build xdialserver component in github rdkcentral | ||
| permissions: | ||
| contents: read | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces version 1.0.1 of xdialserver with several key improvements including security token handling enhancements, UUID-based app routing, manufacturer/model name configuration support, and various bug fixes.
Key Changes:
- Added optional security token support with
DISABLE_SECURITY_TOKENbuild flag - Implemented per-boot UUID-based application URIs for DIAL apps
- Added manufacturer and model name dynamic configuration APIs
- Fixed memory leaks, improved error handling, and enhanced logging throughout
Reviewed Changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| stubs/securityagent/SecurityTokenUtil.h | New header for security token utility functions |
| stubs/securityagent/SecurityTokenUtil.cpp | New stub implementation for security token retrieval |
| stubs/iarm_stubs.cpp | New IARM bus stub implementations for testing |
| server/plat/gdialappcache.cpp | Updated to use std::move for performance optimization and fixed logging format |
| server/plat/gdial_app_registry.c | Added UUID generation and persistence for DIAL apps |
| server/plat/gdial.hpp | Added manufacturer and model name callback typedefs |
| server/plat/gdial.cpp | Major refactoring: added manufacturer/model name support, fixed security token logic, improved memory management, and enhanced logging |
| server/plat/gdial-plat-util.c | Minor formatting fix |
| server/plat/gdial-plat-dev.c | Removed static manufacturer/model getter functions |
| server/plat/gdial-plat-app.c | Added manufacturer and model name registration callbacks |
| server/plat/gdial-os-app.h | Added manufacturer and model name update function declarations |
| server/plat/CMakeLists.txt | Added DISABLE_SECURITY_TOKEN option and uuid library dependency |
| server/include/gdialserviceimpl.h | Added manufacturer/model name events, removed onStopped method |
| server/include/gdialservicecommon.h | Removed onStopped pure virtual method |
| server/include/gdialservice.h | Added manufacturer and model name setter methods |
| server/include/gdial_app_registry.h | Added app_uri field for UUID-based routing |
| server/include/gdial-plat-dev.h | Removed manufacturer and model getter function declarations |
| server/include/gdial-plat-app.h | Added manufacturer and model name callback typedefs and registration functions |
| server/include/gdial-config.h | Fixed header guard macro name and removed unused enum |
| server/gdialservice.cpp | Implemented manufacturer/model handlers, improved move semantics, fixed type casting issues |
| server/gdialserver_ut.cpp | Updated unit tests to remove onStopped and use move semantics |
| server/gdial-ssdp.h | Added manufacturer and model name setter function declarations |
| server/gdial-ssdp.c | Implemented thread-safe manufacturer/model name updates with mutex protection |
| server/gdial-rest.h | Updated function signatures for UUID-based routing |
| server/gdial-rest.c | Refactored to support UUID-based app routing instead of app-name based routing |
| server/CMakeLists.txt | Added libsoup-3.0 support detection |
| cov_build.sh | New build script for CI/CD coverage builds |
| build_dependencies.sh | New script to build all project dependencies |
| Makefile | Updated to pass DISABLE_SECURITY_TOKEN flag to cmake |
| CHANGELOG.md | Added comprehensive changelog for version 1.0.1 |
| .github/workflows/native_full_build.yml | New GitHub Actions workflow for native builds |
| .github/CODEOWNERS | New CODEOWNERS file for maintainer assignment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.
Inconsistent casing in function name. The parameter type is gdial_manufacturername_cb but the function is registered as gdial_register_modelname_cb. This should be gdial_register_modelname_cb with a parameter type of gdial_modelname_cb or the parameter should match the manufacturer callback type.
| void gdial_register_modelname_cb(gdial_manufacturername_cb cb); | |
| void gdial_register_modelname_cb(gdial_modelname_cb cb); |
| fgets(uuid_data, sizeof(uuid_data), fuuid); | ||
| fclose(fuuid); |
There was a problem hiding this comment.
Missing null-terminator after fgets. The uuid_data buffer is read from file using fgets, but the newline character (if present) is not removed, which could cause issues when using the UUID. Consider using uuid_data[strcspn(uuid_data, "\n")] = 0; to remove the trailing newline.
| if (NULL == controllerRemoteObject) { | ||
| int ret = GetSecurityToken(MAX_LENGTH,buffer); | ||
| if(ret<0) | ||
| 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()); | ||
| } | ||
| } | ||
| #endif | ||
| controllerRemoteObject = new JSONRPC::LinkType<Core::JSON::IElement>(std::string(), false, query); |
There was a problem hiding this comment.
Potential race condition: The code checks if controllerRemoteObject is NULL and then creates it, but this is not thread-safe. If GetCurrentState() is called from multiple threads simultaneously, it could result in multiple allocations and memory leaks. Consider using a mutex or ensuring single-threaded access to this function.
| } | ||
| } | ||
| #endif | ||
| controllerRemoteObject = new JSONRPC::LinkType<Core::JSON::IElement>(std::string(), false, query); |
There was a problem hiding this comment.
Potential memory leak: controllerRemoteObject is allocated with new but is never deleted. The code creates a new JSONRPC::LinkType object each time GetCurrentState() is called (when controllerRemoteObject is NULL), but there's no corresponding cleanup. Consider implementing proper lifecycle management or using smart pointers.
| 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.
Inconsistent function name casing: line 78 uses target_link_Libraries (capital 'L') while line 81 uses target_link_libraries (lowercase 'l'). CMake function names are case-insensitive, but consistency is preferred. Use target_link_libraries (lowercase) for both.
| 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) |
| 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 error message parameter name is incorrect. The log message says "manufacturer[%p]" but the variable being logged is model, not manufacturer.
| GDIAL_LOGERROR("NULL GDialObjHandle[%p] manufacturer[%p]",GDialObjHandle,model); | |
| GDIAL_LOGERROR("NULL GDialObjHandle[%p] model[%p]",GDialObjHandle,model); |
| 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 returns the destination pointer (buffer), which will never be NULL unless the input buffer is NULL. This condition if(!memcpy(buffer,payload.c_str(),len)) will always be false for a valid buffer pointer. Additionally, the function returns 0 on success according to the comment, but returns -1 on the memcpy "failure" and 0 at the end, which contradicts the documented behavior (should return positive length on success).
| * | ||
| * 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 parameter name in the documentation comment is inconsistent with the actual parameter. The comment refers to "Id" but the actual parameter is named "buffer".
| if (NULL == controllerRemoteObject) { | ||
| int ret = GetSecurityToken(MAX_LENGTH,buffer); | ||
| if(ret<0) | ||
| 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()); | ||
| } | ||
| } | ||
| #endif | ||
| controllerRemoteObject = new JSONRPC::LinkType<Core::JSON::IElement>(std::string(), false, query); |
There was a problem hiding this comment.
The removed code checked if(ret<0) to handle failure case and created a controller object without token. The new code creates the controller object after the token check regardless of success/failure. This means if GetSecurityToken returns a negative value (failure), the code will still try to use an uninitialized or empty sToken and query, which could lead to unexpected behavior. The logic should handle the failure case explicitly.
| if (0 != pthread_mutex_init(&ssdpServerEventSync, NULL)) | ||
| { | ||
| GDIAL_LOGERROR("Failed to initializing mutex"); | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
The mutex is initialized in gdial_ssdp_new() (line 177) but if the function returns early due to a failed precondition check (lines 174-175), the mutex will not be initialized. However, gdial_ssdp_destroy() always tries to destroy the mutex (line 318), which could lead to undefined behavior if the mutex was never initialized.
No description provided.