Created attachment 372850 [details] [review]
multiudpsink: Make sure that GstMemory references are valid until we unmap

It's actually much worse than that. Consider the following case: Thread A merges the memories of a buffer while mapping it completely, thread B iterates over all memories to do things with them separately. While iterating in B, thread A can finish, replace all memories and from that point onwards thread B will assert/crash/etc because there are no N memories anymore in the buffer.

The only safe solution to this seems to be to never replace the memories in buffers if they are not writable, and instead only store the merged memory in the GstMapInfo (plus a flag to unref it on unmap). This will mean that merging would happen every single time for such buffers, and even at the same time in multiple threads if we're unlucky. But at least it would be safe :)

Comments?

Good thing we're moving to an issue tracker that has emojis.

Interesting, actually we only replace the memories if the buffer is writable. So why does this fail at all then, even the original one.

>     if (writable) {
>      _replace_memory (buffer, len, idx, length, gst_memory_ref (nmem));

Needs some more debugging now.

(In reply to Tim-Philipp Müller from comment #3)
> Good thing we're moving to an issue tracker that has emojis.

Good that Bugzilla at least supports unicode so 

(In reply to Sebastian Dröge (slomo) from comment #4)

> (In reply to Tim-Philipp Müller from comment #3)
> > Good thing we're moving to an issue tracker that has emojis.
> 
> Good that Bugzilla at least supports unicode so

Except that Bugzilla apparently strips them off when storing the comment. Amazing!

Ok, new insights, it is all different again and broken elsewhere :)

So the problem is actually that gst_buffer_list_get() does not increase the reference count. A buffer can have its only reference inside the buffer list (-> it is writable) but the buffer list has a refcount > 1 (-> is not writable): example a tee is passing the same buffer list to two elements downstream.

Now gst_buffer_list_get() on that non-writable buffer list gives back a borrowed buffer, that is the buffer is still writable even if the list itself it not writable and shared in different places. Two threads can then at the same time get a buffer like that and map them, merge memories and break everything. Or otherwise modify buffers because it's considered writable after all but actually is not, however in those cases people would hopefully use gst_buffer_list_get_writable() which does not have the problem.
With mapping it's more hidden because a read-map seems like a read-only operation but is actually not.

So two options here:
1) We deprecate gst_buffer_list_get() and add a new gst_buffer_list_dup() that returns a new reference
2) We never ever replace memories when doing read-mapping in a buffer, even if the buffer was writable. This however does not solve the problem of the buffer being considered writable because of refcount==1
3) Always increase the refcount of buffers in the buffer list twice, but that needs some special care for gst_buffer_list_get_writable() and I'm not actually sure how to implement that properly in that case

Any other ideas?

4) Make buffers writable and set a new READONLY flag on them when they're a member of a bufferlist?

Created attachment 372860 [details] [review]
bufferlist: Add test to illustrate #796692

(In reply to Jan Schmidt from comment #7)
> 4) Make buffers writable and set a new READONLY flag on them when they're a
> member of a bufferlist?

That would interfere with how gst_buffer_list_get_writable() works

I think the only way things can works is with the following:
  - A buffer is writable if all memory are writable
  - A buffer list is writable if all GstBuffer is writable

Merging/Replacing/Adding/Removing memory is unsafe it's not the case. Buffers and BufferLists must have a way to stay immutable, otherwise you also break the ability to use tee element.

