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

Wan-Teh Chang wtc at google.com
Tue Nov 22 01:37:49 EET 2016


Make the one-time initialization in av_get_cpu_flags() thread-safe.
The fix assumes -1 is an invalid value for cpu flags.

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

The data race fix is written by Dmitry Vyukov of Google.

Signed-off-by: Wan-Teh Chang <wtc at google.com>
---
 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/tests/atomic.c | 13 +++++++++++++
 7 files changed, 180 insertions(+), 21 deletions(-)

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..cbcca4c 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();
+    /* Not reached. */
+}
 
 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/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;
 }
-- 
2.8.0.rc3.226.g39d4020



More information about the ffmpeg-devel mailing list