[FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().

[Wan-Teh Chang]

Hi Michael,

On Mon, Nov 21, 2016 at 4:35 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Mon, Nov 21, 2016 at 03:37:49PM -0800, Wan-Teh Chang wrote:
>> Make the one-time initialization in av_get_cpu_flags() thread-safe.
>> The fix assumes -1 is an invalid value for cpu flags.
>
> please explain the bug / race that occurs and how it is fixed

Note: I will include the following explanation in the next version of my patch.

The static variables |flags| and |checked| in libavutil/cpu.c are read
and written using normal load and store instructions. These are
considered as data races. The fix is to use atomic load and store
instructions. The fix also eliminates the |checked| variable because
the invalid value of -1 for |flags| can be used to indicate the same
condition.

Our application runs into the data races because two threads call
avformat_find_stream_info() at the same time.
avformat_find_stream_info() calls av_parser_init(), which may
eventually call av_get_cpu_flag(), depending on the codec.

I just wrote a minimal test case (libavutil/tests/cpu_init.c) that
reproduces the data races. The test program creates two threads that
call av_get_cpu_flags(). When built with --toolchain=clang-tsan, the
test program triggers two ThreadSanitizer warnings:

$ libavutil/tests/cpu_init
==================
WARNING: ThreadSanitizer: data race (pid=66608)
  Read of size 4 at 0x7f7aa8c15d40 by thread T2:
    #0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:78
(exe+0x0000000a8536)
    #1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x0000000a8494)

  Previous write of size 4 at 0x7f7aa8c15d40 by thread T1:
    #0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:90
(exe+0x0000000a8578)
    #1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x0000000a8494)

  Thread T2 (tid=66611, running) created by main thread at:
    #0 pthread_create ??:0 (exe+0x00000004d9bb)
    #1 main /usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:51
(exe+0x0000000a83cb)

  Thread T1 (tid=66610, running) created by main thread at:
    #0 pthread_create ??:0 (exe+0x00000004d9bb)
    #1 main /usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:47
(exe+0x0000000a83a9)

SUMMARY: ThreadSanitizer: data race
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:78
av_get_cpu_flags
==================
==================
WARNING: ThreadSanitizer: data race (pid=66608)
  Read of size 4 at 0x7f7aa8c15d3c by thread T2:
    #0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:79
(exe+0x0000000a854b)
    #1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x0000000a8494)

  Previous write of size 4 at 0x7f7aa8c15d3c by thread T1:
    #0 av_get_cpu_flags
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:88
(exe+0x0000000a8566)
    #1 thread_main
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:32
(exe+0x0000000a8494)

  Thread T2 (tid=66611, running) created by main thread at:
    #0 pthread_create ??:0 (exe+0x00000004d9bb)
    #1 main /usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:51
(exe+0x0000000a83cb)

  Thread T1 (tid=66610, running) created by main thread at:
    #0 pthread_create ??:0 (exe+0x00000004d9bb)
    #1 main /usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/tests/cpu_init.c:47
(exe+0x0000000a83a9)

SUMMARY: ThreadSanitizer: data race
/usr/local/google/home/wtc/ffmpeg-cpu-data-race-before/ffmpeg/libavutil/cpu.c:79
av_get_cpu_flags
==================
ThreadSanitizer: reported 2 warnings

>> The fix requires new atomic functions to get, set, and compare-and-swap
>> an integer without a memory barrier.
>
> why ?

The fix needs a compare-and-swap function for an atomic integer. There
is no such function in libavutil/atomic.h.

Although the fix can use avpriv_atomic_int_get() and
avpriv_atomic_int_set(), these two functions execute a memory barrier
with sequentially-consistent ordering, which the fix doesn't need. To
improve performance, I added avpriv_atomic_int_get_relaxed() and
avpriv_atomic_int_set_relaxed()

>> The data race fix is written by Dmitry Vyukov of Google.
>
> Then the author for the git patch should be set accordingly

I will look into this. I may just identify him as a co-author.

>> @@ -44,7 +45,20 @@
>>  #include <unistd.h>
>>  #endif
>>
>> -static int flags, checked;
>> +static int cpu_flags = -1;
>> +
>> +static int get_cpu_flags(void)
>> +{
>> +    if (ARCH_AARCH64)
>> +        return ff_get_cpu_flags_aarch64();
>> +    if (ARCH_ARM)
>> +        return ff_get_cpu_flags_arm();
>> +    if (ARCH_PPC)
>> +        return ff_get_cpu_flags_ppc();
>> +    if (ARCH_X86)
>> +        return ff_get_cpu_flags_x86();
>> +    /* Not reached. */
>
> src/libavutil/cpu.c: In function ‘get_cpu_flags’:
> src/libavutil/cpu.c:61: error: control reaches end of non-void function

Thanks. I will fix this in the next version of my patch.

Wan-Teh Chang