RDKEMW-6898: gdialserver not compatible with Libsoup3 library#197
RDKEMW-6898: gdialserver not compatible with Libsoup3 library#197
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates gdialserver to support libsoup-3.0 by switching to gssdp-1.6–compatible code paths and conditionally building libsoup2 vs libsoup3 implementations.
Changes:
- Add libsoup-3.0 compatible REST/SSDP/shield implementations (
gdial1p6-*.c) and select them via CMake when libsoup3 is detected. - Add libsoup3-specific handling in
gdialservice.cppfor server callbacks and URI logging. - Update dependency detection logic to prefer gssdp-1.6 when libsoup3 is present.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| server/gdialservice.cpp | Adds libsoup3-specific callback signatures and URI/logging handling. |
| server/gdial1p6-ssdp.c | Introduces libsoup3 SSDP HTTP handling and gssdp-1.6 client setup. |
| server/gdial1p6-shield.c | Introduces libsoup3 shield hooks using SoupServerMessage. |
| server/gdial1p6-rest.c | Introduces libsoup3 REST server implementation using SoupServerMessage. |
| server/CMakeLists.txt | Selects libsoup3 + gssdp-1.6 build inputs and defines feature macros. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (0 != pthread_mutex_init(&ssdpServerEventSync, NULL)) | ||
| { | ||
| GDIAL_LOGERROR("Failed to initializing mutex"); | ||
| return EXIT_FAILURE; | ||
| } | ||
|
|
There was a problem hiding this comment.
ssdpServerEventSync is initialized with PTHREAD_MUTEX_INITIALIZER and then pthread_mutex_init() is called again in gdial_ssdp_new(). Re-initializing an already-initialized mutex is undefined behavior. Either remove the static initializer and rely on pthread_mutex_init(), or keep the initializer and remove the explicit init call (and adjust destroy accordingly).
| if (0 != pthread_mutex_init(&ssdpServerEventSync, NULL)) | |
| { | |
| GDIAL_LOGERROR("Failed to initializing mutex"); | |
| return EXIT_FAILURE; | |
| } |
| app_random_uuid = g_strdup(random_uuid); | ||
| gchar *dail_ssdp_handler = g_strdup_printf("/%s/%s", random_uuid,"dd.xml"); | ||
| soup_server_add_handler(ssdp_http_server_, dail_ssdp_handler, ssdp_http_server_callback, NULL, NULL); | ||
| ssdp_client_ = ssdp_client; |
There was a problem hiding this comment.
The handler path string allocated with g_strdup_printf is never freed after soup_server_add_handler, which leaks memory. If libsoup copies the path internally (typical), free dail_ssdp_handler after adding the handler; if not, store it (e.g., as a global) and free it during destroy.
| char *tmp = g_uri_escape_string(query_str, NULL, FALSE); | ||
| // note that we later g_free(query_str_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| query_str_safe = g_strdup(tmp); | ||
| free(tmp); | ||
| } |
There was a problem hiding this comment.
g_uri_escape_string() returns memory that must be released with g_free(), but this code calls free(tmp). Using the wrong deallocator is undefined behavior. Replace free(tmp) with g_free(tmp) (and keep g_free(query_str_safe) as-is).
| char *tmp = g_uri_escape_string(payload, "=&", FALSE); | ||
| // note that we later g_free(payload_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| payload_safe = g_strdup(tmp); | ||
| free(tmp); | ||
| } |
There was a problem hiding this comment.
Same issue as above: tmp is allocated by g_uri_escape_string() and must be freed with g_free(), not free().
| gchar *additional_data_url = 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 ); | ||
| } | ||
| gchar *additional_data_url_safe = g_uri_escape_string(additional_data_url, NULL, FALSE); |
There was a problem hiding this comment.
additional_data_url is only set when app_registry->use_additional_data is true, but g_uri_escape_string(additional_data_url, ...) is called unconditionally. If use_additional_data is false, this passes NULL and can crash. Guard the escape call (and the later free) so NULL is handled safely.
| pthread_mutex_lock(&ssdpServerEventSync); | ||
| if (ssdp_http_server_) | ||
| { | ||
| soup_server_remove_handler(ssdp_http_server_, "/dd.xml"); |
There was a problem hiding this comment.
gdial_ssdp_destroy() removes a handler for "/dd.xml", but the handler that was added uses the per-instance path "/%s/dd.xml". This means the actual handler is never removed, which can lead to callbacks firing after globals are cleared/unref’d. Remove the same path that was added (e.g., reconstruct it from app_random_uuid or store the handler path).
| soup_server_remove_handler(ssdp_http_server_, "/dd.xml"); | |
| if (app_random_uuid != NULL) | |
| { | |
| gchar *dd_xml_handler_path = g_strdup_printf("/%s/dd.xml", app_random_uuid); | |
| soup_server_remove_handler(ssdp_http_server_, dd_xml_handler_path); | |
| g_free(dd_xml_handler_path); | |
| } | |
| else | |
| { | |
| soup_server_remove_handler(ssdp_http_server_, "/dd.xml"); | |
| } |
| 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) { |
There was a problem hiding this comment.
This file uses SoupMessage* for helper signatures and hash table keys, but the connected libsoup-3.0 server signals provide SoupServerMessage*. Mixing these types triggers incompatible-pointer warnings and can break if the underlying types diverge. Update the helpers and hash table key type to consistently use SoupServerMessage*.
| if (encoded_href) { | ||
| char *tmp = g_uri_escape_string(encoded_href, NULL, FALSE); | ||
| rbuilder->link_href= g_strdup(tmp); | ||
| free(tmp); |
There was a problem hiding this comment.
tmp is allocated by g_uri_escape_string() and should be released with g_free(), not free(). Otherwise this can crash depending on the allocator used by GLib.
| free(tmp); | |
| g_free(tmp); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| g_object_ref(ssdp_http_server); | ||
| ssdp_http_server_ = ssdp_http_server; | ||
| app_random_uuid = g_strdup(random_uuid); | ||
| gchar *dail_ssdp_handler = g_strdup_printf("/%s/%s", random_uuid,"dd.xml"); | ||
| soup_server_add_handler(ssdp_http_server_, dail_ssdp_handler, ssdp_http_server_callback, NULL, NULL); | ||
| ssdp_client_ = ssdp_client; | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| int gdial_ssdp_destroy() { | ||
| GDIAL_LOGTRACE("Entering ..."); | ||
| pthread_mutex_lock(&ssdpServerEventSync); | ||
| if (ssdp_http_server_) | ||
| { | ||
| soup_server_remove_handler(ssdp_http_server_, "/dd.xml"); | ||
| } |
There was a problem hiding this comment.
dail_ssdp_handler is allocated and registered (e.g. /<uuid>/dd.xml), but it is never freed, and gdial_ssdp_destroy() removes the hard-coded handler "/dd.xml" instead of the dynamically-added path. This can leak memory and leave the handler registered; store the handler path (or remove using the same string) and free the allocated string after soup_server_add_handler().
| 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; |
There was a problem hiding this comment.
The mutex is declared with PTHREAD_MUTEX_INITIALIZER and then later re-initialized with pthread_mutex_init(). Re-initializing an already-initialized mutex is undefined/may fail with EBUSY. Either remove the static initializer and always init/destroy at runtime, or keep the initializer and drop the explicit init/destroy calls.
| static pthread_mutex_t ssdpServerEventSync = PTHREAD_MUTEX_INITIALIZER; | |
| static pthread_mutex_t ssdpServerEventSync; |
|
|
||
| #include <stdio.h> | ||
| #include <glib.h> | ||
| #include <assert.h> |
There was a problem hiding this comment.
This file uses pthread_self() and usleep() but does not include the required headers (<pthread.h> and <unistd.h>). With modern C compilers this can be a hard compile error due to missing prototypes; add the proper includes.
| #include <assert.h> | |
| #include <assert.h> | |
| #include <pthread.h> | |
| #include <unistd.h> |
| 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); |
There was a problem hiding this comment.
additional_data_url is only set when use_additional_data is true, but g_uri_escape_string(additional_data_url, ...) is called unconditionally. If use_additional_data is false this passes NULL and can crash. Only escape when additional_data_url != NULL, otherwise keep additional_data_url_safe as NULL and pass NULL through to gdial_app_start().
| 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 (additional_data_url != NULL) { | |
| additional_data_url_safe = g_uri_escape_string(additional_data_url, NULL, FALSE); | |
| } | |
| GDIAL_LOGINFO("additionalDataUrl = %s, %s", | |
| additional_data_url ? additional_data_url : "(null)", | |
| additional_data_url_safe ? additional_data_url_safe : "(null)"); |
| GDIAL_LOGINFO("query = %s", query_str); | ||
| if (!use_query_directly_from_soup) { | ||
| char *tmp = g_uri_escape_string(query_str, NULL, FALSE); | ||
| // note that we later g_free(query_str_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| query_str_safe = g_strdup(tmp); | ||
| free(tmp); | ||
| } |
There was a problem hiding this comment.
g_uri_escape_string() returns memory allocated with GLib allocators and must be freed with g_free(), not free(). Here tmp is freed with free(tmp), which can cause heap corruption on platforms where GLib uses a different allocator; replace with g_free(tmp) (and similarly for other g_uri_escape_string results in this file).
| /* temporary disabling encoding payload for YouTube till cloud side changed*/ | ||
| payload_safe = g_strdup(payload); | ||
| } | ||
| else { | ||
| char *tmp = g_uri_escape_string(payload, "=&", FALSE); | ||
| // note that we later g_free(payload_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| payload_safe = g_strdup(tmp); | ||
| free(tmp); | ||
| } |
There was a problem hiding this comment.
g_uri_escape_string() returns memory allocated with GLib allocators and must be freed with g_free(), not free(). Here tmp is freed with free(tmp), which can cause heap corruption on platforms where GLib uses a different allocator; replace with g_free(tmp) (and similarly for other g_uri_escape_string results in this file).
| bool flag = false; | ||
| if (strlen(elements[j]) == 0) { | ||
| j++; | ||
| continue; | ||
| } | ||
| invalid_uri = invalid_uri || g_strcmp0(copied_str[i], elements[j]); | ||
| gdial_rest_server_http_check_if_fail(!invalid_uri, msg, SOUP_STATUS_NOT_IMPLEMENTED, flag); | ||
| if(flag){ | ||
| g_strfreev(elements); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is a C file but it uses bool/false without including <stdbool.h>, which will not compile as C. Either include <stdbool.h> or switch these locals to gboolean/FALSE (consistent with GLib usage).
| gdial_rest_server_http_return_if_fail(path_len < GDIAL_REST_HTTP_MAX_URI_LEN, msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); | ||
| gdial_rest_server_http_return_if_fail(path_len > (GDIAL_STR_SIZEOF(GDIAL_REST_HTTP_APPS_URI) + GDIAL_STR_SIZEOF("/")), msg, SOUP_STATUS_NOT_IMPLEMENTED); | ||
| gdial_rest_server_http_return_if_fail(strncmp(path, GDIAL_REST_HTTP_APPS_URI, GDIAL_STR_SIZEOF(GDIAL_REST_HTTP_APPS_URI)) == 0, msg, SOUP_STATUS_NOT_IMPLEMENTED); |
There was a problem hiding this comment.
GDIAL_STR_SIZEOF() only works for string literals/arrays, but here it’s applied to GDIAL_REST_HTTP_APPS_URI which is a gchar* set at runtime. Using GDIAL_STR_SIZEOF(GDIAL_REST_HTTP_APPS_URI) will evaluate to sizeof(char*)-1, causing incorrect path-length checks and an incorrect strncmp() prefix match. Replace these uses with strlen(GDIAL_REST_HTTP_APPS_URI) (or cache the length) to validate the dynamic prefix correctly.
| gdial_rest_server_http_return_if_fail(path_len < GDIAL_REST_HTTP_MAX_URI_LEN, msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); | |
| gdial_rest_server_http_return_if_fail(path_len > (GDIAL_STR_SIZEOF(GDIAL_REST_HTTP_APPS_URI) + GDIAL_STR_SIZEOF("/")), msg, SOUP_STATUS_NOT_IMPLEMENTED); | |
| gdial_rest_server_http_return_if_fail(strncmp(path, GDIAL_REST_HTTP_APPS_URI, GDIAL_STR_SIZEOF(GDIAL_REST_HTTP_APPS_URI)) == 0, msg, SOUP_STATUS_NOT_IMPLEMENTED); | |
| size_t apps_uri_len = strlen(GDIAL_REST_HTTP_APPS_URI); | |
| gdial_rest_server_http_return_if_fail(path_len < GDIAL_REST_HTTP_MAX_URI_LEN, msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); | |
| gdial_rest_server_http_return_if_fail(path_len > (apps_uri_len + GDIAL_STR_SIZEOF("/")), msg, SOUP_STATUS_NOT_IMPLEMENTED); | |
| gdial_rest_server_http_return_if_fail(strncmp(path, GDIAL_REST_HTTP_APPS_URI, apps_uri_len) == 0, msg, SOUP_STATUS_NOT_IMPLEMENTED); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const gchar *copied_str[] = {base, app_name, instance, last_elem}; | ||
| i = 0; j = 0; | ||
| while (i < element_num && (unsigned int)i < sizeof(copied_str)/sizeof(copied_str[0])) { | ||
| bool flag = false; |
There was a problem hiding this comment.
bool/false are used here but this C file does not include <stdbool.h>, which will fail to compile with a C compiler. Either include <stdbool.h> (and use true/false) or switch to GLib types (gboolean with TRUE/FALSE).
| bool flag = false; | |
| gboolean flag = FALSE; |
| char *tmp = g_uri_escape_string(query_str, NULL, FALSE); | ||
| // note that we later g_free(query_str_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| query_str_safe = g_strdup(tmp); | ||
| free(tmp); | ||
| } | ||
| else { | ||
| query_str_safe = g_strdup(query_str); | ||
| } | ||
| } | ||
| const gchar *payload = request_body->data; | ||
| gchar *payload_safe = NULL; | ||
| if (payload && strlen(payload)) { | ||
| if (g_str_has_prefix(app->name, "YouTube")) { | ||
| /* temporary disabling encoding payload for YouTube till cloud side changed*/ | ||
| payload_safe = g_strdup(payload); | ||
| } | ||
| else { | ||
| char *tmp = g_uri_escape_string(payload, "=&", FALSE); | ||
| // note that we later g_free(payload_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| payload_safe = g_strdup(tmp); | ||
| free(tmp); | ||
| } | ||
| } | ||
| 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); | ||
| g_free(additional_data_url); |
There was a problem hiding this comment.
The strings returned by g_uri_escape_string() should be released with g_free(), not free(). This affects both the temporary tmp values and additional_data_url_safe; using free() here is allocator-mismatched and can crash.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char *tmp = g_uri_escape_string(query_str, NULL, FALSE); | ||
| // note that we later g_free(query_str_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| query_str_safe = g_strdup(tmp); | ||
| free(tmp); |
There was a problem hiding this comment.
tmp is allocated by g_uri_escape_string() (GLib allocator) but is freed with free(). This should be g_free(tmp) to avoid allocator mismatch/heap corruption.
| char *tmp = g_uri_escape_string(payload, "=&", FALSE); | ||
| // note that we later g_free(payload_safe) which doesn't necessarily work with malloc'ed memory (seems to depend on glib version) | ||
| payload_safe = g_strdup(tmp); | ||
| free(tmp); | ||
| } |
There was a problem hiding this comment.
tmp is allocated by g_uri_escape_string() (GLib allocator) but is freed with free(). This should be g_free(tmp) to avoid allocator mismatch/heap corruption.
| g_object_ref(ssdp_http_server); | ||
| ssdp_http_server_ = ssdp_http_server; | ||
| app_random_uuid = g_strdup(random_uuid); | ||
| gchar *dail_ssdp_handler = g_strdup_printf("/%s/%s", random_uuid,"dd.xml"); | ||
| soup_server_add_handler(ssdp_http_server_, dail_ssdp_handler, ssdp_http_server_callback, NULL, NULL); | ||
| ssdp_client_ = ssdp_client; | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| int gdial_ssdp_destroy() { | ||
| GDIAL_LOGTRACE("Entering ..."); | ||
| pthread_mutex_lock(&ssdpServerEventSync); | ||
| if (ssdp_http_server_) | ||
| { | ||
| soup_server_remove_handler(ssdp_http_server_, "/dd.xml"); | ||
| } |
There was a problem hiding this comment.
The handler is registered with a dynamic path ("/%s/dd.xml"), but destroy removes only "/dd.xml". This leaves the actual handler installed and can cause leaks/unexpected callbacks. Store the handler path (or remove using the same string) and free the allocated dail_ssdp_handler.
| #include <stdio.h> | ||
| #include <glib.h> | ||
| #include <assert.h> | ||
| #include <libsoup/soup.h> | ||
|
|
||
| #include "gdial-config.h" | ||
| #include "gdial-debug.h" | ||
|
|
There was a problem hiding this comment.
This file uses pthread_self() and usleep() but does not include the required headers (<pthread.h> and <unistd.h>). Relying on indirect includes will cause build failures or implicit-declaration warnings. Add the explicit includes.
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Reason for change: gdialserver not compatible with Libsoup3 library
Updating gssdp to 1.6.3 provides libsoup3 support.
Test Procedure: Compiled and Verified
Risks: Low
Priority: P1
version: minor