Skip to content

RDKEMW-6898: GSSDP upgradtion to latest 1.6.3 version#196

Open
balav08 wants to merge 45 commits intomainfrom
feature/RDKEMW-6898_apr2
Open

RDKEMW-6898: GSSDP upgradtion to latest 1.6.3 version#196
balav08 wants to merge 45 commits intomainfrom
feature/RDKEMW-6898_apr2

Conversation

@balav08
Copy link
Copy Markdown

@balav08 balav08 commented Apr 2, 2026

No description provided.

scthunderbolt and others added 30 commits December 17, 2024 15:48
Signed-off-by: apatel859 <amit_patel5@comcast.com>
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>
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
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>
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>
mukesh972 and others added 11 commits September 3, 2025 16:04
* 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>
* 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>
* RDKEMW-12059: Fix Coverity identified issues

* Update gdial-plat-util.c

* Update gdial-rest.c

* Update gdial-plat-util.c

* Update gdial-plat-util.c
@balav08 balav08 requested a review from a team as a code owner April 2, 2026 10:18
Copilot AI review requested due to automatic review settings April 2, 2026 10:18
Comment on lines +11 to +25
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

Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}

Copilot Autofix

AI 14 days ago

In general, fix this by explicitly defining a permissions: block that grants only the minimal rights required. Since this workflow just checks out the code and runs build scripts, the safe default is contents: read (and possibly other specific read-only scopes if needed in the future). We can place permissions: at the workflow root (so it applies to all jobs unless overridden) or under the specific job.

The minimal, non-breaking change is to add a top-level permissions: block with contents: read after the on: section. This documents the token scope, ensures the workflow won’t get unexpected write access if defaults change, and should not affect the existing functionality because checkout and read operations work with contents: read. No extra imports or dependencies are needed; this is a pure YAML configuration change in .github/workflows/native_full_build.yml.

Concretely, in .github/workflows/native_full_build.yml, insert:

permissions:
  contents: read

between the on: block (lines 3–7) and the jobs: block (line 9).

