GNOME Bugzilla – Bug 768100
ghostpad: invalid ref getting internal pad
Last modified: 2018-11-03 12:35:13 UTC
Hello, I found an invalid reference usage while I was doing "massive testing". In the GDB trace [gdb-1] we can see that the internal pad ref is not valid when the refcount is tried to be increased [ref-1] Refs [ref-1] https://github.com/Kurento/gstreamer/blob/2f41e7bc6a842044cb73bc0470601200575c378a/gst/gstghostpad.c#L224 GDB traces [gdb-1]
+ Trace 236395
Created attachment 330438 [details] [review] ghostpad: set internal ref of the internal pad to NULL This patch should fix the bug.
Created attachment 330439 [details] [review] ghostpad: manage internal ref holding the lock
Review of attachment 330438 [details] [review]: ::: gst/gstghostpad.c @@ +507,3 @@ internal = GST_PROXY_PAD_INTERNAL (pad); + GST_OBJECT_LOCK (internal); Taking the object lock of the internal pad while we hold the one of the pad seems dangerous here, especially when seeing the gst_object_unparent() further below that might trigger going into another dispose().
Review of attachment 330439 [details] [review]: ::: gst/gstghostpad.c @@ +590,3 @@ goto parent_failed; + GST_OBJECT_LOCK (internal); construct() is supposed to be called exactly after instantiation of the object, nothing else should have a reference to it other than this very thread that runs construct(). When does this lead to problems? @@ +842,3 @@ internal = GST_PROXY_PAD_INTERNAL (gpad); + if (newtarget == internal) { This would be a programming error and should get a g_return_val_if_fail() or g_warning(). You should never set the internal pad as target of its ghostpad.
Review of attachment 330438 [details] [review]: ::: gst/gstghostpad.c @@ +507,3 @@ internal = GST_PROXY_PAD_INTERNAL (pad); + GST_OBJECT_LOCK (internal); In general your fix makes sense though, just needs careful checking if it can't lead to a deadlock (which I did not do) @@ -511,2 @@ /* disposes of the internal pad, since the ghostpad is the only possible object * that has a refcount on the internal pad. */ This comment also seems wrong :)
Review of attachment 330438 [details] [review]: ::: gst/gstghostpad.c @@ +507,3 @@ internal = GST_PROXY_PAD_INTERNAL (pad); + GST_OBJECT_LOCK (internal); The GST_PROXY_PAD_INTERNAL usage must be done holding the lock to avoid invalid reference. I reviewed the patch and made some tests, but I agree with you to be reviewed carefully again. @@ -511,2 @@ /* disposes of the internal pad, since the ghostpad is the only possible object * that has a refcount on the internal pad. */ This comment is related with this other: https://github.com/Kurento/gstreamer/blob/2f41e7bc6a842044cb73bc0470601200575c378a/gst/gstghostpad.c#L589
Review of attachment 330439 [details] [review]: ::: gst/gstghostpad.c @@ +590,3 @@ goto parent_failed; + GST_OBJECT_LOCK (internal); gst_ghost_pad_construct is public, so it is not ensured to be only used from gst_ghost_pad_new_full. Moreover: - I prefer having the discipline of managing GST_PROXY_PAD_INTERNAL into the critical section - The lock is also held for the pad var @@ +842,3 @@ internal = GST_PROXY_PAD_INTERNAL (gpad); + if (newtarget == internal) { I cannot use g_return_val_if_fail because this check is into the critical section and the lock must be released. Do you want to use g_warning with a specific message?
(In reply to Miguel París Díaz from comment #7) > Review of attachment 330439 [details] [review] [review]: > > ::: gst/gstghostpad.c > @@ +590,3 @@ > goto parent_failed; > > + GST_OBJECT_LOCK (internal); > > gst_ghost_pad_construct is public, so it is not ensured to be only used from > gst_ghost_pad_new_full. > Moreover: > - I prefer having the discipline of managing GST_PROXY_PAD_INTERNAL into > the critical section > - The lock is also held for the pad var It's invalid usage of the API otherwise, but I agree that we should take the lock here for consistency reasons already. I was just wondering if you were getting a problem from that specific code too. > @@ +842,3 @@ > internal = GST_PROXY_PAD_INTERNAL (gpad); > > + if (newtarget == internal) { > > I cannot use g_return_val_if_fail because this check is into the critical > section and the lock must be released. > Do you want to use g_warning with a specific message? Or unlock inside the if and then do a g_return_val_if_fail() ;)
(In reply to Sebastian Dröge (slomo) from comment #8) > (In reply to Miguel París Díaz from comment #7) > > Review of attachment 330439 [details] [review] [review] [review]: > > > > ::: gst/gstghostpad.c > > @@ +590,3 @@ > > goto parent_failed; > > > > + GST_OBJECT_LOCK (internal); > > > > gst_ghost_pad_construct is public, so it is not ensured to be only used from > > gst_ghost_pad_new_full. > > Moreover: > > - I prefer having the discipline of managing GST_PROXY_PAD_INTERNAL into > > the critical section > > - The lock is also held for the pad var > > It's invalid usage of the API otherwise, but I agree that we should take the > lock here for consistency reasons already. I was just wondering if you were > getting a problem from that specific code too. I don't think so, my problem should have happened by the code changed in the other commit. > > > @@ +842,3 @@ > > internal = GST_PROXY_PAD_INTERNAL (gpad); > > > > + if (newtarget == internal) { > > > > I cannot use g_return_val_if_fail because this check is into the critical > > section and the lock must be released. > > Do you want to use g_warning with a specific message? > > Or unlock inside the if and then do a g_return_val_if_fail() ;) OK, I will do it!!
Created attachment 330491 [details] [review] ghostpad: manage internal ref holding the lock
Patch was updated.
Could you try with latest master? This might already have been fixed: commit b63ed9e066e24e3fb110cff351fbf841a16c475f Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Nov 24 09:53:41 2017 +0100 ghostpad: return TRUE if target pad was already set The state is as it should be, so no reason to return FALSE really, everything's good. commit 3203a10821ae0f05b42f7e2556a9869598e701c1 Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Nov 24 09:40:07 2017 +0100 ghostpad: access internal pad with lock held commit e515aa06fe9496674dbc31eb7de642ffbd2ac28d Author: Havard Graff <havard.graff@gmail.com> Date: Thu Mar 30 09:17:08 2017 +0200 ghostpad: fix race-condition while tearing down An upstream query will take a ref on the internal proxypad, and can hence end up owning the last reference to that pad, causing a crash.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/177.