From 05a4ca74642783fc1482c96d116fcb1b90cd545d Mon Sep 17 00:00:00 2001 From: Rico Wang Date: Thu, 14 May 2026 09:49:29 +1000 Subject: [PATCH 1/5] counting at the time of use with a lock --- libgstd/gstd_list.c | 19 +++++++++---------- libgstd/gstd_list.h | 2 -- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/libgstd/gstd_list.c b/libgstd/gstd_list.c index 2664b17e..5e06f60f 100644 --- a/libgstd/gstd_list.c +++ b/libgstd/gstd_list.c @@ -37,7 +37,6 @@ enum N_PROPERTIES // NOT A PROPERTY }; -#define GSTD_LIST_DEFAULT_COUNT 0 #define GSTD_LIST_DEFAULT_NODE_TYPE G_TYPE_NONE #define GSTD_LIST_DEFAULT_FLAGS GSTD_PARAM_READ | GSTD_PARAM_CREATE | GSTD_PARAM_DELETE @@ -80,7 +79,7 @@ gstd_list_class_init (GstdListClass * klass) g_param_spec_uint ("count", "Count", "The amount of nodes in the list", - 0, G_MAXINT, GSTD_LIST_DEFAULT_COUNT, G_PARAM_READABLE | GSTD_PARAM_READ); + 0, G_MAXINT, 0, G_PARAM_READABLE | GSTD_PARAM_READ); properties[PROP_NODE_TYPE] = g_param_spec_gtype ("node-type", @@ -114,7 +113,6 @@ gstd_list_init (GstdList * self) { GST_INFO_OBJECT (self, "Initializing list"); self->list = NULL; - self->count = GSTD_LIST_DEFAULT_COUNT; self->node_type = GSTD_LIST_DEFAULT_NODE_TYPE; } @@ -143,9 +141,15 @@ gstd_list_get_property (GObject * object, switch (property_id) { case PROP_COUNT: - GST_DEBUG_OBJECT (self, "Returning count of %u", self->count); - g_value_set_uint (value, self->count); + { + guint count; + GST_OBJECT_LOCK (self); + count = g_list_length (self->list); + GST_OBJECT_UNLOCK (self); + GST_DEBUG_OBJECT (self, "Returning count of %u", count); + g_value_set_uint (value, count); break; + } case PROP_NODE_TYPE: GST_DEBUG_OBJECT (self, "Returning type %s", g_type_name (self->node_type)); @@ -218,8 +222,6 @@ gstd_list_create (GstdObject * object, const gchar * name, goto error; } - self->count++; - if (!gstd_list_append_child (self, out)) { g_object_unref (out); ret = GSTD_EXISTING_RESOURCE; @@ -274,8 +276,6 @@ gstd_list_delete (GstdObject * object, const gchar * node) return ret; } - self->count--; - self->list = g_list_delete_link (self->list, found); GST_OBJECT_UNLOCK (self); @@ -369,7 +369,6 @@ gstd_list_append_child (GstdList * self, GstdObject * child) } self->list = g_list_append (self->list, child); - self->count = g_list_length (self->list); GST_OBJECT_UNLOCK (self); GST_INFO_OBJECT (self, "Appended %s to %s list", GSTD_OBJECT_NAME (child), GSTD_OBJECT_NAME (self)); diff --git a/libgstd/gstd_list.h b/libgstd/gstd_list.h index bc2f2ba6..11005419 100644 --- a/libgstd/gstd_list.h +++ b/libgstd/gstd_list.h @@ -52,8 +52,6 @@ struct _GstdList { GstdObject parent; - guint count; - GType node_type; GParamFlags flags; From 2e391cfe9a3cdcd5d9ceca12add8e29d582ceec4 Mon Sep 17 00:00:00 2001 From: Rico Wang Date: Thu, 14 May 2026 10:44:30 +1000 Subject: [PATCH 2/5] lock shared linked access --- libgstd/gstd_list.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/libgstd/gstd_list.c b/libgstd/gstd_list.c index 5e06f60f..c12e2c8e 100644 --- a/libgstd/gstd_list.c +++ b/libgstd/gstd_list.c @@ -302,14 +302,19 @@ gstd_list_to_string (GstdObject * object, gchar ** outstring) g_return_val_if_fail (GSTD_IS_OBJECT (object), GSTD_NULL_ARGUMENT); g_warn_if_fail (!*outstring); - /* Lets leverage the parent's class implementation */ + /* Parent to_string must run unlocked: it re-enters PROP_COUNT which + * takes GST_OBJECT_LOCK (non-recursive). */ GSTD_OBJECT_CLASS (gstd_list_parent_class)->to_string (GSTD_OBJECT (object), &props); // A little hack to remove the last bracket props[strlen (props) - 2] = '\0'; - list = self->list; acc = g_strdup (""); + + /* Lock the walk so concurrent gstd_list_delete cannot free a link or unref + * a child mid-iteration. */ + GST_OBJECT_LOCK (self); + list = self->list; while (list) { separator = list->next ? "," : ""; node = @@ -319,6 +324,7 @@ gstd_list_to_string (GstdObject * object, gchar ** outstring) acc = node; list = list->next; } + GST_OBJECT_UNLOCK (self); *outstring = g_strdup_printf ("%s,\n \"nodes\" : [%s]\n}", props, acc); g_free (props); From 75a05f04b4fed2c45a0d474112586ff2bbb22c9b Mon Sep 17 00:00:00 2001 From: Rico Wang Date: Thu, 14 May 2026 11:15:02 +1000 Subject: [PATCH 3/5] fix UAF in find_child by returning owned ref --- libgstd/gstd_list.c | 9 +++------ libgstd/gstd_list.h | 1 + libgstd/gstd_list_reader.c | 3 ++- libgstd/gstd_parser.c | 8 ++++++-- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/libgstd/gstd_list.c b/libgstd/gstd_list.c index c12e2c8e..a6f78044 100644 --- a/libgstd/gstd_list.c +++ b/libgstd/gstd_list.c @@ -333,23 +333,20 @@ gstd_list_to_string (GstdObject * object, gchar ** outstring) return GSTD_EOK; } +/* Returns a new reference; caller must g_object_unref when done. */ GstdObject * gstd_list_find_child (GstdList * self, const gchar * name) { GList *result; - GstdObject *child; + GstdObject *child = NULL; g_return_val_if_fail (self, NULL); g_return_val_if_fail (name, NULL); GST_OBJECT_LOCK (self); result = g_list_find_custom (self->list, name, gstd_list_find_node); - - if (result) { - child = GSTD_OBJECT (result->data); - } else { - child = NULL; + child = GSTD_OBJECT (g_object_ref (result->data)); } GST_OBJECT_UNLOCK (self); diff --git a/libgstd/gstd_list.h b/libgstd/gstd_list.h index 11005419..a810b6bf 100644 --- a/libgstd/gstd_list.h +++ b/libgstd/gstd_list.h @@ -66,6 +66,7 @@ struct _GstdListClass GType gstd_list_get_type (void); +/* Returns a new reference; caller must g_object_unref when done. */ GstdObject *gstd_list_find_child (GstdList * self, const gchar * name); gboolean gstd_list_append_child (GstdList *, GstdObject * child); diff --git a/libgstd/gstd_list_reader.c b/libgstd/gstd_list_reader.c index 2ccb3056..0cb43f4c 100644 --- a/libgstd/gstd_list_reader.c +++ b/libgstd/gstd_list_reader.c @@ -155,9 +155,10 @@ gstd_list_reader_read_child (GstdIReader * iface, g_return_val_if_fail (name, GSTD_NULL_ARGUMENT); g_return_val_if_fail (out, GSTD_NULL_ARGUMENT); + /* gstd_list_find_child returns an owned ref; transfer it to *out. */ found = gstd_list_find_child (GSTD_LIST (object), name); if (found) { - *out = GSTD_OBJECT (g_object_ref (found)); + *out = GSTD_OBJECT (found); ret = GSTD_EOK; } else { *out = NULL; diff --git a/libgstd/gstd_parser.c b/libgstd/gstd_parser.c index 821984ee..dcb42943 100644 --- a/libgstd/gstd_parser.c +++ b/libgstd/gstd_parser.c @@ -1007,7 +1007,7 @@ gstd_parser_pipeline_create_ref (GstdSession * session, gchar * action, GST_OBJECT_LOCK (session); - /* Look for the pipeline node */ + /* Look for the pipeline node (owned ref, must unref) */ pipeline_node = gstd_list_find_child (GSTD_LIST (pipeline_list_node), tokens[0]); @@ -1032,6 +1032,8 @@ gstd_parser_pipeline_create_ref (GstdSession * session, gchar * action, ret = gstd_pipeline_increment_refcount (GSTD_PIPELINE (pipeline_node)); create_error: + if (pipeline_node) + g_object_unref (pipeline_node); GST_OBJECT_UNLOCK (session); gst_object_unref (pipeline_list_node); pipeline_list_node_error: @@ -1061,7 +1063,7 @@ gstd_parser_pipeline_delete_ref (GstdSession * session, gchar * action, GST_OBJECT_LOCK (session); - /* Look for the pipeline node */ + /* Look for the pipeline node (owned ref, must unref) */ pipeline_node = gstd_list_find_child (GSTD_LIST (pipeline_list_node), args); if (!pipeline_node) { ret = GSTD_NO_PIPELINE; @@ -1076,6 +1078,8 @@ gstd_parser_pipeline_delete_ref (GstdSession * session, gchar * action, } pipeline_node_error: + if (pipeline_node) + g_object_unref (pipeline_node); GST_OBJECT_UNLOCK (session); gst_object_unref (pipeline_list_node); pipeline_list_node_error: From edc96edc1e652a84f9ae3f0d6765210328cc0fa4 Mon Sep 17 00:00:00 2001 From: Rico Wang Date: Thu, 14 May 2026 11:39:03 +1000 Subject: [PATCH 4/5] lock racy property accessors --- libgstd/gstd_pipeline.c | 86 +++++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 28 deletions(-) diff --git a/libgstd/gstd_pipeline.c b/libgstd/gstd_pipeline.c index 97c00c33..5d14e666 100644 --- a/libgstd/gstd_pipeline.c +++ b/libgstd/gstd_pipeline.c @@ -346,9 +346,13 @@ gstd_pipeline_get_property (GObject * object, switch (property_id) { case PROP_DESCRIPTION: + /* g_value_set_string deep-copies under the lock; once we unlock the + * caller owns its own copy. */ + GST_OBJECT_LOCK (self); GST_DEBUG_OBJECT (self, "Returning description of \"%s\"", self->description); g_value_set_string (value, self->description); + GST_OBJECT_UNLOCK (self); break; case PROP_ELEMENTS: GST_DEBUG_OBJECT (self, "Returning element list %p", self->elements); @@ -359,8 +363,12 @@ gstd_pipeline_get_property (GObject * object, g_value_set_object (value, self->pipeline_bus); break; case PROP_STATE: + /* Lock the read; g_value_set_object refs the result so caller is safe + * after we unlock. Pairs with locked swap in set_property. */ + GST_OBJECT_LOCK (self); GST_DEBUG_OBJECT (self, "Returning pipeline state %p", self->state); g_value_set_object (value, self->state); + GST_OBJECT_UNLOCK (self); break; case PROP_EVENT: GST_DEBUG_OBJECT (self, "Returning event handler %p", @@ -376,10 +384,15 @@ gstd_pipeline_get_property (GObject * object, break; case PROP_VERBOSE: - GST_DEBUG_OBJECT (self, "Returning verbose handler %lu", - self->deep_notify_id); - g_value_set_boolean (value, 0 != self->deep_notify_id); + { + gulong id; + GST_OBJECT_LOCK (self); + id = self->deep_notify_id; + GST_OBJECT_UNLOCK (self); + GST_DEBUG_OBJECT (self, "Returning verbose handler %lu", id); + g_value_set_boolean (value, 0 != id); break; + } case PROP_REFCOUNT: GST_OBJECT_LOCK (self); @@ -389,27 +402,25 @@ gstd_pipeline_get_property (GObject * object, break; case PROP_POSITION: - if (!gst_element_query_position (self->pipeline, GST_FORMAT_TIME, - &self->position)) { - /* if the query could not be performed. return 0 */ - self->position = G_GINT64_CONSTANT (0); - } - + { + /* Use a stack-local; do not write back into self->position to avoid + * torn writes between concurrent readers. */ + gint64 pos = G_GINT64_CONSTANT (0); + gst_element_query_position (self->pipeline, GST_FORMAT_TIME, &pos); GST_DEBUG_OBJECT (self, "Returning pipeline position %" GST_TIME_FORMAT, - GST_TIME_ARGS (self->position)); - g_value_set_int64 (value, self->position); + GST_TIME_ARGS (pos)); + g_value_set_int64 (value, pos); break; + } case PROP_DURATION: - if (!gst_element_query_duration (self->pipeline, GST_FORMAT_TIME, - &self->duration)) { - /* if the query could not be performed. return 0 */ - self->duration = G_GINT64_CONSTANT (0); - } - + { + gint64 dur = G_GINT64_CONSTANT (0); + gst_element_query_duration (self->pipeline, GST_FORMAT_TIME, &dur); GST_DEBUG_OBJECT (self, "Returning pipeline duration %" GST_TIME_FORMAT, - GST_TIME_ARGS (self->duration)); - g_value_set_int64 (value, self->duration); + GST_TIME_ARGS (dur)); + g_value_set_int64 (value, dur); break; + } default: /* We don't have any other property... */ G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); @@ -428,24 +439,42 @@ gstd_pipeline_set_property (GObject * object, switch (property_id) { case PROP_DESCRIPTION: - if (self->description) - g_free (self->description); - self->description = g_value_dup_string (value); - GST_INFO_OBJECT (self, "Changed description to \"%s\"", - self->description); + { + /* Same pattern as PROP_STATE: swap under lock, free old outside. */ + gchar *old; + gchar *new_desc = g_value_dup_string (value); + GST_OBJECT_LOCK (self); + old = self->description; + self->description = new_desc; + GST_OBJECT_UNLOCK (self); + g_free (old); + GST_INFO_OBJECT (self, "Changed description to \"%s\"", new_desc); break; + } case PROP_STATE: - if (self->state) { - g_object_unref (self->state); - } - self->state = g_value_get_object (value); + { + /* Swap under lock; use dup_object (takes a ref) — get_object would + * borrow and leave self->state dangling once the GValue is unset. + * Unref the old state outside the lock so its finalizer never runs + * under our object lock. */ + GstdState *old; + GST_OBJECT_LOCK (self); + old = self->state; + self->state = g_value_dup_object (value); + GST_OBJECT_UNLOCK (self); + if (old) + g_object_unref (old); break; + } #if GST_VERSION_MINOR >= 10 case PROP_VERBOSE: verbose = g_value_get_boolean (value); + /* Lock the read-modify-write of deep_notify_id against concurrent + * setters and the verbose getter. */ + GST_OBJECT_LOCK (self); if (verbose == FALSE && self->deep_notify_id != 0) { g_signal_handler_disconnect (self->pipeline, self->deep_notify_id); self->deep_notify_id = 0; @@ -455,6 +484,7 @@ gstd_pipeline_set_property (GObject * object, gst_element_add_property_deep_notify_watch (self->pipeline, NULL, TRUE); } + GST_OBJECT_UNLOCK (self); break; #endif From 31576137430d6e0b9bdc5be0952d14dc2a6891fd Mon Sep 17 00:00:00 2001 From: Rico Wang Date: Thu, 14 May 2026 11:44:26 +1000 Subject: [PATCH 5/5] gstd_session: lock PROP_PIPELINES/PROP_DEBUG access; unref old on swap --- libgstd/gstd_session.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/libgstd/gstd_session.c b/libgstd/gstd_session.c index 3412fa89..d39abb83 100644 --- a/libgstd/gstd_session.c +++ b/libgstd/gstd_session.c @@ -165,16 +165,21 @@ gstd_session_get_property (GObject * object, switch (property_id) { case PROP_PIPELINES: + /* g_value_set_object refs the result; safe after unlock. */ + GST_OBJECT_LOCK (self); GST_DEBUG_OBJECT (self, "Returning pipeline list %p", self->pipelines); g_value_set_object (value, self->pipelines); + GST_OBJECT_UNLOCK (self); break; case PROP_PID: GST_DEBUG_OBJECT (self, "Returning pid %d", self->pid); g_value_set_int (value, self->pid); break; case PROP_DEBUG: + GST_OBJECT_LOCK (self); GST_DEBUG_OBJECT (self, "Returning debug object %p", self->debug); g_value_set_object (value, self->debug); + GST_OBJECT_UNLOCK (self); break; default: @@ -192,13 +197,33 @@ gstd_session_set_property (GObject * object, switch (property_id) { case PROP_PIPELINES: - self->pipelines = g_value_dup_object (value); - GST_INFO_OBJECT (self, "Changed pipeline list to %p", self->pipelines); + { + /* Swap under lock; unref the old outside the lock so its finalizer + * never runs under our object lock. */ + GstdList *old; + GstdList *new_list = g_value_dup_object (value); + GST_OBJECT_LOCK (self); + old = self->pipelines; + self->pipelines = new_list; + GST_OBJECT_UNLOCK (self); + if (old) + g_object_unref (old); + GST_INFO_OBJECT (self, "Changed pipeline list to %p", new_list); break; + } case PROP_DEBUG: - self->debug = g_value_dup_object (value); - GST_DEBUG_OBJECT (self, "Changing debug object to %p", self->debug); + { + GstdDebug *old; + GstdDebug *new_debug = g_value_dup_object (value); + GST_OBJECT_LOCK (self); + old = self->debug; + self->debug = new_debug; + GST_OBJECT_UNLOCK (self); + if (old) + g_object_unref (old); + GST_DEBUG_OBJECT (self, "Changing debug object to %p", new_debug); break; + } default: /* We don't have any other property... */