[FFmpeg-devel] [PATCH] avutil: fix data race in av_get_cpu_flags().
wm4
nfxjfg at googlemail.com
Tue Nov 22 23:41:59 EET 2016
On Tue, 22 Nov 2016 13:36:11 -0800
Wan-Teh Chang <wtc-at-google.com at ffmpeg.org> wrote:
> Make the one-time initialization in av_get_cpu_flags() thread-safe. The
> static variables |flags| and |checked| in libavutil/cpu.c are read and
> written using normal load and store operations. These are considered as
> data races. The fix is to use atomic load and store operations. The fix
> also eliminates the |checked| variable because the invalid value of -1
> for |flags| can be used to indicate the same condition.
>
> The fix requires a new atomic integer compare and swap function. Since
> the fix does not need memory barriers, "relaxed" variants of
> avpriv_atomic_int_get() and avpriv_atomic_int_set() are also added.
>
> Add a test program libavutil/tests/cpu_init.c to verify the one-time
> initialization in av_get_cpu_flags() is thread-safe.
>
> Co-author: Dmitry Vyukov of Google, which suggested the data race fix.
>
> Signed-off-by: Wan-Teh Chang <wtc at google.com>
> ---
> libavutil/Makefile | 1 +
> libavutil/atomic.c | 40 ++++++++++++++++++++++++++
> libavutil/atomic.h | 34 ++++++++++++++++++++--
> libavutil/atomic_gcc.h | 33 +++++++++++++++++++++
> libavutil/atomic_suncc.h | 19 ++++++++++++
> libavutil/atomic_win32.h | 21 ++++++++++++++
> libavutil/cpu.c | 41 ++++++++++++++------------
> libavutil/cpu.h | 2 --
> libavutil/tests/atomic.c | 13 +++++++++
> libavutil/tests/cpu_init.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++
> 10 files changed, 253 insertions(+), 23 deletions(-)
> create mode 100644 libavutil/tests/cpu_init.c
>
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 0fa90fe..732961a 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -187,6 +187,7 @@ TESTPROGS = adler32 \
> camellia \
> color_utils \
> cpu \
> + cpu_init \
> crc \
> des \
> dict \
> diff --git a/libavutil/atomic.c b/libavutil/atomic.c
> index 64cff25..7bce013 100644
> --- a/libavutil/atomic.c
> +++ b/libavutil/atomic.c
> @@ -40,6 +40,11 @@ int avpriv_atomic_int_get(volatile int *ptr)
> return res;
> }
>
> +int avpriv_atomic_int_get_relaxed(volatile int *ptr)
> +{
> + return avpriv_atomic_int_get(ptr);
> +}
> +
> void avpriv_atomic_int_set(volatile int *ptr, int val)
> {
> pthread_mutex_lock(&atomic_lock);
> @@ -47,6 +52,11 @@ void avpriv_atomic_int_set(volatile int *ptr, int val)
> pthread_mutex_unlock(&atomic_lock);
> }
>
> +void avpriv_atomic_int_set_relaxed(volatile int *ptr, int val)
> +{
> + avpriv_atomic_int_set(ptr, val);
> +}
> +
> int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc)
> {
> int res;
> @@ -59,6 +69,17 @@ int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc)
> return res;
> }
>
> +int avpriv_atomic_int_cas_relaxed(volatile int *ptr, int oldval, int newval)
> +{
> + int ret;
> + pthread_mutex_lock(&atomic_lock);
> + ret = *ptr;
> + if (ret == oldval)
> + *ptr = newval;
> + pthread_mutex_unlock(&atomic_lock);
> + return ret;
> +}
> +
> void *avpriv_atomic_ptr_cas(void * volatile *ptr, void *oldval, void *newval)
> {
> void *ret;
> @@ -77,17 +98,36 @@ int avpriv_atomic_int_get(volatile int *ptr)
> return *ptr;
> }
>
> +int avpriv_atomic_int_get_relaxed(volatile int *ptr)
> +{
> + return avpriv_atomic_int_get(ptr);
> +}
> +
> void avpriv_atomic_int_set(volatile int *ptr, int val)
> {
> *ptr = val;
> }
>
> +void avpriv_atomic_int_set_relaxed(volatile int *ptr, int val)
> +{
> + avpriv_atomic_int_set(ptr, val);
> +}
> +
> int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc)
> {
> *ptr += inc;
> return *ptr;
> }
>
> +int avpriv_atomic_int_cas_relaxed(volatile int *ptr, int oldval, int newval)
> +{
> + if (*ptr == oldval) {
> + *ptr = newval;
> + return oldval;
> + }
> + return *ptr;
> +}
> +
> void *avpriv_atomic_ptr_cas(void * volatile *ptr, void *oldval, void *newval)
> {
> if (*ptr == oldval) {
> diff --git a/libavutil/atomic.h b/libavutil/atomic.h
> index 15906d2..c5aa1a4 100644
> --- a/libavutil/atomic.h
> +++ b/libavutil/atomic.h
> @@ -40,20 +40,38 @@
> *
> * @param ptr atomic integer
> * @return the current value of the atomic integer
> - * @note This acts as a memory barrier.
> + * @note This acts as a memory barrier with sequentially-consistent ordering.
> */
> int avpriv_atomic_int_get(volatile int *ptr);
>
> /**
> + * Load the current value stored in an atomic integer.
> + *
> + * @param ptr atomic integer
> + * @return the current value of the atomic integer
> + * @note This does NOT act as a memory barrier.
> + */
> +int avpriv_atomic_int_get_relaxed(volatile int *ptr);
> +
> +/**
> * Store a new value in an atomic integer.
> *
> * @param ptr atomic integer
> * @param val the value to store in the atomic integer
> - * @note This acts as a memory barrier.
> + * @note This acts as a memory barrier with sequentially-consistent ordering.
> */
> void avpriv_atomic_int_set(volatile int *ptr, int val);
>
> /**
> + * Store a new value in an atomic integer.
> + *
> + * @param ptr atomic integer
> + * @param val the value to store in the atomic integer
> + * @note This does NOT act as a memory barrier.
> + */
> +void avpriv_atomic_int_set_relaxed(volatile int *ptr, int val);
> +
> +/**
> * Add a value to an atomic integer.
> *
> * @param ptr atomic integer
> @@ -65,12 +83,24 @@ void avpriv_atomic_int_set(volatile int *ptr, int val);
> int avpriv_atomic_int_add_and_fetch(volatile int *ptr, int inc);
>
> /**
> + * Atomic integer compare and swap.
> + *
> + * @param ptr pointer to the integer to operate on
> + * @param oldval do the swap if the current value of *ptr equals to oldval
> + * @param newval value to replace *ptr with
> + * @return the value of *ptr before comparison
> + * @note This does NOT act as a memory barrier.
> + */
> +int avpriv_atomic_int_cas_relaxed(volatile int *ptr, int oldval, int newval);
> +
> +/**
> * Atomic pointer compare and swap.
> *
> * @param ptr pointer to the pointer to operate on
> * @param oldval do the swap if the current value of *ptr equals to oldval
> * @param newval value to replace *ptr with
> * @return the value of *ptr before comparison
> + * @note This acts as a memory barrier with sequentially-consistent ordering.
> */
> void *avpriv_atomic_ptr_cas(void * volatile *ptr, void *oldval, void *newval);
>
> diff --git a/libavutil/atomic_gcc.h b/libavutil/atomic_gcc.h
> index 5f9fc49..15dd508 100644
> --- a/libavutil/atomic_gcc.h
> +++ b/libavutil/atomic_gcc.h
> @@ -36,6 +36,16 @@ static inline int atomic_int_get_gcc(volatile int *ptr)
> #endif
> }
>
> +#define avpriv_atomic_int_get_relaxed atomic_int_get_relaxed_gcc
> +static inline int atomic_int_get_relaxed_gcc(volatile int *ptr)
> +{
> +#if HAVE_ATOMIC_COMPARE_EXCHANGE
> + return __atomic_load_n(ptr, __ATOMIC_RELAXED);
> +#else
> + return *ptr;
> +#endif
> +}
> +
> #define avpriv_atomic_int_set atomic_int_set_gcc
> static inline void atomic_int_set_gcc(volatile int *ptr, int val)
> {
> @@ -47,6 +57,16 @@ static inline void atomic_int_set_gcc(volatile int *ptr, int val)
> #endif
> }
>
> +#define avpriv_atomic_int_set_relaxed atomic_int_set_relaxed_gcc
> +static inline void atomic_int_set_relaxed_gcc(volatile int *ptr, int val)
> +{
> +#if HAVE_ATOMIC_COMPARE_EXCHANGE
> + __atomic_store_n(ptr, val, __ATOMIC_RELAXED);
> +#else
> + *ptr = val;
> +#endif
> +}
> +
> #define avpriv_atomic_int_add_and_fetch atomic_int_add_and_fetch_gcc
> static inline int atomic_int_add_and_fetch_gcc(volatile int *ptr, int inc)
> {
> @@ -57,6 +77,19 @@ static inline int atomic_int_add_and_fetch_gcc(volatile int *ptr, int inc)
> #endif
> }
>
> +#define avpriv_atomic_int_cas_relaxed atomic_int_cas_relaxed_gcc
> +static inline int atomic_int_cas_relaxed_gcc(volatile int *ptr,
> + int oldval, int newval)
> +{
> +#if HAVE_SYNC_VAL_COMPARE_AND_SWAP
> + return __sync_val_compare_and_swap(ptr, oldval, newval);
> +#else
> + __atomic_compare_exchange_n(ptr, &oldval, newval, 0, __ATOMIC_RELAXED,
> + __ATOMIC_RELAXED);
> + return oldval;
> +#endif
> +}
> +
> #define avpriv_atomic_ptr_cas atomic_ptr_cas_gcc
> static inline void *atomic_ptr_cas_gcc(void * volatile *ptr,
> void *oldval, void *newval)
> diff --git a/libavutil/atomic_suncc.h b/libavutil/atomic_suncc.h
> index a75a37b..0997f23 100644
> --- a/libavutil/atomic_suncc.h
> +++ b/libavutil/atomic_suncc.h
> @@ -31,6 +31,12 @@ static inline int atomic_int_get_suncc(volatile int *ptr)
> return *ptr;
> }
>
> +#define avpriv_atomic_int_get_relaxed atomic_int_get_relaxed_suncc
> +static inline int atomic_int_get_relaxed_suncc(volatile int *ptr)
> +{
> + return *ptr;
> +}
> +
> #define avpriv_atomic_int_set atomic_int_set_suncc
> static inline void atomic_int_set_suncc(volatile int *ptr, int val)
> {
> @@ -38,12 +44,25 @@ static inline void atomic_int_set_suncc(volatile int *ptr, int val)
> __machine_rw_barrier();
> }
>
> +#define avpriv_atomic_int_set_relaxed atomic_int_set_relaxed_suncc
> +static inline void atomic_int_set_relaxed_suncc(volatile int *ptr, int val)
> +{
> + *ptr = val;
> +}
> +
> #define avpriv_atomic_int_add_and_fetch atomic_int_add_and_fetch_suncc
> static inline int atomic_int_add_and_fetch_suncc(volatile int *ptr, int inc)
> {
> return atomic_add_int_nv(ptr, inc);
> }
>
> +#define avpriv_atomic_int_cas_relaxed atomic_int_cas_relaxed_suncc
> +static inline int atomic_int_cas_relaxed_suncc(volatile int *ptr,
> + int oldval, int newval)
> +{
> + return atomic_cas_uint((volatile uint_t *)ptr, oldval, newval);
> +}
> +
> #define avpriv_atomic_ptr_cas atomic_ptr_cas_suncc
> static inline void *atomic_ptr_cas_suncc(void * volatile *ptr,
> void *oldval, void *newval)
> diff --git a/libavutil/atomic_win32.h b/libavutil/atomic_win32.h
> index f729933..7ea0a56 100644
> --- a/libavutil/atomic_win32.h
> +++ b/libavutil/atomic_win32.h
> @@ -31,6 +31,12 @@ static inline int atomic_int_get_win32(volatile int *ptr)
> return *ptr;
> }
>
> +#define avpriv_atomic_int_get_relaxed atomic_int_get_relaxed_win32
> +static inline int atomic_int_get_relaxed_win32(volatile int *ptr)
> +{
> + return *ptr;
> +}
> +
> #define avpriv_atomic_int_set atomic_int_set_win32
> static inline void atomic_int_set_win32(volatile int *ptr, int val)
> {
> @@ -38,12 +44,27 @@ static inline void atomic_int_set_win32(volatile int *ptr, int val)
> MemoryBarrier();
> }
>
> +#define avpriv_atomic_int_set_relaxed atomic_int_set_relaxed_win32
> +static inline void atomic_int_set_relaxed_win32(volatile int *ptr, int val)
> +{
> + *ptr = val;
> +}
> +
> #define avpriv_atomic_int_add_and_fetch atomic_int_add_and_fetch_win32
> static inline int atomic_int_add_and_fetch_win32(volatile int *ptr, int inc)
> {
> return inc + InterlockedExchangeAdd(ptr, inc);
> }
>
> +#define avpriv_atomic_int_cas_relaxed atomic_int_cas_relaxed_win32
> +static inline int atomic_int_cas_relaxed_win32(volatile int *ptr,
> + int oldval, int newval)
> +{
> + /* InterlockedCompareExchangeNoFence is available on Windows 8 or later
> + * only. */
> + return InterlockedCompareExchange((volatile LONG *)ptr, newval, oldval);
> +}
> +
> #define avpriv_atomic_ptr_cas atomic_ptr_cas_win32
> static inline void *atomic_ptr_cas_win32(void * volatile *ptr,
> void *oldval, void *newval)
> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> index f5785fc..70b61de 100644
> --- a/libavutil/cpu.c
> +++ b/libavutil/cpu.c
> @@ -23,6 +23,7 @@
> #include "config.h"
> #include "opt.h"
> #include "common.h"
> +#include "atomic.h"
>
> #if HAVE_SCHED_GETAFFINITY
> #ifndef _GNU_SOURCE
> @@ -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();
> + return 0;
> +}
>
> void av_force_cpu_flags(int arg){
> if ( (arg & ( AV_CPU_FLAG_3DNOW |
> @@ -69,33 +83,22 @@ void av_force_cpu_flags(int arg){
> arg |= AV_CPU_FLAG_MMX;
> }
>
> - flags = arg;
> - checked = arg != -1;
> + avpriv_atomic_int_set_relaxed(&cpu_flags, arg);
> }
>
> int av_get_cpu_flags(void)
> {
> - if (checked)
> - return flags;
> -
> - if (ARCH_AARCH64)
> - flags = ff_get_cpu_flags_aarch64();
> - if (ARCH_ARM)
> - flags = ff_get_cpu_flags_arm();
> - if (ARCH_PPC)
> - flags = ff_get_cpu_flags_ppc();
> - if (ARCH_X86)
> - flags = ff_get_cpu_flags_x86();
> -
> - checked = 1;
> + int flags = avpriv_atomic_int_get_relaxed(&cpu_flags);
> + if (flags == -1) {
> + flags = get_cpu_flags();
> + avpriv_atomic_int_cas_relaxed(&cpu_flags, -1, flags);
> + }
> return flags;
> }
>
> void av_set_cpu_flags_mask(int mask)
> {
> - checked = 0;
> - flags = av_get_cpu_flags() & mask;
> - checked = 1;
> + avpriv_atomic_int_set_relaxed(&cpu_flags, get_cpu_flags() & mask);
> }
>
> int av_parse_cpu_flags(const char *s)
> diff --git a/libavutil/cpu.h b/libavutil/cpu.h
> index 4bff167..8499f0e 100644
> --- a/libavutil/cpu.h
> +++ b/libavutil/cpu.h
> @@ -85,8 +85,6 @@ void av_force_cpu_flags(int flags);
> * Set a mask on flags returned by av_get_cpu_flags().
> * This function is mainly useful for testing.
> * Please use av_force_cpu_flags() and av_get_cpu_flags() instead which are more flexible
> - *
> - * @warning this function is not thread safe.
> */
> attribute_deprecated void av_set_cpu_flags_mask(int mask);
>
> diff --git a/libavutil/tests/atomic.c b/libavutil/tests/atomic.c
> index c92f220..5a12439 100644
> --- a/libavutil/tests/atomic.c
> +++ b/libavutil/tests/atomic.c
> @@ -26,9 +26,22 @@ int main(void)
>
> res = avpriv_atomic_int_add_and_fetch(&val, 1);
> av_assert0(res == 2);
> + res = avpriv_atomic_int_add_and_fetch(&val, -1);
> + av_assert0(res == 1);
> avpriv_atomic_int_set(&val, 3);
> res = avpriv_atomic_int_get(&val);
> av_assert0(res == 3);
> + avpriv_atomic_int_set_relaxed(&val, 4);
> + res = avpriv_atomic_int_get_relaxed(&val);
> + av_assert0(res == 4);
> + res = avpriv_atomic_int_cas_relaxed(&val, 3, 5);
> + av_assert0(res == 4);
> + res = avpriv_atomic_int_get_relaxed(&val);
> + av_assert0(res == 4);
> + res = avpriv_atomic_int_cas_relaxed(&val, 4, 5);
> + av_assert0(res == 4);
> + res = avpriv_atomic_int_get_relaxed(&val);
> + av_assert0(res == 5);
>
> return 0;
> }
> diff --git a/libavutil/tests/cpu_init.c b/libavutil/tests/cpu_init.c
> new file mode 100644
> index 0000000..402b6a8
> --- /dev/null
> +++ b/libavutil/tests/cpu_init.c
> @@ -0,0 +1,72 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/*
> + * This test program verifies that the one-time initialization in
> + * av_get_cpu_flags() is thread-safe.
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "config.h"
> +
> +#include "libavutil/cpu.h"
> +#include "libavutil/thread.h"
> +
> +#if HAVE_PTHREADS
> +static void *thread_main(void *arg)
> +{
> + int *flags = arg;
> +
> + *flags = av_get_cpu_flags();
> + return NULL;
> +}
> +#endif
> +
> +
> +int main(int argc, char **argv)
> +{
> +#if HAVE_PTHREADS
> + int cpu_flags1;
> + int cpu_flags2;
> + int ret;
> + pthread_t thread1;
> + pthread_t thread2;
> +
> + if ((ret = pthread_create(&thread1, NULL, thread_main, &cpu_flags1))) {
> + fprintf(stderr, "pthread_create failed: %s.\n", strerror(ret));
> + return 1;
> + }
> + if ((ret = pthread_create(&thread2, NULL, thread_main, &cpu_flags2))) {
> + fprintf(stderr, "pthread_create failed: %s.\n", strerror(ret));
> + return 1;
> + }
> + pthread_join(thread1, NULL);
> + pthread_join(thread2, NULL);
> +
> + if (cpu_flags1 < 0)
> + return 2;
> + if (cpu_flags2 < 0)
> + return 2;
> + if (cpu_flags1 != cpu_flags2)
> + return 3;
> +#endif
> +
> + return 0;
> +}
Again, once the atomics changes in Libav are merged, the avpriv_atomic_
additions will have to be deleted, and code using it rewritten.
More information about the ffmpeg-devel
mailing list