[Bug 796692] sample/bufferlist/buffer: Non-writable container miniobjects allow writable access to their contents, causing memory problems

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=796692

--- Comment #37 from Jan Schmidt <thaytan at noraisin.net> ---
Review of attachment 372947:
 --> (https://bugzilla.gnome.org/review?bug=796692&attachment=372947)

A few comments / re-wording of things

::: gst/gstbufferlist.c
@@ +267,3 @@
+     * by removing ourselves as parent
+     */
+     *

This is potentially more expensive than needed if the buffer_list is already
not writable - in which case we could skip the check on every buffer and just
ref them

::: gst/gstminiobject.c
@@ +76,3 @@
+ * guint and gpointer in the GstMiniObject struct in
+ * a rather complicated way to store the parent(s) and qdata.
+ * Originally the were just the number of qdatas and the qdata.

Probably add a FIXME 2.0 around here so it gets removed one day

@@ +86,3 @@
+ *
+ * Unless we're in state 3, we always have to move to Locking state
+ * states:

atomatically -> atomically

@@ +157,3 @@
   mini_object->free = free_func;

+  mini_object->priv_uint = PRIV_DATA_STATE_NO_PARENT;

This needs to use g_atomic_int_set() to make sure the value is written out to
memory.

@@ +952,3 @@
+ * writability of @object: if the @parent is not writable, @object is also not
+ * writable, regardless of its refcount. @object is only writable if all
+ * @parent: a parent #GstMiniObject

"This adds @parent as a parent for @object. Having one ore more parents affects
the
writability of @object: if a @parent is not writable, @object is also not
writable, regardless of its refcount. @object is only writable if all
the parents are writable and its own refcount is exactly 1."

@@ +981,3 @@
+     *
+     * Note that in this case we did not store PRIV_DATA_STATE_LOCKED! */
+gst_mini_object_add_parent (GstMiniObject * object, GstMiniObject * parent)

This code repeats a few times - maybe factor it out? Also the comment 'Have
full struct now...' is confusing here.

@@ +993,3 @@
+
+  /* Now we either have to add to the full struct, or add our one and only
+

This comment is also misleading - the full struct was already added a few lines
above

@@ +1054,3 @@
+     * from the beginning
+     *
+    while (!g_atomic_int_compare_and_exchange (&priv_data->parent_lock, 0,
1));

Same comments about comments as for add_parent() here.

@@ +1090,3 @@
+
+    /* Unlock again */
+

Probably should restore the original state if the parent passed to
remove_parent() didn't match the only parent, rather than setting it to
NO_PARENT.

::: gst/gstsample.c
@@ +281,1 @@
   old = sample->buffer_list;

Worth a shortcut check if old == buffer_list here? (and for buffers and caps
below)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.