RDK-59367: Sync up xdialserver changes from RDKE#170
RDK-59367: Sync up xdialserver changes from RDKE#170yuvaramachandran-gurusamy wants to merge 35 commits into25Q3_sprintfrom
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>
…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>
| name: Build xdialserver component in github rdkcentral | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ubuntu:22.04 | ||
|
|
||
| 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 }} No newline at end of file |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
In general, the problem is fixed by explicitly setting the least-privilege permissions for the GITHUB_TOKEN, either at the workflow root (applies to all jobs) or per job (applies only to that job). For a simple build job that only checks out code and runs shell scripts, contents: read is usually sufficient.
The best fix here, without changing existing functionality, is to add a minimal permissions block to the build-entservices-on-pr job. This ensures the GITHUB_TOKEN (if used implicitly by any actions in the job) has only read access to repo contents. Since the job uses only actions/checkout@v3 and shell commands, contents: read is enough and will not break existing behavior. Concretely, in .github/workflows/native_full_build.yml, under jobs: build-entservices-on-pr:, add:
permissions:
contents: readindented to align with runs-on and container. No additional imports or definitions are required, and no other files need to be changed.
| @@ -10,6 +10,8 @@ | ||
| build-entservices-on-pr: | ||
| name: Build xdialserver component in github rdkcentral | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| container: | ||
| image: ubuntu:22.04 | ||
|
|
There was a problem hiding this comment.
Pull request overview
This PR syncs xdialserver changes from RDKE, adding several significant features and fixes including security token support, manufacturer/model name configuration, UUID-based application routing, and comprehensive build/CI infrastructure.
Changes:
- Added security token authentication support with conditional compilation flag
- Implemented dynamic manufacturer and model name configuration via new callback mechanisms
- Introduced UUID-based application routing and per-boot persistent UUID generation
- Added mutex protection for SSDP operations to prevent race conditions
- Improved error handling, logging consistency, and memory management throughout
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| stubs/securityagent/SecurityTokenUtil.h | New header defining security token retrieval interface |
| stubs/securityagent/SecurityTokenUtil.cpp | Stub implementation for security token (contains critical bugs) |
| stubs/iarm_stubs.cpp | IARM bus stub implementations for testing |
| server/plat/gdialappcache.hpp | Initialize observer pointer to nullptr |
| server/plat/gdialappcache.cpp | Add null pointer checks and fix logging format |
| server/plat/gdial_app_registry.c | Add UUID generation and persistence for app URIs |
| server/plat/gdial.hpp | Add manufacturer/model name callback typedefs |
| server/plat/gdial.cpp | Extensive changes: move semantics, new callbacks, security token integration |
| server/plat/gdial-plat-util.c | Fix format specifier for thread ID logging |
| server/plat/gdial-plat-dev.c | Remove manufacturer/model getter functions |
| server/plat/gdial-plat-app.c | Add manufacturer/model name callback registration |
| server/plat/gdial-os-app.h | Declare new manufacturer/model update functions |
| server/plat/CMakeLists.txt | Add DISABLE_SECURITY_TOKEN build option and uuid library |
| server/include/gdialserviceimpl.h | Add manufacturer/model events, remove onStopped |
| server/include/gdialservicecommon.h | Remove onStopped from interface (breaking change) |
| server/include/gdialservice.h | Add manufacturer/model setter methods |
| server/include/gdial_app_registry.h | Add app_uri field for UUID-based routing |
| server/include/gdial-plat-dev.h | Remove manufacturer/model getter declarations |
| server/include/gdial-plat-app.h | Add manufacturer/model callback typedefs and registration |
| server/include/gdial-config.h | Fix header guard naming convention |
| server/gdialservice.cpp | Implement manufacturer/model handlers, improve initialization |
| server/gdialserver_ut.cpp | Update unit tests for interface changes |
| server/gdial-ssdp.h | Add manufacturer/model setter function declarations |
| server/gdial-ssdp.c | Add mutex protection and manufacturer/model support |
| server/gdial-rest.h | Update additional_data_url signature for UUID support |
| server/gdial-rest.c | Implement UUID-based routing, fix typo, add null checks |
| server/CMakeLists.txt | Add libsoup-3.0 detection and fallback |
| cov_build.sh | New coverage build script |
| build_dependencies.sh | Comprehensive dependency installation script |
| Makefile | Add DISABLE_SECURITY_TOKEN flag support |
| .github/workflows/native_full_build.yml | GitHub Actions workflow for native builds |
| .github/CODEOWNERS | Define code ownership |
💡 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, making the condition always false. Additionally, the function should return the length of the token on success (as documented in the header), not 0. The correct implementation should return len on success.
| size_t len = payload.length(); | ||
|
|
||
| if(!memcpy(buffer,payload.c_str(),len)) | ||
| return -1; |
There was a problem hiding this comment.
Missing buffer size validation. The function should check if maxLength is sufficient to hold the token before calling memcpy to prevent buffer overflow. According to the function contract, if the buffer is too small, it should return a negative value indicating the required length.
| if (nullptr!=appName){ | ||
| applicationName = appName; | ||
| } | ||
| if (nullptr!=argPayload){ | ||
| payLoad = argPayload; | ||
| } | ||
| if (nullptr!=argQueryString){ | ||
| queryString = argQueryString; | ||
| } | ||
| if (nullptr!=argAdditionalDataUrl){ |
There was a problem hiding this comment.
Inconsistent bracing style. The condition blocks starting at lines 239, 243, and 265 use braces, while others don't. For consistency and to prevent future errors when adding code, all if-statement blocks should use braces.
| if (nullptr!=appName){ | |
| applicationName = appName; | |
| } | |
| if (nullptr!=argPayload){ | |
| payLoad = argPayload; | |
| } | |
| if (nullptr!=argQueryString){ | |
| queryString = argQueryString; | |
| } | |
| if (nullptr!=argAdditionalDataUrl){ | |
| if (nullptr!=appName) | |
| { | |
| applicationName = appName; | |
| } | |
| if (nullptr!=argPayload) | |
| { | |
| payLoad = argPayload; | |
| } | |
| if (nullptr!=argQueryString) | |
| { | |
| queryString = argQueryString; | |
| } | |
| if (nullptr!=argAdditionalDataUrl) | |
| { |
|
|
||
| git clone --branch main https://github.com/rdkcentral/entservices-apis.git | ||
|
|
||
| git clone https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git |
There was a problem hiding this comment.
Hardcoded GitHub token usage. The script uses $GITHUB_TOKEN for cloning a private repository at line 41, but there's no validation that this environment variable is set. The script should check if GITHUB_TOKEN is defined before attempting to use it, or provide a clear error message if it's missing.
| git clone https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git | |
| if [ -z "${GITHUB_TOKEN:-}" ]; then | |
| echo "Error: GITHUB_TOKEN environment variable is not set. Cannot clone entservices-testframework." >&2 | |
| exit 1 | |
| fi | |
| git clone https://${GITHUB_TOKEN}@github.com/rdkcentral/entservices-testframework.git |
| fuuid = fopen(app_uuid_file_path, "w"); | ||
| if (fuuid != NULL) | ||
| { | ||
| fprintf(fuuid, "%s", uuid_data); | ||
| fclose(fuuid); | ||
| } |
There was a problem hiding this comment.
The file handle is not closed if the second fopen fails. This creates a file descriptor leak. The error path should also close the file handle opened for reading before returning.
| * | ||
| * 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 function comment incorrectly refers to the parameter as "Id" when the actual parameter name is "buffer". This inconsistency makes the documentation misleading.
| #include <gdial_app_registry.h> | ||
| #include "gdialservicelogging.h" | ||
|
|
||
| #define UUID_FILE_TEMPLATE "/tmp/.dial_%s_uuid.txt" |
There was a problem hiding this comment.
Using a hardcoded path template in /tmp with predictable filenames creates a security risk. An attacker could create symlinks or race conditions (TOCTOU) to manipulate these files. Consider using mkstemp or a more secure temporary file creation mechanism, or store these in a protected directory with appropriate permissions.
| #define UUID_FILE_TEMPLATE "/tmp/.dial_%s_uuid.txt" | |
| #define UUID_FILE_TEMPLATE "/var/lib/gdial/.dial_%s_uuid.txt" |
| 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 typedef at line 55 has an inconsistent name. It's defined as "gdial_modelname_cb" but uses "gdial_manufacturername_cb" as the parameter type. While this may be intentional (same signature), it's confusing. The parameter type should be "gdial_modelname_cb" for clarity.
| void gdial_register_modelname_cb(gdial_manufacturername_cb cb); | |
| void gdial_register_modelname_cb(gdial_modelname_cb cb); |
| 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 initialization error handling returns EXIT_FAILURE which is typically 1, but the function signature returns int and other parts return 0 or -1. For consistency with the rest of the function's error handling pattern, this should return -1 instead of EXIT_FAILURE.
| return EXIT_FAILURE; | |
| return -1; |
| gchar *application_url_str = g_strdup_printf("http://%s:%d/%s/", iface_ipv4_address, GDIAL_REST_HTTP_PORT,app_random_uuid); | ||
|
|
There was a problem hiding this comment.
Missing null check for app_random_uuid. The variable app_random_uuid is used in g_strdup_printf at line 130 without verifying it's not NULL. If this variable is NULL, it could lead to undefined behavior or produce an incorrect URL string.
| gchar *application_url_str = g_strdup_printf("http://%s:%d/%s/", iface_ipv4_address, GDIAL_REST_HTTP_PORT,app_random_uuid); | |
| const gchar *app_uuid = app_random_uuid; | |
| if (app_uuid == NULL) { | |
| GDIAL_LOGERROR("app_random_uuid is NULL, cannot construct Application-URL"); | |
| soup_message_set_status(msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); | |
| pthread_mutex_unlock(&ssdpServerEventSync); | |
| return; | |
| } | |
| gchar *application_url_str = g_strdup_printf("http://%s:%d/%s/", iface_ipv4_address, GDIAL_REST_HTTP_PORT, app_uuid); |
RDK-59367: Sync up xdialserver changes from RDKE
Signed-off-by: yuvaramachandran_gurusamy [yuvaramachandran_gurusamy@comcast.com]