libgstd: fix race conditions in pipeline list and session property access

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);
 
@@ -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);
@@ -327,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);
 
@@ -369,7 +372,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));

libgstd/gstd_list.h

@@ -52,8 +52,6 @@ struct _GstdList
 {
   GstdObject parent;
 
-  guint count;
-
   GType node_type;
 
   GParamFlags flags;
@@ -68,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);
 

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;

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:

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
 

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... */