Conversation
|
b'## Copyright scan failure |
There was a problem hiding this comment.
Pull request overview
Updates the GDial server build and runtime codepaths to support libsoup 3.x and gssdp 1.6, primarily by introducing libsoup-3-compatible implementations of the REST/SSDP/shield layers and selecting them at build time.
Changes:
- Add libsoup-3.0 / gssdp-1.6 CMake detection and switch to libsoup-3-compatible source files when available.
- Add new libsoup-3-compatible implementations:
gdial1p6-rest.c,gdial1p6-ssdp.c,gdial1p6-shield.c. - Update
gdialservice.cppfor libsoup-3 handler signatures and URI logging.
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/CMakeLists.txt | Detect libsoup 3 and (attempt to) select gssdp 1.6 + new source set. |
| server/gdialservice.cpp | Add libsoup-3 signature/status handling + log server URIs using GUri. |
| server/gdial1p6-rest.c | New libsoup-3 REST server implementation. |
| server/gdial1p6-ssdp.c | New gssdp-1.6 + libsoup-3 SSDP/dd.xml implementation. |
| server/gdial1p6-shield.c | New libsoup-3 request shielding/throttling implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.
When use_additional_data is false, additional_data_url stays NULL but is still passed to g_uri_escape_string(). That can trigger GLib criticals/crashes. Only call g_uri_escape_string() when additional_data_url is non-NULL, otherwise keep additional_data_url_safe as NULL.
| 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.
additional_data_url_safe comes from g_uri_escape_string() and must be freed with g_free(), not free(). Using free() here mixes allocators and can crash on some platforms.
| 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) { |
There was a problem hiding this comment.
This libsoup-3.0 variant still uses SoupMessage* for server-side messages (e.g., as hash keys and callback parameters), but the server signals provide SoupServerMessage*. In libsoup 3 these are different types, so this is unsafe type confusion. Update these APIs to consistently use SoupServerMessage* throughout this file.
| guint listening_port = g_inet_socket_address_get_port(G_INET_SOCKET_ADDRESS(socket_addr)); | ||
| gdial_rest_server_http_return_if_fail(listening_port != 0, msg, SOUP_STATUS_INTERNAL_SERVER_ERROR); | ||
|
|
||
| GDIAL_LOGERROR("Starting the app with payload %.*s", (int)request_body->length, request_body->data); |
There was a problem hiding this comment.
This log assumes request_body and request_body->data are valid. For requests without a body, request_body->data may be NULL, and passing it to %.*s is undefined behavior. Guard this log (and later payload usage) with request_body && request_body->data && request_body->length, or log a safe placeholder.
| GDIAL_LOGERROR("Starting the app with payload %.*s", (int)request_body->length, request_body->data); | |
| if (request_body && request_body->data && request_body->length) { | |
| GDIAL_LOGERROR("Starting the app with payload %.*s", (int)request_body->length, request_body->data); | |
| } else { | |
| GDIAL_LOGERROR("Starting the app with no payload"); | |
| } |
| 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() allocates with GLib’s allocator and must be freed with g_free(). Using free(tmp) here mixes allocators and can crash. Replace free(tmp) with g_free(tmp).
| 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_new() registers the handler at a dynamic path ("/%s/dd.xml"), but gdial_ssdp_destroy() removes only "/dd.xml". This leaves the handler installed after destroy and can route requests into freed state. Remove the exact same path you registered (store it or reconstruct from app_random_uuid) and free any allocated handler string.
| soup_server_remove_handler(ssdp_http_server_, "/dd.xml"); | |
| if (app_random_uuid != NULL) | |
| { | |
| gchar *handler_path = g_strdup_printf("/%s/dd.xml", app_random_uuid); | |
| if (handler_path != NULL) | |
| { | |
| soup_server_remove_handler(ssdp_http_server_, handler_path); | |
| g_free(handler_path); | |
| } | |
| } |
| } | ||
| else { | ||
| GDIAL_LOGINFO("clear [%s] dial_data", app_name); | ||
| GHashTable* empty = g_hash_table_new(NULL, NULL); |
There was a problem hiding this comment.
g_hash_table_new(NULL, NULL) is not a valid GLib API usage (hash and equal funcs must be provided unless you’re explicitly using direct hash/equal). This can assert/abort in debug builds. Create an empty table with the same key/value semantics expected by gdial_app_set_additional_dial_data (likely g_str_hash/g_str_equal, or use gdial_util_str_str_hashtable_* helper).
| GHashTable* empty = g_hash_table_new(NULL, NULL); | |
| GHashTable* empty = g_hash_table_new(g_str_hash, g_str_equal); |
| gchar *value = g_hash_table_lookup(query,"rest_enable"); | ||
| g_print_with_timestamp("gdial_rest_http_server_system_callback emit SIGNAL_REST_ENABLE value:%s ",(gchar *)value); | ||
| g_signal_emit(gdial_rest_server, gdial_rest_server_signals[SIGNAL_REST_ENABLE], 0,value); |
There was a problem hiding this comment.
gdial_rest_http_server_system_callback() calls g_hash_table_lookup(query, ...) without checking whether query is NULL. libsoup can pass NULL when there is no query string, which would crash here. Add a NULL check and treat the missing parameter as an error (or default) before emitting SIGNAL_REST_ENABLE.
| gchar *value = g_hash_table_lookup(query,"rest_enable"); | |
| g_print_with_timestamp("gdial_rest_http_server_system_callback emit SIGNAL_REST_ENABLE value:%s ",(gchar *)value); | |
| g_signal_emit(gdial_rest_server, gdial_rest_server_signals[SIGNAL_REST_ENABLE], 0,value); | |
| gchar *value = NULL; | |
| if (query != NULL) { | |
| value = g_hash_table_lookup(query, "rest_enable"); | |
| } | |
| if (value != NULL) { | |
| g_print_with_timestamp("gdial_rest_http_server_system_callback emit SIGNAL_REST_ENABLE value:%s ", (gchar *) value); | |
| g_signal_emit(gdial_rest_server, gdial_rest_server_signals[SIGNAL_REST_ENABLE], 0, value); | |
| } else { | |
| GDIAL_LOGINFO("gdial_rest_http_server_system_callback: missing or NULL 'rest_enable' parameter in PUT request"); | |
| } |
| gint instance_id = (gint)g_ascii_strtoll(instance, &endptr, 10); | ||
| if (strlen(endptr) == 0) { | ||
| app_by_instance = gdial_app_find_instance_by_instance_id(instance_id); | ||
| g_return_val_if_fail(app == app_by_instance, app); |
There was a problem hiding this comment.
If the instance id in the URL does not match the app returned by gdial_app_find_instance_by_instance_id(), the g_return_val_if_fail currently returns app (non-NULL), which means the caller will proceed and operate on the wrong instance. Return NULL on mismatch so the request can be rejected cleanly.
| g_return_val_if_fail(app == app_by_instance, app); | |
| g_return_val_if_fail(app == app_by_instance, NULL); |
|
Hi @preeja33 : There is one code match here, in gdial1p6-ssdp.c, and it comes with a Netflix copyright. The code match is to Cobalt. Where is the original Netflix for this? |
|
b'## Copyright scan failure |
|
b'## Copyright scan failure |
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.
| GDIAL_STATIC_INLINE void *GET_APP_response_builder_new(const gchar *app_name) { | ||
| GDialServerResponseBuilderGetApp *rbuilder = (GDialServerResponseBuilderGetApp *) malloc(sizeof(*rbuilder)); | ||
| memset(rbuilder, 0, sizeof(*rbuilder)); | ||
| memset(rbuilder, 0, sizeof(*rbuilder)); |
There was a problem hiding this comment.
The memset call on line 1279 is redundant as the same operation was performed on line 1278. Remove the duplicate memset call.
| memset(rbuilder, 0, sizeof(*rbuilder)); |
|
|
||
| GDIAL_STATIC_INLINE gchar *GET_APP_response_builder_build(void *builder, gsize *length) { | ||
| GDialServerResponseBuilderGetApp * rbuilder = (GDialServerResponseBuilderGetApp *)builder; | ||
| GString *rbuf = g_string_new_len('\0', 128); |
There was a problem hiding this comment.
g_string_new_len expects a string pointer as the first argument, not a character. Use g_string_sized_new(128) or g_string_new("") instead.
| GString *rbuf = g_string_new_len('\0', 128); | |
| GString *rbuf = g_string_sized_new(128); |
|
b'## Copyright scan failure |
|
b'## Copyright scan failure |
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.
|
|
||
| GDIAL_STATIC gchar *gdial_rest_server_new_additional_data_url(guint listening_port, const gchar *app_name, gboolean encode , const gchar* app_uri) { | ||
| /* | ||
| * The specifciation of additionalDataUrl in form of /apps/<app_name>/dial_data |
There was a problem hiding this comment.
Typo in comment: "specifciation" should be "specification".
| * The specifciation of additionalDataUrl in form of /apps/<app_name>/dial_data | |
| * The specification of additionalDataUrl in form of /apps/<app_name>/dial_data |
| #ifndef HAVE_GSSDP_VERSION_1_6_OR_NEWER | ||
| NULL, | ||
| #endif | ||
| gdial_options_->iface_name, &error); |
There was a problem hiding this comment.
The preprocessor conditional checks for HAVE_GSSDP_VERSION_1_6_OR_NEWER but based on the CMakeLists.txt changes, when using libsoup-3.0, the code expects gssdp-1.6 (which requires different API parameters). The gssdp_client_new function signature changed between gssdp-1.2 and gssdp-1.6 - in gssdp-1.6, the first NULL parameter was removed. However, the file is located in libsoup-3p0 directory which should only be compiled when LIBSOUP3_FOUND AND GSSDP16_FOUND are both true (per CMakeLists.txt lines 37-42). The conditional compilation here could be confusing since this file shouldn't be used with gssdp versions prior to 1.6 when libsoup-3.0 is in use.
| g_socket_close(conn_context->read_gsocket, NULL);//this will trigger abort callback | ||
| } | ||
| GDIAL_LOGTRACE("Exiting ..."); | ||
| return G_SOURCE_REMOVE;; |
There was a problem hiding this comment.
Double semicolon at the end of the return statement. This is a syntax issue that should be corrected to a single semicolon.
| return G_SOURCE_REMOVE;; | |
| return G_SOURCE_REMOVE; |
| /* | ||
| * Do not support duplicate registration with different param | ||
| * | ||
| *@TODO: check params. If identical to previous registraiton, return TRUE; |
There was a problem hiding this comment.
Typo in comment: "registraiton" should be "registration".
| *@TODO: check params. If identical to previous registraiton, return TRUE; | |
| *@TODO: check params. If identical to previous registration, return TRUE; |
|
|
||
| /* | ||
| * Valid http paths (DIAL 2.1) | ||
| * |
There was a problem hiding this comment.
Typo in comment: "guarnteed" should be "guaranteed".
| /* | ||
| * Do not support duplicate registration with different param | ||
| * | ||
| *@TODO: check params. If identical to previous registraiton, return TRUE; |
There was a problem hiding this comment.
Typo in comment: "registraiton" should be "registration".
| if ((cached_payload != NULL) && (payload != NULL)) { | ||
| int changed = g_strcmp0(cached_payload, payload); | ||
| if (changed) { | ||
| GDIAL_LOGINFO("relaunch requred due to payload change [%s] vs [%s]", cached_payload, payload); |
There was a problem hiding this comment.
Typo in comment: "requred" should be "required".
| GDIAL_LOGINFO("relaunch requred due to payload change [%s] vs [%s]", cached_payload, payload); | |
| GDIAL_LOGINFO("relaunch required due to payload change [%s] vs [%s]", cached_payload, payload); |
| 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); |
There was a problem hiding this comment.
Mixed use of free() and g_free(). The variable 'additional_data_url_safe' is allocated with g_uri_escape_string() (which uses GLib's allocator), but is freed with free() instead of g_free(). This could lead to undefined behavior or memory corruption. Use g_free() for GLib-allocated memory.
| 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); |
There was a problem hiding this comment.
Memory leak: The string allocated by g_strdup_printf for 'dail_ssdp_handler' is never freed. This variable should be freed with g_free() after the soup_server_add_handler call.
| soup_server_add_handler(ssdp_http_server_, dail_ssdp_handler, ssdp_http_server_callback, NULL, NULL); | |
| soup_server_add_handler(ssdp_http_server_, dail_ssdp_handler, ssdp_http_server_callback, NULL, NULL); | |
| g_free(dail_ssdp_handler); | |
| dail_ssdp_handler = NULL; |
|
b'## Copyright scan failure |
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.
| GDIAL_LOGINFO("Inside gdial_rest_server_unregister_all_apps"); | ||
| GDialRestServerPrivate *priv = gdial_rest_server_get_instance_private(self); | ||
| GList *registered_apps_head = priv->registered_apps; | ||
| /*Stopping all registread Apps*/ |
There was a problem hiding this comment.
Spelling error in comment: 'registread' should be 'registered'.
| /*Stopping all registread Apps*/ | |
| /*Stopping all registered Apps*/ |
| * Minimum URI must be larger than "/apps/" | ||
| * URI must not end with '/' | ||
| * | ||
| * Default <instance> is "run", but this is not guarnteed |
There was a problem hiding this comment.
Spelling error in comment: 'guarnteed' should be 'guaranteed'.
| * Default <instance> is "run", but this is not guarnteed | |
| * Default <instance> is "run", but this is not guaranteed |
| /* | ||
| * Do not support duplicate registration with different param | ||
| * | ||
| *@TODO: check params. If identical to previous registraiton, return TRUE; |
There was a problem hiding this comment.
Spelling error in comment: 'registraiton' should be 'registration' (occurs twice in this comment block).
| /* | ||
| * Any request only respond to app name that is among registered apps | ||
| */ | ||
| g_signal_emit(gdial_rest_server, gdial_rest_server_signals[SIGNAL_INVALID_URI], 0, "URI containes unregistered app name"); |
There was a problem hiding this comment.
Spelling error in comment: 'containes' should be 'contains'.
| g_signal_emit(gdial_rest_server, gdial_rest_server_signals[SIGNAL_INVALID_URI], 0, "URI containes unregistered app name"); | |
| g_signal_emit(gdial_rest_server, gdial_rest_server_signals[SIGNAL_INVALID_URI], 0, "URI contains unregistered app name"); |
|
b'## Copyright scan failure |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (gdial_options_->friendly_name != NULL) | ||
| { | ||
| g_free(gdial_options_->friendly_name); | ||
| gdial_options_->friendly_name = NULL; | ||
| } |
There was a problem hiding this comment.
gdial_ssdp_new() may allocate defaults for multiple GDialOptions strings (e.g., manufacturer/model_name), but gdial_ssdp_destroy() only frees friendly_name here. Please ensure all option strings allocated by this module are freed (or document/avoid taking ownership of options fields).
| #include <string.h> | ||
| #include <stdio.h> | ||
| #include <netinet/in.h> | ||
| #include <sys/socket.h> |
There was a problem hiding this comment.
This file uses pthread_self() (e.g., in request callbacks) but does not include <pthread.h>. Please include it explicitly to avoid implicit declaration / portability issues.
| #include <sys/socket.h> | |
| #include <sys/socket.h> | |
| #include <pthread.h> |
| 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 dynamically allocated dail_ssdp_handler string is never freed after calling soup_server_add_handler(), which leaks memory. Also consider storing the handler path so gdial_ssdp_destroy() can remove the same handler path later.
| #include <stdio.h> | ||
| #include <glib.h> | ||
| #include <assert.h> | ||
| #include <libsoup/soup.h> |
There was a problem hiding this comment.
This file uses pthread_self() and usleep() but does not include <pthread.h> / <unistd.h>. Relying on indirect includes can break builds (and causes implicit-declaration warnings); include the headers directly where the APIs are used.
| #include <libsoup/soup.h> | |
| #include <libsoup/soup.h> | |
| #include <pthread.h> | |
| #include <unistd.h> |
|
|
||
| g_return_val_if_fail(ssdp_http_server != NULL, -1); | ||
| g_return_val_if_fail(options != NULL, -1); | ||
| g_return_val_if_fail(options->iface_name != NULL, -1); |
There was a problem hiding this comment.
options->iface_name is required by g_return_val_if_fail(), but later in this function you attempt to default gdial_options_->iface_name when it is NULL. As written, NULL can never reach the defaulting code; either set the default before the guard, or remove the defaulting logic.
| g_return_val_if_fail(options->iface_name != NULL, -1); |
| g_free(application_url_str); | ||
| application_url_str = NULL; | ||
|
|
||
| soup_server_message_set_response(msg, "text/xml; charset=utf-8", SOUP_MEMORY_STATIC, dd_xml_response_str_, dd_xml_response_str_len); |
There was a problem hiding this comment.
soup_server_message_set_response(..., SOUP_MEMORY_STATIC, dd_xml_response_str_, ...) requires dd_xml_response_str_ to remain valid for the lifetime of the response, but this module later frees/replaces dd_xml_response_str_ when friendly/manufacturer/model changes. This can lead to use-after-free during an in-flight response; consider SOUP_MEMORY_COPY (or otherwise deferring frees until the message is finished).
| soup_server_message_set_response(msg, "text/xml; charset=utf-8", SOUP_MEMORY_STATIC, dd_xml_response_str_, dd_xml_response_str_len); | |
| soup_server_message_set_response(msg, "text/xml; charset=utf-8", SOUP_MEMORY_COPY, dd_xml_response_str_, dd_xml_response_str_len); |
| 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.
g_uri_escape_string() returns memory that should be released with g_free(), but gdial_rest_server_handle_POST() releases additional_data_url_safe with free(). This can break if GLib is built with a different allocator. Use g_free(additional_data_url_safe) (and similarly prefer g_free for other GLib-allocated strings).
| 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 already statically initialized with PTHREAD_MUTEX_INITIALIZER, but gdial_ssdp_new() calls pthread_mutex_init() again. Re-initializing an already-initialized mutex is undefined; either remove the initializer or remove the explicit pthread_mutex_init()/pthread_mutex_destroy() pair and rely on static init.
| if (0 != pthread_mutex_init(&ssdpServerEventSync, NULL)) | |
| { | |
| GDIAL_LOGERROR("Failed to initializing mutex"); | |
| return EXIT_FAILURE; | |
| } |
3 participants
No description provided.