(In reply to Nicolas Dufresne (ndufresne) from comment #10)
> I think the only way things can works is with the following:
>   - A buffer is writable if all memory are writable
>   - A buffer list is writable if all GstBuffer is writable

This seems the wrong way around. A buffer is writable independent of the writability of its memories (you can change the PTS even if all memories are read-only!). And a buffer list is writable even if all buffers are read-only (you can add new read-only buffers to it).

I'm not sure what your point is :) You mean that buffers should only be writable if their parent (the buffer list) is writable and the buffer itself has a refcount of 1?

> Merging/Replacing/Adding/Removing memory is unsafe it's not the case.
> Buffers and BufferLists must have a way to stay immutable, otherwise you
> also break the ability to use tee element.

I don't understand what you mean here. But tee is exactly the case where I ran into this problem.

The problem is simply that buffers in a buffer list are writable although the list itself is not writable. And you can get a reference with refcount==1 from gst_buffer_list_get() in that case. While you are not supposed to modify it in any way, other immutable operations (buffer mapping!) can modify the buffer as an optimization and then all falls apart.

Good point, that's right. Seems like a gap in the API. What about taking a ref on all the buffer list buffers in tee, before iterating and pushing ?

Nop, I've looked, that won't work, there could be a queue downstream. So maybe what we need is for gst_buffer_list_ref() to ref each GstBuffer. That would create the condition that if GstBufferList is not writable due to 2 ref or more, then GstBuffer are not writable either ?

Created attachment 372865 [details] [review]
bufferlist: Ensure buffer and buffer list writability matches

This makes the gst_buffer_list_ref/unref non inline function that will
now ref all the internal buffers. This way, when the buffer list becomes
non-writable, same happens for the buffers inside.

A quick attempt, which passes the new test_multiple_mutable_buffer_references(). It makes gst_buffer_list_ref/unref more expensive though, but is overall safe. Note that I didn't update the Win def files.

About test_merge_mapped_memory(), I think you must be owner of the GstBuffer, so it's avoidable in general. But I might be missing something.

Comment on attachment 372865 [details] [review]
bufferlist: Ensure buffer and buffer list writability matches

Thanks for looking into this, but this is unfortunately API/ABI breakage. It was also one of the things that I thought of, however:

a) existing binaries will only call gst_mini_object_ref() due to it being an inline function before
b) it's perfectly valid to call gst_mini_object_ref/unref() on buffer lists, and code does that. There are code paths that handle buffers and buffer lists the same way as a generic mini object

(In reply to Nicolas Dufresne (ndufresne) from comment #16)
> About test_merge_mapped_memory(), I think you must be owner of the
> GstBuffer, so it's avoidable in general. But I might be missing something.

Ignore that one, the test is wrong and that all works correctly :) It was on the way of finding the actual bug, which is in GstBufferList

Yeah, I don't think MiniObject already have any mechanism to implement this solution. Do you think it's worth it, or does a solution where buffer_list_get users needs to be aware of writability works fine enough ?

(In reply to Nicolas Dufresne (ndufresne) from comment #19)
> Yeah, I don't think MiniObject already have any mechanism to implement this
> solution. Do you think it's worth it, or does a solution where
> buffer_list_get users needs to be aware of writability works fine enough ?

That's not enough unfortunately as explained above (and it already is part of the gst_buffer_list_get_buffer() API that the return value is not writable, that's why get_writable_buffer() exists). The problem is that even if *you* don't do any operation on the buffer that requires writability, some other code outside your control might. Like in my case read-mapping the buffer, which merges and replaces all memories inside the buffer if the buffer happens to be writable.

A possible solution is to never replace the memories of course, but that does not prevent other accidental modifications like this elsewhere.

I think the only possible solutions are
1) Deprecating get_buffer() and add a new dup_buffer() that returns a new reference
2) Find a backwards compatible way of coupling the "parent" (buffer list) refcount with the writability of the buffer


And I just noticed that the same problem does not affect only buffer lists but also GstSample. Exactly same problem really. Aaaand GstBuffer with the contained GstMemories, see following testcase (buffer could be in two places at once, memory refcount==1, both places could modify the memory via e.g. gst_memory_resize() or both map the memory writable and modify them together).


I think we need a general solution for extending miniobject writability so that miniobjects owned by another one are only writable if the parent refcount is 1 and their own is too. I'll experiment with that.

Created attachment 372875 [details] [review]
buffer: Test-case to show mutability problem of memories in buffers

(In reply to Sebastian Dröge (slomo) from comment #20)

> I think we need a general solution for extending miniobject writability so
> that miniobjects owned by another one are only writable if the parent
> refcount is 1 and their own is too. I'll experiment with that.

GstMiniObject having no padding is not exactly helpful for finding a solution that is backwards compatible :)