Suggested changeset 1
.github/workflows/native_full_build.yml

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/.github/workflows/native_full_build.yml b/.github/workflows/native_full_build.yml
--- a/.github/workflows/native_full_build.yml
+++ b/.github/workflows/native_full_build.yml
@@ -6,6 +6,9 @@
   pull_request:
     branches: [ main, 'sprint/**', 'release/**', topic/*, develop ]
 
+permissions:
+  contents: read
+
 jobs:
   build-entservices-on-pr:
     name: Build xdialserver component in github rdkcentral
EOF
@@ -6,6 +6,9 @@
pull_request:
branches: [ main, 'sprint/**', 'release/**', topic/*, develop ]

permissions:
contents: read

jobs:
build-entservices-on-pr:
name: Build xdialserver component in github rdkcentral
Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Copy Markdown

Copilot AI left a 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 updates the GDial server stack to better support newer GSSDP/libSoup combinations (notably GSSDP 1.6.x + libSoup 3), while also extending the platform/service interfaces to propagate manufacturer/model names and introducing UUID-based per-app URIs for local REST routing.

Changes:

  • Add libSoup 3 + GSSDP 1.6 build path and introduce gdial1p6-* source variants for REST/SSDP/shield.
  • Extend GDial callbacks/service plumbing for manufacturer/model name updates and adjust local REST routing to use UUID-based app URIs.
  • Add optional security-token disabling, plus CI/build scripts and stubs to support native builds.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 18 comments.

Show a summary per file
File Description
stubs/securityagent/SecurityTokenUtil.h Adds a stub header for security token retrieval.
stubs/securityagent/SecurityTokenUtil.cpp Adds a stub implementation of GetSecurityToken().
stubs/iarm_stubs.cpp Adds IARM bus stub implementations for native builds.
server/plat/gdialappcache.hpp Initializes observer pointer to nullptr.
server/plat/gdialappcache.cpp Improves cache update/logging and adds a null-check on cache lookup.
server/plat/gdial.hpp Adds manufacturer/model callback types + registration APIs.
server/plat/gdial.cpp Adds manufacturer/model update flow; conditional security-token use; logging/ownership tweaks.
server/plat/gdial-plat-util.c Fixes thread id formatting and adds a Coverity suppression annotation.
server/plat/gdial-plat-dev.c Removes getenv-based manufacturer/model getters.
server/plat/gdial-plat-app.c Adds manufacturer/model callback registration and update entry points.
server/plat/gdial-os-app.h Exposes OS-level manufacturer/model update APIs.
server/plat/gdial_app_registry.c Adds UUID persistence and stores a UUID-derived app_uri in the registry.
server/plat/CMakeLists.txt Adds DISABLE_SECURITY_TOKEN option and conditional linking changes.
server/include/gdialserviceimpl.h Adds new request event fields; removes onStopped; initializes observer.
server/include/gdialservicecommon.h Removes GDialNotifier::onStopped().
server/include/gdialservice.h Adds setManufacturerName() / setModelName().
server/include/gdial-plat-dev.h Removes manufacturer/model getter declarations.
server/include/gdial-plat-app.h Adds platform callbacks and update APIs for manufacturer/model.
server/include/gdial-config.h Updates include guard and removes an unused enum block.
server/include/gdial_app_registry.h Adds APP_MAX_UUID_SIZE and app_uri storage to the registry struct.
server/gdialservice.cpp Adds manufacturer/model handlers; adds libSoup3 compatibility in throttle + URI logging paths.
server/gdialserver_ut.cpp Updates unit test scaffolding for notifier interface changes and move semantics.
server/gdial1p6-ssdp.c Introduces libSoup3-compatible SSDP implementation with manufacturer/model support.
server/gdial1p6-shield.c Introduces libSoup3-compatible shield/throttling implementation.
server/gdial1p6-rest.c Introduces libSoup3-compatible REST implementation and UUID-based local routing.
server/gdial-ssdp.h Adds manufacturer/model setter APIs for SSDP.
server/gdial-ssdp.c Adds manufacturer/model support and mutex protection in SSDP path.
server/gdial-rest.h Extends additionalDataUrl builder signature and adds lookup-by-UUID declaration.
server/gdial-rest.c Implements lookup-by-UUID, UUID-based local handler routing, and adjusted additionalDataUrl building.
server/CMakeLists.txt Adds libSoup3 detection, prefers GSSDP 1.6 when available, switches sources based on libSoup version.
Makefile Passes DISABLE_SECURITY_TOKEN through to CMake invocation.
cov_build.sh Adds a Coverity build helper script.
build_dependencies.sh Adds a dependency/bootstrap script for native builds (incl. building gssdp).
.github/workflows/native_full_build.yml Adds a GitHub Actions workflow for native builds in a container.
.github/CODEOWNERS Adds default maintainers as code owners.
Comments suppressed due to low confidence (1)

server/gdial-ssdp.c:264

  • gdial_ssdp_destroy removes handler "/dd.xml", but gdial_ssdp_new registers the SSDP handler at a UUID-scoped path ("/%s/dd.xml"). This mismatch leaves the handler installed after destroy and can result in callbacks firing on freed state. Please store the exact handler path used during registration and remove that same path during destroy.
  pthread_mutex_lock(&ssdpServerEventSync);
  if (ssdp_http_server_)
  {
    soup_server_remove_handler(ssdp_http_server_, "/dd.xml");
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +50 to +54
size_t len = payload.length();

if(!memcpy(buffer,payload.c_str(),len))
return -1;
return 0;
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetSecurityToken’s implementation violates its own contract: it always returns 0 on success (the header comment says success must be > 0 and 0 should never occur), doesn’t check maxLength, and doesn’t NUL-terminate the buffer. This can cause callers to treat success as failure and can lead to reading past the written bytes when the buffer is used as a C-string. Please return the number of bytes written (or a negative required length on insufficient buffer), validate maxLength, and write a terminator when returning a string token.

Copilot uses AI. Check for mistakes.
Comment thread stubs/iarm_stubs.cpp
Comment on lines +5 to +8
IARM_Result_t IARM_Malloc(IARM_MemType_t type, size_t size, void **ptr)
{
return IARM_RESULT_SUCCESS;
}
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IARM_Malloc returns success without allocating or setting *ptr, which can lead to use of an uninitialized pointer by callers. Even for a stub, it should either allocate (e.g., malloc/calloc) and populate *ptr (and have IARM_Free release it), or return an error code to force callers down the failure path.

Copilot uses AI. Check for mistakes.
Comment thread server/plat/gdial.hpp
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);
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gdial_register_modelname_cb is declared with the wrong callback type (it takes gdial_manufacturername_cb instead of gdial_modelname_cb). This breaks the API contract and can cause type mismatches for callers. Please update the function signature to accept gdial_modelname_cb.

Suggested change
void gdial_register_modelname_cb(gdial_manufacturername_cb cb);
void gdial_register_modelname_cb(gdial_modelname_cb cb);

Copilot uses AI. Check for mistakes.
Comment thread server/plat/gdial.cpp
GDIAL_LOGTRACE("Entering ...");
if ((nullptr == GDialObjHandle)||(nullptr == model))
{
GDIAL_LOGERROR("NULL GDialObjHandle[%p] manufacturer[%p]",GDialObjHandle,model);
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log message labels the second pointer as manufacturer, but this code path is for the model name. This makes diagnostics misleading. Please update the message to refer to model[%p] (or the parameter name) instead of manufacturer[%p].

Suggested change
GDIAL_LOGERROR("NULL GDialObjHandle[%p] manufacturer[%p]",GDialObjHandle,model);
GDIAL_LOGERROR("NULL GDialObjHandle[%p] model[%p]", GDialObjHandle, model);

Copilot uses AI. Check for mistakes.
Comment thread server/gdial-ssdp.c
static gchar *app_random_uuid = NULL;
static gchar *app_manufacturer_name = NULL;
static gchar *app_model_name = NULL;
static pthread_mutex_t ssdpServerEventSync = PTHREAD_MUTEX_INITIALIZER;
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssdpServerEventSync is declared with PTHREAD_MUTEX_INITIALIZER and is also explicitly initialized with pthread_mutex_init() in gdial_ssdp_new(). Re-initializing an already-initialized mutex can fail (e.g., EBUSY) or be undefined behavior. Please choose one initialization strategy: either keep the static initializer and remove pthread_mutex_init(), or remove the initializer and keep pthread_mutex_init().

Suggested change
static pthread_mutex_t ssdpServerEventSync = PTHREAD_MUTEX_INITIALIZER;
static pthread_mutex_t ssdpServerEventSync;

Copilot uses AI. Check for mistakes.
Comment thread build_dependencies.sh
Comment on lines +43 to +45
git clone --branch gssdp-1.2.3 https://gitlab.gnome.org/GNOME/gssdp.git

############################
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script still clones gssdp branch gssdp-1.2.3 (and later builds it), which doesn’t align with the PR goal of upgrading to gssdp 1.6.3. If the intent is to validate the 1.6.x upgrade path, please update the script to fetch/build gssdp-1.6.x (and install libsoup-3.0-dev if required), or clarify why 1.2.3 is still used here.

Copilot uses AI. Check for mistakes.
Comment thread server/gdial-ssdp.c
static gchar *app_manufacturer_name = NULL;
static gchar *app_model_name = NULL;
static pthread_mutex_t ssdpServerEventSync = PTHREAD_MUTEX_INITIALIZER;

Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app_manufacturer_name/app_model_name are allocated via g_strdup in gdial_ssdp_set_* but never freed in gdial_ssdp_destroy() (and app_friendly_name is also leaked). This introduces a persistent leak across the server lifetime and across repeated init/term cycles. Please free these globals (and null them) during destroy/term.

Suggested change
/* Ensure global SSDP strings are freed on termination to avoid persistent leaks. */
static void gdial_ssdp_global_cleanup(void) __attribute__((destructor));
static void
gdial_ssdp_global_cleanup(void)
{
if (dd_xml_response_str_) {
g_free(dd_xml_response_str_);
dd_xml_response_str_ = NULL;
}
if (app_friendly_name) {
g_free(app_friendly_name);
app_friendly_name = NULL;
}
if (app_manufacturer_name) {
g_free(app_manufacturer_name);
app_manufacturer_name = NULL;
}
if (app_model_name) {
g_free(app_model_name);
app_model_name = NULL;
}
}

Copilot uses AI. Check for mistakes.
static gchar *app_random_uuid = NULL;
static gchar *app_manufacturer_name = NULL;
static gchar *app_model_name = NULL;
static pthread_mutex_t ssdpServerEventSync = PTHREAD_MUTEX_INITIALIZER;
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssdpServerEventSync is declared with PTHREAD_MUTEX_INITIALIZER but is also explicitly initialized with pthread_mutex_init() in gdial_ssdp_new(). Re-initializing an already-initialized mutex can fail (e.g., EBUSY) or be undefined behavior. Please choose one initialization strategy (static initializer OR pthread_mutex_init).

Suggested change
static pthread_mutex_t ssdpServerEventSync = PTHREAD_MUTEX_INITIALIZER;
static pthread_mutex_t ssdpServerEventSync;

Copilot uses AI. Check for mistakes.
pthread_mutex_lock(&ssdpServerEventSync);
if (ssdp_http_server_)
{
soup_server_remove_handler(ssdp_http_server_, "/dd.xml");
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gdial_ssdp_destroy removes handler "/dd.xml", but gdial_ssdp_new registers the SSDP handler at a UUID-scoped path ("/%s/dd.xml"). This mismatch leaves the handler installed after destroy and can result in callbacks firing on freed state. Please store the exact handler path used during registration and remove that same path during destroy.

Suggested change
soup_server_remove_handler(ssdp_http_server_, "/dd.xml");
if (gdial_options_ != NULL && gdial_options_->uuid != NULL)
{
gchar *handler_path = g_strdup_printf("/%s/dd.xml", gdial_options_->uuid);
if (handler_path != NULL)
{
soup_server_remove_handler(ssdp_http_server_, handler_path);
g_free(handler_path);
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +76 to +80
static gchar *app_friendly_name = NULL;
static gchar *app_random_uuid = NULL;
static gchar *app_manufacturer_name = NULL;
static gchar *app_model_name = NULL;
static pthread_mutex_t ssdpServerEventSync = PTHREAD_MUTEX_INITIALIZER;
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app_manufacturer_name/app_model_name (and app_friendly_name) are heap-allocated via g_strdup but are never freed in gdial_ssdp_destroy(). This leaks memory across the server lifetime and across repeated init/term cycles. Please free these strings (and null them) during destroy.

Copilot uses AI. Check for mistakes.
@rdkcmf-jenkins
Copy link
Copy Markdown
Contributor

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 2 files pending identification.

  • Protex Server Path: /home/blackduck/github/xdialserver/196/rdkcentral/xdialserver

  • Commit: 9bc1b7a

Report detail: gist'

Copilot AI review requested due to automatic review settings April 6, 2026 08:52
@rdkcmf-jenkins
Copy link
Copy Markdown
Contributor

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 2 files pending identification.

  • Protex Server Path: /home/blackduck/github/xdialserver/196/rdkcentral/xdialserver

  • Commit: e64e145

Report detail: gist'

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (4)

server/libsoup2p4/gdial-ssdp.c:81

  • ssdpServerEventSync is declared with PTHREAD_MUTEX_INITIALIZER and also initialized/destroyed via pthread_mutex_init()/pthread_mutex_destroy(). Double-initializing a statically initialized mutex is undefined. Either remove the static initializer and manage init/destroy, or keep the initializer and remove the explicit init/destroy calls.
    server/libsoup2p4/gdial-ssdp.c:263
  • gdial_ssdp_new() allocates dail_ssdp_handler for a dynamic handler path, but gdial_ssdp_destroy() removes only "/dd.xml". Persist/remove the same dynamic path during destroy; otherwise the handler remains registered (and the allocated handler string should also be freed after registration).
    server/libsoup2p4/gdial-rest.c:501
  • additional_data_url_safe is allocated by soup_uri_encode() but released with free(). Use g_free() (and likewise avoid mixing GLib allocators with libc free) to prevent allocator mismatches.
    server/libsoup2p4/gdial-rest.c:462
  • additional_data_url may be NULL when use_additional_data is false, but it is still passed to soup_uri_encode() and logged with %s. Guard the encode/logging so NULL stays NULL to avoid undefined behavior when printing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/plat/gdial.cpp
Comment on lines 596 to +599
}
#endif
controllerRemoteObject = new JSONRPC::LinkType<Core::JSON::IElement>(std::string(), false, query);

Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

controllerRemoteObject is allocated unconditionally, which overwrites the global pointer and leaks any previously allocated LinkType. Keep the allocation inside the if (NULL == controllerRemoteObject) block (or delete/reset the old object before reassigning).

Copilot uses AI. Check for mistakes.
Comment on lines 95 to +97
AppInfo* appEntry = ObjectCache->findObject(id);

state = appEntry->appState;
GDIAL_LOGINFO("APPCache: App Name[%s] AppID[%s] Error[%s]",
appEntry->appName.c_str(),
appEntry->appId.c_str(),
appEntry->appError.c_str());
// FIX(Copilot): Add NULL check for appEntry
if (appEntry) {
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The // FIX(Copilot): ... marker comments look like autogenerated scaffolding and don't add long-term value. Consider removing or rewriting them as normal rationale/comments before merging to keep the codebase clean.

Copilot uses AI. Check for mistakes.
Comment thread server/CMakeLists.txt Outdated
Comment on lines +90 to +95
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial1p6-rest.c
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial1p6-ssdp.c
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial1p6-shield.c
${CMAKE_CURRENT_SOURCE_DIR}/gdialservice.cpp
)
message("Using libsoup-3.0 compatible source files (gdial1p6-*.c)")
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The libsoup3 build path lists libsoup3/gdial1p6-rest.c (and related files), but the actual files in server/libsoup3/ are gdial-rest.c, gdial-ssdp.c, and gdial-shield.c. Update these filenames (or rename the files) or the libsoup3 build will fail.

Suggested change
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial1p6-rest.c
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial1p6-ssdp.c
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial1p6-shield.c
${CMAKE_CURRENT_SOURCE_DIR}/gdialservice.cpp
)
message("Using libsoup-3.0 compatible source files (gdial1p6-*.c)")
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial-rest.c
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial-ssdp.c
${CMAKE_CURRENT_SOURCE_DIR}/libsoup3/gdial-shield.c
${CMAKE_CURRENT_SOURCE_DIR}/gdialservice.cpp
)
message("Using libsoup-3.0 compatible source files (gdial-*.c)")

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +27
#include <stdio.h>
#include <glib.h>
#include <assert.h>
#include <libsoup/soup.h>

#include "gdial-config.h"
#include "gdial-debug.h"

Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file uses pthread_self() and usleep() but does not include the required headers. Add <pthread.h> and <unistd.h> (or equivalent) to avoid implicit declarations / build failures.

Copilot uses AI. Check for mistakes.
start_error = gdial_app_start(app, payload_safe, query_str_safe, additional_data_url_safe, gdial_rest_server);
if (query_str_safe) g_free(query_str_safe);
if (payload_safe) g_free(payload_safe);
free(additional_data_url_safe);
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

g_uri_escape_string() returns memory that must be released with g_free(), but this code uses free(). Use g_free(additional_data_url_safe) to avoid allocator mismatch.

Suggested change
free(additional_data_url_safe);
g_free(additional_data_url_safe);

Copilot uses AI. Check for mistakes.
Comment thread build_dependencies.sh
Comment on lines +1 to +3
#!/bin/bash
set -x
set -e
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script runs with set -x and clones a repo using https://$GITHUB_TOKEN@github.com/..., which will echo the token into CI logs. Avoid embedding tokens in command lines under xtrace; use a credential helper/GIT_ASKPASS, or disable set -x around the clone.

Copilot uses AI. Check for mistakes.
Comment thread build_dependencies.sh

git clone --branch main https://github.com/rdkcentral/entservices-apis.git

git clone https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloning with https://$GITHUB_TOKEN@github.com/... while xtrace is enabled will leak the token in logs. Use a safer auth mechanism (credential helper/headers) or disable set -x around this command.

Suggested change
git clone https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git
set +x
git -c http.extraheader="Authorization: Bearer $GITHUB_TOKEN" clone https://github.com/rdkcentral/entservices-testframework.git
set -x

Copilot uses AI. Check for mistakes.
Comment on lines +120 to +124
fgets(uuid_data, sizeof(uuid_data), fuuid);
fclose(fuuid);
}

