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

Reimar Döffinger Reimar.Doeffinger at gmx.de
Fri Aug 26 08:24:43 CEST 2011



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.


More information about the ffmpeg-devel mailing list