Ok I'm not really sure how to best implement tracking of the parents of a miniobject. There is simply no sane way to store anything new in miniobjects due to a) miniobjects being allocated by the subclass (so we can't overallocate in gstminiobject.c), b) no padding in GstMiniObject, c) qdata allowing storing of additional data but it involves more allocations and a *global* mutex.

If someone has any ideas that do not involve going from libgstreamer-1.0.so.0 to .so.1 that would be great :) This is really a serious problem

I didn't want to propose it, but we are pretty stuck. We could steal qdata pointer, allocate an array of two.

  qdata = g_new (GstQData, 2);

Use qdata[0].data as out private pointer, and qdata[1].data as our qdata storage. Or something along these lines ... not great. From there, we could add a new function callback (ref callback), or implement your parenting design, which seems more lightweight.

(In reply to Nicolas Dufresne (ndufresne) from comment #24)
> I didn't want to propose it, but we are pretty stuck. We could steal qdata
> pointer, allocate an array of two.
> 
>   qdata = g_new (GstQData, 2);

Yeah we can basically do anything we want with the qdata pointer and qdata length integer, they're only internally used.

I was thinking of always allocating something (parent tracking and the actual qdata storage) into the pointer but that would mean that every miniobject is now going to be two allocations. Far from ideal but probably the best of all options at this point.

Note that the pointer is a plain gpointer, we can store a struct there that directly stores the parents and then the qdata. No need to keep the current qdata struct there, it's internal. I guess that's the only good news :)

> Use qdata[0].data as out private pointer, and qdata[1].data as our qdata
> storage. Or something along these lines ... not great. From there, we could
> add a new function callback (ref callback)

Similar to what you did in your bufferlist patch so that bufferlist can override that reffing also refs each buffer inside it? That's quite heavy and I would prefer to not do that :)

> or implement your parenting design, which seems more lightweight.

I have a half-finished patch of that but then all the un-extensibleness of GstMiniObject frustrated me too much to continue that this week. I'll take a look at finishing that next week by adding another allocation to each miniobject as outlined above.

