[FFmpeg-devel] Race conditions in libavcodec/pthread.c

[Reimar Döffinger]

On 26 Aug 2011, at 00:55, Aaron Colwell <acolwell at chromium.org> wrote:

> On Thu, Aug 25, 2011 at 3:15 PM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de>wrote:
> 
>> On Thu, Aug 25, 2011 at 02:45:25PM -0700, Aaron Colwell wrote:
>>> On Thu, Aug 25, 2011 at 1:31 PM, Reimar Döffinger
>>> <Reimar.Doeffinger at gmx.de>wrote:
>>> 
>>>> It would help if you pointed out there. I don't see any case of it
>>>> at a quick glance.
>>>> 
>>> 
>>> park_frame_worker_threads() has an instance. p->state shouldn't be
>> checked
>>> outside of the lock.
>>> frame_worker_thread() has an instance at the top of the while loop.
>>> submit_packet() has an instance after the if(prev_thread)  line.
>>> ff_thread_decode_frame() has an instance at the top of the do loop.
>> 
>> Ah. I don't think state is supposed to/intended to be protected by a
>> lock, but the lock/unlock is only there because you can't use
>> pthread_cond_wait
>> without it.
>> I guess changing that would make it more correct, but it would be a change
>> in
>> semantics and it looks to me like you'd have to add a whole lot more
>> lock/unlock around places where it is used.
>> 
>> 
> The thing is that state is modified by the main thread (submit_packet()) &
> worker threads (frame_worker_thread()). It needs lock protection to avoid
> race conditions. I agree it's a non-trivial change to fix this. That's why I
> wanted to ping the list first. I figured it was worth a discussion before
> diving in. :)

See Uoti's comment, except possibly for architectures with strange caching models that should be the right answer.

> 
>>> Sorry about that. The main one I've seen so far is in
>> frame_worker_thread().
>>> ff_thread_finish_setup() is called with and w/o p->mutex protection in
>> that
>>> method. ff_thread_finish_setup() locks p->progress_mutex so it sets up a
>>> situation where locks may be acquired out of order. I haven't completely
>>> proven to myself this will lead to deadlock, but it seems like a red flag
>>> that something isn't right.
>> 
>> That one looks like pure laziness to avoid duplicating code by having
>> the unlock in both the if and the else case.
>> I think the buffer_mutex is only necessary/intended for the get_buffer
>> and requested_frame usages.
>> 
> 
> Oh I didn't see the case in ff_thread_get_buffer() . This means it runs in 3
> different locking scenarios: no other locks, w/ buffer_mutex, & w/ mutex!

I wondering if maybe pthread's extensive use of the word "lock" maybe confuses you.
As far as I can tell, the code mostly uses two concepts: critical sections, for dealing with and protecting the library users from seeing the threading, and a signalling/token scheme to wait on another thread.
They aren't for directly protecting any variables, variables that are used by concurrently "active" threads are protected by having multiple copies of them.