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 606435 - gsttee not threadsafe
gsttee not threadsafe
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal critical
: 0.10.26
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-01-08 18:58 UTC by Håvard Graff (hgr)
Modified: 2010-01-11 10:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (3.47 KB, patch)
2010-01-08 18:58 UTC, Håvard Graff (hgr)
none Details | Review

Description Håvard Graff (hgr) 2010-01-08 18:58:48 UTC
Created attachment 151052 [details] [review]
patch

The race is basically that in release_pad, the allocpad is "padA", and the pad being freed is "padB". Now, while just having completed the check "is allocpad (padA) == the pad being freed (padB), the function will let go of the OBJECT_LOCK(), and go on to take the DYN_LOCK(), and start to free the pad. However, in this "lock-gap", allocpad could be set to "padB", and this will crash horribly next time allocpad is tried used, since it now will be pointing to the freed padB.
Comment 1 Tim-Philipp Müller 2010-01-08 20:14:24 UTC
Feel like writing a unit test for this as well by any chance? :)
Comment 2 Håvard Graff (hgr) 2010-01-09 02:18:59 UTC
I have a test I used for the development of the patch, but to reproduce the race reliably I had to add a sleep in the "lock-gap". If the sleep is not there, the window is so small that it would only happen once every few 100 runs or so, in other words not well suited for a unittest. Also, the test I used is very much tied up to our framework.

However, we do plan to start running the gstreamer-tests in our framework as well, and when that happens it will be easier for us to add to, and expand on those for our element/pipeline oriented testing.
Comment 3 Wim Taymans 2010-01-11 10:17:54 UTC
Review of attachment 151052 [details] [review]:

The first part seems ok.
Is the get_qdata() in gst_tee_find_buffer_alloc() really needed? It seems the ->removed field that you are accessing
is changed with the DYN_LOCK so it seems to be missing a lock somewhere.
Comment 4 Wim Taymans 2010-01-11 10:57:15 UTC
Can you try this patch? Reopen if that is not enough for some reason.

commit a3c4a3201a705eb1934ceeea34d1ca42d4571c07
Author: Håvard Graff <havard.graff@tandberg.com>
Date:   Mon Jan 11 11:38:32 2010 +0100

    tee: make release_pad threadsafe
    
    Protect the ->removed field with the object lock as well. Take the DYN lock
    earlier so that we can mark the pad removed and avoid a race in pad_alloc.
    
    Fixes #606435