snprintf( app_registry->app_uri, sizeof(app_registry->app_uri), "/%s" , uuid_data);
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading the UUID with fgets(), the buffer may contain a trailing newline. Strip trailing whitespace/newlines and validate fgets() succeeded before using uuid_data to build app_uri.

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +52
int GetSecurityToken(unsigned short maxLength, unsigned char buffer[])
{
// get a localhost token
string payload = "http://localhost";

size_t len = payload.length();

if(!memcpy(buffer,payload.c_str(),len))
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stub does not enforce maxLength and does not NUL-terminate the output. It should ensure the token fits (including terminator) and report the required length when the buffer is too small.

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +55
if(!memcpy(buffer,payload.c_str(),len))
return -1;
return 0;
}
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stub returns 0 on success, but the header contract says success should return >0 (token length) and 0 should never occur. Return the number of bytes written to match callers’ expectations.

Copilot uses AI. Check for mistakes.
@rdkcmf-jenkins
Copy link
Copy Markdown
Contributor

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 2 files pending identification.

  • Protex Server Path: /home/blackduck/github/xdialserver/196/rdkcentral/xdialserver

  • Commit: 12b8589

Report detail: gist'

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 6, 2026 11:04
@rdkcmf-jenkins
Copy link
Copy Markdown
Contributor

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 2 files pending identification.

  • Protex Server Path: /home/blackduck/github/xdialserver/196/rdkcentral/xdialserver

  • Commit: 1f9eea1

