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

[Aaron Colwell]

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. :)


> >
> > 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!

In any case it looks like I have my work cut out for me. If you have any
other insights or suggestions I'd appreciate them. Are there any other docs
for this code you might know about?

Aaron