GNOME Bugzilla – Bug 606435
gsttee not threadsafe
Last modified: 2010-01-11 10:57:15 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.
Feel like writing a unit test for this as well by any chance? :)
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.
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.
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