Report detail: gist'

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (2)

server/libsoup2p4/gdial-ssdp.c:265

  • dd.xml handler is added at a dynamic path ("/%s/dd.xml"), but gdial_ssdp_destroy() removes only "/dd.xml", so the registered handler will remain installed. Store the handler path (or compute it from app_random_uuid) and remove the exact same path during destroy.
    server/libsoup2p4/gdial-rest.c:463
  • When use_additional_data is false, additional_data_url remains NULL but is still passed to soup_uri_encode() and logged with %s. This can yield NULL returns and undefined behavior in logging or downstream code. Guard the encoding/logging and pass NULL/empty safely when there is no additionalDataUrl.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +45 to +55
int GetSecurityToken(unsigned short maxLength, unsigned char buffer[])
{
// get a localhost token
string payload = "http://localhost";

size_t len = payload.length();

if(!memcpy(buffer,payload.c_str(),len))
return -1;
return 0;
}
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetSecurityToken ignores maxLength and can overflow buffer; it also returns 0 on success even though the API contract says a positive length should be returned (and 0 must not occur). Please bound the copy (and NUL-terminate if callers treat it as a C-string) and return the number of bytes written (or a negative required length on failure).

Copilot uses AI. Check for mistakes.
Comment thread stubs/iarm_stubs.cpp
Comment on lines +30 to +33
IARM_Result_t IARM_Bus_IsConnected(const char* memberName, int* isRegistered)
{
return IARM_RESULT_SUCCESS;
}
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IARM_Bus_IsConnected returns success but never writes to the out-parameter isRegistered. Callers will read an uninitialized value; set *isRegistered (e.g., 1 or 0) or return an error when the value cannot be determined.