(In reply to Sebastian Dröge (slomo) from comment #25)

> Yeah we can basically do anything we want with the qdata pointer and qdata
> length integer, they're only internally used.

And with that I found a solution that does not require allocations in the majority of cases and is even lock-free.

Created attachment 372939 [details] [review]
miniobject: Add parent pointers to the miniobject to influence writability

Every container of miniobjects now needs to store itself as parent in
the child object, and remove itself again at a later time.

A miniobject is only writable if there is at most one parent, and that
parent is writable itself, and if the reference count of the miniobject
is 1.

GstBuffer (for memories), GstBufferList (for buffers) and GstSample (for
caps, buffer, bufferlist) was updated accordingly.

Without this it was possible to have e.g. a bufferlist with refcount 2
in two places, modifying the same buffer with refcount 1 at the same
time.

Created attachment 372940 [details] [review]
bufferlist: Add test to ensure that buffers in an non-writable list are not writable

Created attachment 372941 [details] [review]
buffer: Add test to ensure that memories in a non-writable buffer are not writable

This currently breaks the rtpbasepayload test. There are buffers there that are not writable anymore after this change but expected to be writable. Investigating tomorrow

Created attachment 372947 [details] [review]
miniobject: Add parent pointers to the miniobject to influence writability

Every container of miniobjects now needs to store itself as parent in
the child object, and remove itself again at a later time.

A miniobject is only writable if there is at most one parent, and that
parent is writable itself, and if the reference count of the miniobject
is 1.

GstBuffer (for memories), GstBufferList (for buffers) and GstSample (for
caps, buffer, bufferlist) was updated accordingly.

Without this it was possible to have e.g. a bufferlist with refcount 2
in two places, modifying the same buffer with refcount 1 at the same
time.

Created attachment 372948 [details] [review]
bufferlist: Add test to ensure that buffers in an non-writable list are not writable

Created attachment 372949 [details] [review]
buffer: Add test to ensure that memories in a non-writable buffer are not writable

Created attachment 372952 [details] [review]
bufferlist: Prevent gst_buffer_list_foreach() from modifying non-writeable lists

Previously gst_buffer_list_foreach() could modify (drop or replace)
buffers in non-writable lists, which could cause all kinds of problems
if other code also has a reference to the list and assumes that it stays
the same.

(In reply to Sebastian Dröge (slomo) from comment #34)
> Created attachment 372952 [details] [review] [review]
> bufferlist: Prevent gst_buffer_list_foreach() from modifying non-writeable
> lists
> 
> Previously gst_buffer_list_foreach() could modify (drop or replace)
> buffers in non-writable lists, which could cause all kinds of problems
> if other code also has a reference to the list and assumes that it stays
> the same.

This is a different bug from everything else in this ticket and also existed since forever, but on top of the other changes it is easier to fix (because we already have another reference to buffers in non-writable lists anyway and don't have to save them from the callback destroying them).

With this, all tests in core/base/good/rtsp-server are passing btw, it's probably good to go but I'd like to have a review first. The changes are not exactly trivial.

Review of attachment 372947 [details] [review]:

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)

Review of attachment 372948 [details] [review]:

Straightforward

Review of attachment 372949 [details] [review]:

Straightforward too

Review of attachment 372952 [details] [review]:

Looks good

::: gst/gstbufferlist.c
@@ +271,3 @@
      */
     was_writable = gst_buffer_is_writable (buf);
+    if (was_writable && list_was_writable)

Ah, this implements one of the comments I made on the initial patch

Created attachment 372975 [details] [review]
miniobject: Add parent pointers to the miniobject to influence writability

Every container of miniobjects now needs to store itself as parent in
the child object, and remove itself again at a later time.

A miniobject is only writable if there is at most one parent, and that
parent is writable itself, and if the reference count of the miniobject
is 1.

GstBuffer (for memories), GstBufferList (for buffers) and GstSample (for
caps, buffer, bufferlist) was updated accordingly.

Without this it was possible to have e.g. a bufferlist with refcount 2
in two places, modifying the same buffer with refcount 1 at the same
time.

Created attachment 372976 [details] [review]
bufferlist: Add test to ensure that buffers in an non-writable list are not writable

Created attachment 372977 [details] [review]
buffer: Add test to ensure that memories in a non-writable buffer are not writable

Created attachment 372978 [details] [review]
bufferlist: Prevent gst_buffer_list_foreach() from modifying non-writeable lists

Previously gst_buffer_list_foreach() could modify (drop or replace)
buffers in non-writable lists, which could cause all kinds of problems
if other code also has a reference to the list and assumes that it stays
the same.

Attachment 372975 [details] pushed as 6fa3514 - miniobject: Add parent pointers to the miniobject to influence writability
Attachment 372976 [details] pushed as 13a45c0 - bufferlist: Add test to ensure that buffers in an non-writable list are not writable
Attachment 372977 [details] pushed as 111faa5 - buffer: Add test to ensure that memories in a non-writable buffer are not writable
Attachment 372978 [details] pushed as cb51bd6 - bufferlist: Prevent gst_buffer_list_foreach() from modifying non-writeable lists

Created attachment 373011 [details] [review]
v4l2bufferpool: Remove duplicate check

We were calling gst_v4l2_is_buffer_valid() before and inside
gst_v4l2_buffer_pool_qbuf() as we needed to access the group. The second
check failed since the writability of the buffer get inherited from the
GstMemory, which lead to pipeline failure. As we cannot avoid the extra
ref, it would be racy otherwise, just pass the group to _dbuf() so it
does not have to call gst_v4l2_is_buffer_valid() again.

Comment on attachment 373011 [details] [review]
v4l2bufferpool: Remove duplicate check

Attachment 373011 [details] pushed as 38b68cb - v4l2bufferpool: Remove duplicate check

(In reply to Nicolas Dufresne (ndufresne) from comment #47)
> Comment on attachment 373011 [details] [review] [review]
> v4l2bufferpool: Remove duplicate check
> 
> Attachment 373011 [details] pushed as 38b68cb - v4l2bufferpool: Remove
> duplicate check

Attached here, just so that we are aware of the kind of regression this may cause.