GNOME Bugzilla – Bug 623589
Fix races/refcounting bugs with slave clocks
Last modified: 2010-07-06 09:24:20 UTC
Created attachment 165271 [details] [review] add gst_clock_id_wait_async_full I found a race and a refcounting bug using slaved clocks. I'm attacching patches here for review. I added a new gst_clock_id_wait_async_full function which is now used by gst_clock_set_master. The function is needed to avoid a race between the master's slave_callback and the slave being deallocated. Also in gst_clock_set_master we were possibly deallocating a clock (the master) before unreffing one of its entries. I fixed that.
Created attachment 165272 [details] [review] fix refcounting bug
Created attachment 165273 [details] [review] use gst_clock_id_wait_async_full + tests
Comment on attachment 165271 [details] [review] add gst_clock_id_wait_async_full >--- a/gst/gstclock.h >+++ b/gst/gstclock.h >@@ -346,6 +346,7 @@ struct _GstClockEntry { > GstClockReturn status; > GstClockCallback func; > gpointer user_data; >+ GDestroyNotify destroy_data; > }; This patch looks harmless and ok in general, but here we're extending a public struct that has no padding, not sure how I feel about that, wonder what the chances are that someone outthere has subclassed this..
Review of attachment 165271 [details] [review]: ::: gst/gstclock.h @@ +347,3 @@ GstClockCallback func; gpointer user_data; + GDestroyNotify destroy_data; We can't do that, GstClockEntry is public and this changes its size.
> + GDestroyNotify destroy_data; > > We can't do that, GstClockEntry is public and this changes its size. This is going to fail when someone were to allocate a GstClockID themselves with the slice allocator and so, instead of using the _new_*_id() functions. If someone would do that, it would have failed previously as well because you need to give the size to the slice allocator to free the entry (and that size would be wrong when someone extended the structure and used the default unref function). The only way it could have worked previously is when the custom GstClockEntry allocator would keep an extra ref to the entry and free them when cleaning up. None of the existing elements that perform sync would ever manage to allocate such an entry.. I'm pretty confident nobody does this and that making the struct bigger is not going to cause trouble.
Ok then, let's do it. Maybe also add a sentence to the GstClockEntry docs that makes it clear that the structure should be treated as an opaque structure and must not be subclassed or allocated outside of GStreamer. Wim, were the other patches also fine with you?
> Wim, were the other patches also fine with you? I don't quite understand the reason for this second patch. the other patch is fine.
(just for reference since the discussion carried on on IRC and this patch set was committed) The second patch is needed to avoid unreffing clock->clockid after unreffing clock->master. clock->clockid belongs to clock->master here so if clock->master is unreffed and deallocated, unreffing clock->clockid could access deallocated memory.
commit 7acde37fc10486908e3b685ecd895543beaf8d96 Author: Alessandro Decina <alessandro.decina@collabora.co.uk> Date: Tue Jul 6 10:35:09 2010 +0200 win32: export gst_clock_id_wait_async_full commit e6889791698f01ae237f41355df96a8547a21f6c Author: Alessandro Decina <alessandro.decina@collabora.co.uk> Date: Tue Jul 6 10:31:25 2010 +0200 tests: remove ABI checks for GstClockEntry. commit a02fcb0478fb196271425527b3e070b4ae215fcd Author: Alessandro Decina <alessandro.decina@collabora.co.uk> Date: Mon Jul 5 18:45:55 2010 +0200 clock: document that GstClockEntry should be treated as ana opaque structure. commit 819780acf79217c8a9b40c486f33668c0e2ae8d7 Author: Alessandro Decina <alessandro.decina@collabora.co.uk> Date: Mon Jul 5 13:10:09 2010 +0200 clock: use the new gst_clock_id_wait_async_full. Use the new gst_clock_id_wait_async_full in gst_clock_set_master. Also add some tests. commit 538e82c6f7ff8f2652651d6201c32f27b0ae3a62 Author: Alessandro Decina <alessandro.decina@collabora.co.uk> Date: Mon Jul 5 13:01:53 2010 +0200 clock: fix refcounting bug in gst_clock_set_master. Make sure clock->clockid is unreffed before clock->master. gst_clock_id_unschedule (clock->clockid) tries to access clock->master. If clock->master is unreffed before and it's deallocated, _unschedule could access free'd memory. commit dda86638110a97518c0d76aff405ffc0781c3573 Author: Alessandro Decina <alessandro.decina@collabora.co.uk> Date: Mon Jul 5 12:56:40 2010 +0200 clock: add gst_clock_id_wait_async_full. Add gst_clock_id_wait_async_full. It's the same as gst_clock_id_wait_async but allows passing a GDestroyNotify to destroy user_data.