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

[Reimar Döffinger]

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:
> 
> > On Thu, Aug 25, 2011 at 11:26:09AM -0700, Aaron Colwell wrote:> 4. Why is
> > "double-checked locking" being used? Will there be significant
> > > protest if I remove it?
> >
> > 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.

> > > 5. What is the relationship between PerThreadContext::mutex
> > > & PerThreadContext::progress_mutex and what member variables are they
> > > supposed to protect?
> >
> > Obviously input_cond and progress_cond.
> > Beyond that it get murky, however it is likely progress_mutex would be
> > for things that change during frame process while mutex for things
> > that change between frames.
> >
> > > At times these 3 cases appear to modify the same state variables.
> >
> > Being more specific would allow us to give more specific answers.
> >
> 
> 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.