Copilot uses AI. Check for mistakes.
Comment thread server/plat/gdial.cpp
Comment on lines 588 to +599
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);

Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCurrentState() overwrites controllerRemoteObject unconditionally (line 598) even when it is already initialized, leaking the previous JSONRPC::LinkType and defeating the intended caching in the surrounding if (NULL == controllerRemoteObject). Construct the remote object only when controllerRemoteObject is null and handle the token-failure path explicitly (e.g., avoid creating a link with an empty/invalid query string).

Copilot uses AI. Check for mistakes.
Comment thread server/CMakeLists.txt
Comment on lines +29 to +47
pkg_search_module (LIBSOUP3 libsoup-3.0)
if (LIBSOUP3_FOUND)
add_definitions(-DHAVE_LIBSOUP_VERSION_3)
pkg_search_module (GSSDP16 gssdp-1.6)
if (GSSDP16_FOUND)
pkg_search_module (GSSDP REQUIRED gssdp-1.6)
add_definitions(-DHAVE_GSSDP_VERSION_1_6_OR_NEWER)
message("Using gssdp-1.6")
endif()
else()
pkg_search_module (GSSDP REQUIRED gssdp-1.0)
pkg_search_module (GSSDP12 gssdp-1.2)
if (GSSDP12_FOUND)
pkg_search_module (GSSDP REQUIRED gssdp-1.2)
add_definitions(-DHAVE_GSSDP_VERSION_1_2_OR_NEWER)
message("Using gssdp-1.2")
else()
pkg_search_module (GSSDP REQUIRED gssdp-1.0)
endif()
endif()
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When libsoup-3.0 is found, gssdp-1.6 is only used if found, but there is no fallback and the build still unconditionally uses ${GSSDP_INCLUDE_DIRS}/${GSSDP_LIBRARIES}. If gssdp-1.6 is missing, this configuration will fail later. Consider making gssdp-1.6 REQUIRED under the libsoup-3.0 path (or add an explicit fallback/error).

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +43
static void soup_message_weak_ref_callback(gpointer user_data, GObject *obj);
static void server_request_remove_callback (SoupMessage *msg) {
GDIAL_LOGTRACE("Entering ...");
DialShieldConnectionContext * conn_context = (DialShieldConnectionContext *) g_hash_table_lookup(active_conns_, msg);
if (conn_context) {
if (conn_context->read_timeout_source != 0) {
g_print_with_timestamp("server_request_remove_callback tid=[%lx] msg=%p timeout source %d removed",
pthread_self(), msg, conn_context->read_timeout_source);
g_source_remove(conn_context->read_timeout_source);
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This libsoup-3 shield implementation mixes SoupServerMessage* in callbacks with helper functions typed as SoupMessage* (e.g., server_request_remove_callback), which is not type-correct and will not compile cleanly against libsoup-3 headers. Align the helper signatures and hashtable key types to SoupServerMessage* for the libsoup-3 build.

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +27
#include <stdio.h>
#include <glib.h>
#include <assert.h>
#include <libsoup/soup.h>

#include "gdial-config.h"
#include "gdial-debug.h"

Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gdial-shield.c uses pthread_self() and usleep() but does not include the required headers (<pthread.h> and <unistd.h>), which can cause implicit-declaration build failures/warnings depending on toolchain flags. Add the missing includes in this file.

Copilot uses AI. Check for mistakes.
Comment on lines +462 to +466
if (app_registry->use_additional_data) {
additional_data_url = gdial_rest_server_new_additional_data_url(listening_port, app_registry->name, FALSE, app_registry->app_uri );
}
gchar *additional_data_url_safe = g_uri_escape_string(additional_data_url, NULL, FALSE);
GDIAL_LOGINFO("additionalDataUrl = %s, %s", additional_data_url, additional_data_url_safe);
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When use_additional_data is false, additional_data_url remains NULL but is still passed to g_uri_escape_string() and logged with %s. This can yield NULL returns and undefined behavior in logging or downstream code. Guard the encoding/logging and pass NULL/empty safely when there is no additionalDataUrl.

Suggested change
if (app_registry->use_additional_data) {
additional_data_url = gdial_rest_server_new_additional_data_url(listening_port, app_registry->name, FALSE, app_registry->app_uri );
}
gchar *additional_data_url_safe = g_uri_escape_string(additional_data_url, NULL, FALSE);
GDIAL_LOGINFO("additionalDataUrl = %s, %s", additional_data_url, additional_data_url_safe);
gchar *additional_data_url_safe = NULL;
if (app_registry->use_additional_data) {
additional_data_url = gdial_rest_server_new_additional_data_url(listening_port, app_registry->name, FALSE, app_registry->app_uri );
additional_data_url_safe = g_uri_escape_string(additional_data_url, NULL, FALSE);
}
GDIAL_LOGINFO("additionalDataUrl = %s, %s",
additional_data_url ? additional_data_url : "",
additional_data_url_safe ? additional_data_url_safe : "");

Copilot uses AI. Check for mistakes.
Comment on lines 95 to +98
AppInfo* appEntry = ObjectCache->findObject(id);

state = appEntry->appState;
GDIAL_LOGINFO("APPCache: App Name[%s] AppID[%s] Error[%s]",
appEntry->appName.c_str(),
appEntry->appId.c_str(),
appEntry->appError.c_str());
// FIX(Copilot): Add NULL check for appEntry
if (appEntry) {
state = appEntry->appState;
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove tool-generated markers like "FIX(Copilot)" from production code comments; they don’t describe the behavior and add noise for future maintainers. Prefer a brief rationale (or no comment) instead.

Copilot uses AI. Check for mistakes.
Comment on lines +99 to +116
char app_uuid_file_path[64] = {0};
char uuid_data[APP_MAX_UUID_SIZE] = {0};
snprintf( app_uuid_file_path, sizeof(app_uuid_file_path), UUID_FILE_TEMPLATE , app_name);

FILE *fuuid = fopen(app_uuid_file_path, "r");
if (fuuid == NULL)
{
uuid_t random_uuid;
uuid_generate_random(random_uuid);
uuid_unparse(random_uuid, uuid_data);
GDIAL_LOGINFO("generated uuid:[%s]", uuid_data);

fuuid = fopen(app_uuid_file_path, "w");
if (fuuid != NULL)
{
fprintf(fuuid, "%s", uuid_data);
fclose(fuuid);
}
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UUID persistence file is created in /tmp using a predictable name and opened with fopen("w"), which is vulnerable to symlink/hardlink attacks and can overwrite arbitrary files if an attacker pre-creates the path. Consider using an atomic/safe open pattern (e.g., open with O_NOFOLLOW|O_CREAT|O_EXCL and restrictive permissions) and sanitizing app_name for filesystem safety.

Copilot uses AI. Check for mistakes.
Comment thread build_dependencies.sh
Comment on lines +12 to +55
apt update
apt install -y git python3 python3-pip cmake ninja-build meson curl libsoup2.4-dev libxml2-dev libglib2.0-dev gobject-introspection libgirepository1.0-dev libgtk-3-dev libcurl4-openssl-dev libcunit1-dev valac pandoc
pip install jsonref

############################
# Build trevor-base64
if [ ! -d "trower-base64" ]; then
git clone https://github.com/xmidt-org/trower-base64.git
fi
cd trower-base64
meson setup --warnlevel 3 --werror build
ninja -C build
ninja -C build install
cd ..
###########################################
# Clone the required repositories

rm -rf iarmbus ThunderTools Thunder entservices-apis entservices-testframework gssdp


git clone https://github.com/rdkcentral/iarmbus.git
export IARMBUS_PATH=$GITHUB_WORKSPACE/iarmbus

git clone --branch R4.4.3 https://github.com/rdkcentral/ThunderTools.git

git clone --branch R4.4.1 https://github.com/rdkcentral/Thunder.git

git clone --branch main https://github.com/rdkcentral/entservices-apis.git

git clone https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git

git clone --branch gssdp-1.2.3 https://gitlab.gnome.org/GNOME/gssdp.git

############################
# Build gssdp-1.2
echo "======================================================================================"
echo "buliding gssdp-1.2"
cd gssdp

rm -rf build
meson setup build

ninja -C build
ninja -C build install
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI dependency script still clones/builds gssdp-1.2.3 (and installs only libsoup2.4-dev), which doesn’t align with the PR goal of upgrading to gssdp 1.6.3. If this workflow is intended to validate the new libsoup-3/gssdp-1.6 path, update the script accordingly (or clarify why it remains on 1.2.x).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.