After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 732750 - videoaggregator: broken locking in setcaps function
videoaggregator: broken locking in setcaps function
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal critical
: 1.3.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-04 22:21 UTC by Tim-Philipp Müller
Modified: 2014-07-06 21:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videoaggregator: Fix some more the locking logic in update_src_caps (2.06 KB, patch)
2014-07-06 21:36 UTC, Thibault Saunier
committed Details | Review

Description Tim-Philipp Müller 2014-07-04 22:21:51 UTC
I don't really understand how the locking is supposed to work there - at least four different locks at the same time! Respect! (PAD_STREAM_LOCK, AGGREGATOR_SETCAPS_LOCK, AGGREGATOR_LOCK, OBJECT_LOCK)

All I can say is that it's buggy:

Line 558 does GST_OBJECT_UNLOCK (vagg); and then line 613 and 633 call GST_OBJECT_UNLOCK again.
Comment 1 Tim-Philipp Müller 2014-07-04 22:23:23 UTC
This is gst_videoaggregator_update_src_caps() btw.
Comment 2 Tim-Philipp Müller 2014-07-06 21:19:43 UTC
This at least un-breaks it:

 commit 341dc8aecf460bd28868035a8c35d6a7c8bdeec3
 Author: Tim-Philipp Müller <tim@centricular.com>
 Date:   Sun Jul 6 22:16:48 2014 +0100

    videoaggregator: fix broken locking in update_src_caps function
    
    We would unlock an already-unlocked mutex that we never re-locked.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=732750
Comment 3 Thibault Saunier 2014-07-06 21:36:16 UTC
Created attachment 280012 [details] [review]
videoaggregator: Fix some more the locking logic in update_src_caps

We need the GST_OBJECT_LOCK only to iterate the sinkpads, nothing else.
Comment 4 Thibault Saunier 2014-07-06 21:37:44 UTC
(In reply to comment #0)
> I don't really understand how the locking is supposed to work there - at least
> four different locks at the same time! Respect! (PAD_STREAM_LOCK,
> AGGREGATOR_SETCAPS_LOCK, AGGREGATOR_LOCK, OBJECT_LOCK)
> 
> All I can say is that it's buggy:
> 
> Line 558 does GST_OBJECT_UNLOCK (vagg); and then line 613 and 633 call
> GST_OBJECT_UNLOCK again.

Not sure why you talk about the pad stream lock.

Then the locking logic is the exact same one as with videomixer, at that time we need the src cpas lock avoid concurrent srcpad caps negotiation, then we need to avoid the aggregate function to be called at that same time. And we need the object lock to iterate the sinkpads, I agree it can look weird but I think it is correct. I attach here a patch that should be more correct as the object lock should be taken only for the iteration part.
Comment 5 Thibault Saunier 2014-07-06 21:53:01 UTC
Attachment 280012 [details] pushed as e4b07ee - videoaggregator: Fix some more the locking logic in update_src_caps