GNOME Bugzilla – Bug 732445
aggregator: Many invalid memory access to destroyed GSources in the unit test
Last modified: 2014-07-04 12:57:50 UTC
The problem here is that the main context will destroy the sources itself already. And in remove_all_sources() we don't prevent the main context from running, so it could do that while we destroy sources. Solution would be to use g_idle_new(), store a ref to them somewhere and then destroy and unref them. ==25016== Invalid read of size 4 ==25016== at 0x5D4C569: g_source_destroy_internal (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==25016== by 0x4E378B7: _remove_all_sources (gstaggregator.c:387) ==25016== by 0x4E380A4: _sink_event (gstaggregator.c:468) ==25016== by 0x581F60E: gst_pad_send_event_unchecked (gstpad.c:5135) ==25016== by 0x581EE4A: gst_pad_push_event_unchecked (gstpad.c:4833) ==25016== by 0x581EB36: gst_pad_push_event (gstpad.c:4958) ==25016== by 0x5067D10: gst_base_src_perform_seek (gstbasesrc.c:1603) ==25016== by 0x506B496: gst_base_src_event (gstbasesrc.c:2031) ==25016== by 0x581F60E: gst_pad_send_event_unchecked (gstpad.c:5135) ==25016== by 0x5819E29: gst_pad_send_event (gstpad.c:5292) ==25016== by 0x4E38D2E: event_forward_func (gstaggregator.c:821) ==25016== by 0x581A7B7: gst_pad_forward (gstpad.c:2790) ==25016== by 0x4E3887B: _src_event (gstaggregator.c:870) ==25016== by 0x581F60E: gst_pad_send_event_unchecked (gstpad.c:5135) ==25016== by 0x581EE4A: gst_pad_push_event_unchecked (gstpad.c:4833) ==25016== by 0x581EB36: gst_pad_push_event (gstpad.c:4958) ==25016== by 0x5063186: gst_base_sink_send_event (gstbasesink.c:4369) ==25016== by 0x58017EC: gst_element_send_event (gstelement.c:1560) ==25016== by 0x57E317C: gst_bin_send_event (gstbin.c:2849) ==25016== by 0x58017EC: gst_element_send_event (gstelement.c:1560) ==25016== Address 0x87f760c is 44 bytes inside a block of size 96 free'd ==25016== at 0x4C29730: free (vg_replace_malloc.c:468) ==25016== by 0x5D4C2CD: g_source_unref_internal (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==25016== by 0x5D4EE3F: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==25016== by 0x5D4F047: g_main_context_iterate.isra.24 (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==25016== by 0x5D4F0EB: g_main_context_iteration (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==25016== by 0x5847746: gst_task_func (gsttask.c:317) ==25016== by 0x5D7489B: g_thread_pool_thread_proxy (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==25016== by 0x5D73F14: g_thread_proxy (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==25016== by 0x60160C9: start_thread (pthread_create.c:309)
Created attachment 279587 [details] [review] aggregator: Avoid destroying source we do not own https://bugzilla.gnome.org/show_bug.cgi?id=732116
Review of attachment 279587 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ +382,3 @@ +_destroy_gsource (GSource * source) +{ + g_source_destroy (source); Don't you additionally have to unref the source? Otherwise looks good
Created attachment 279588 [details] [review] aggregator: Avoid destroying source we do not own
Review of attachment 279588 [details] [review]: See last comment :) And ... ::: gst-libs/gst/base/gstaggregator.c @@ +501,3 @@ + source = g_idle_source_new (); + g_source_set_callback (source, (GSourceFunc) aggregate_func, self, NULL); + priv->gsources = g_list_append (priv->gsources, source); g_list_prepend() or use a GQueue. But g_list_append() is O(n)
(In reply to comment #2) > Review of attachment 279587 [details] [review]: > > ::: gst-libs/gst/base/gstaggregator.c > @@ +382,3 @@ > +_destroy_gsource (GSource * source) > +{ > + g_source_destroy (source); > > Don't you additionally have to unref the source? Otherwise looks good In g_source_attach_unlocked it increases the refcount of the source and in g_main_context_invoke_full it was g_source_unref ing, so we actually own the refs at this point.
Created attachment 279589 [details] [review] aggregator: Avoid destroying source we do not own Use g_list_prepend instead of g_list_append
(In reply to comment #5) > (In reply to comment #2) > > Review of attachment 279587 [details] [review] [details]: > > > > ::: gst-libs/gst/base/gstaggregator.c > > @@ +382,3 @@ > > +_destroy_gsource (GSource * source) > > +{ > > + g_source_destroy (source); > > > > Don't you additionally have to unref the source? Otherwise looks good > > In g_source_attach_unlocked it increases the refcount of the source and in > g_main_context_invoke_full it was g_source_unref ing, so we actually own the > refs at this point. Yes, that's why I think it should be > g_source_destroy (source); > g_source_unref (source);
Created attachment 279591 [details] [review] aggregator: Avoid destroying source we do not own Remove all source on dispose as a GSource can be added on pad-removed during finalization
Review of attachment 279591 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ +1041,3 @@ + + _remove_all_sources (self); + G_OBJECT_CLASS (aggregator_parent_class)->dispose (object); This should probably already happen in PAUSED->READY state transition
Created attachment 279594 [details] [review] aggregator: Avoid destroying sources we do not own + Unref the maincontext in a new dispose function + Make sure to remove all sources on dispose
(In reply to comment #9) > Review of attachment 279591 [details] [review]: > > ::: gst-libs/gst/base/gstaggregator.c > @@ +1041,3 @@ > + > + _remove_all_sources (self); > + G_OBJECT_CLASS (aggregator_parent_class)->dispose (object); > > This should probably already happen in PAUSED->READY state transition It does but GstElement->dispose () removes pads which leads to adding a GSource to the main context and thuse we end up with 1 GSource set to the maincontext and we own a ref to it.
> Yes, that's why I think it should be > > g_source_destroy (source); > > g_source_unref (source); I did that in the last patch
Attachment 279594 [details] pushed as 71c81a5 - aggregator: Avoid destroying sources we do not own