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

[Reimar Döffinger]

On Fri, Aug 26, 2011 at 01:17:39PM +0000, Loren Merritt wrote:
> On Fri, 26 Aug 2011, Reimar Döffinger wrote:
> > On 26 Aug 2011, at 00:55, Aaron Colwell wrote:
> >> On Thu, Aug 25, 2011 at 3:15 PM, Reimar Döffinger wrote:
> >>> On Thu, Aug 25, 2011 at 02:45:25PM -0700, Aaron Colwell wrote:
> >>>
> >>>> 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.
> 
> In particular, a double-checked condition variable can fail if your
> architecture allows two stores to different addresses by one processor to
> arrive at another processor out of order, or if the condition is not an
> atomic read. (No comment on how common that is among real cpus.)

I do not think that is enough to make the construct that is used
in FFmpeg fail.
What is happening here is that one process does

while (1){
  if (state != a) wait-for-condition;
  state = b;
}

and the other

while (1){
  if (state != b) wait-for-condition;
  state = a;
}

This can only really fail if a write would "arrive" at the other processor
before it is actually written out, i.e.

Thread 1: state = a;
Thread 2: if (state != a) <- is already considered false
Thread 2: state = b; <- written out to memory immediately
<- only now is a written out to memory, so state now has the wrong value

This seems like a rather insane and unlikely design so I'm not really
concerned about it. Of course whether there's a point in keeping the
current implementation instead of simplifying it is a different question
and benchmarks would probably answer that best.