diff mbox

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

Message ID 1481049685-76564-1-git-send-email-wtc@google.com
State Accepted
Commit fed50c4304eecb352e29ce789cdb96ea84d6162f
Headers show

Commit Message

Wan-Teh Chang Dec. 6, 2016, 6:41 p.m. UTC 
Make the one-time initialization in av_get_cpu_flags() thread-safe. The
static variable |cpu_flags| in libavutil/cpu.c is 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 can be verified by running the libavutil/tests/cpu_init.c test
program under ThreadSanitizer:
    ./configure --toolchain=clang-tsan
    make libavutil/tests/cpu_init
    libavutil/tests/cpu_init

There should be no warnings from ThreadSanitizer.

Co-author: Dmitry Vyukov of Google, which suggested the data race fix.

Signed-off-by: Wan-Teh Chang <wtc@google.com>
---
 libavutil/cpu.c | 12 +++++++-----
 libavutil/cpu.h |  2 --
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Wan-Teh Chang Dec. 7, 2016, 2:09 a.m. UTC | #1 
On Tue, Dec 6, 2016 at 10:41 AM, Wan-Teh Chang <wtc@google.com> wrote:
> Make the one-time initialization in av_get_cpu_flags() thread-safe. The
> static variable |cpu_flags| in libavutil/cpu.c is 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 can be verified by running the libavutil/tests/cpu_init.c test
> program under ThreadSanitizer:
>     ./configure --toolchain=clang-tsan
>     make libavutil/tests/cpu_init
>     libavutil/tests/cpu_init
>
> There should be no warnings from ThreadSanitizer.
>
> Co-author: Dmitry Vyukov of Google, which suggested the data race fix.

Please ignore this patch. I just noticed that the last line of the
commit message has a grammatical error ("which" should be changed to
"who").

I will correct that and send a new patch.

Wan-Teh Chang
diff mbox

Patch

diff --git a/libavutil/cpu.c b/libavutil/cpu.c
index 73317c4..16e0c92 100644
--- a/libavutil/cpu.c
+++ b/libavutil/cpu.c
@@ -17,6 +17,7 @@ 
  */
 
 #include <stdint.h>
+#include <stdatomic.h>
 
 #include "cpu.h"
 #include "cpu_internal.h"
@@ -44,7 +45,7 @@ 
 #include <unistd.h>
 #endif
 
-static int cpu_flags = -1;
+static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1);
 
 static int get_cpu_flags(void)
 {
@@ -82,22 +83,23 @@  void av_force_cpu_flags(int arg){
         arg |= AV_CPU_FLAG_MMX;
     }
 
-    cpu_flags = arg;
+    atomic_store_explicit(&cpu_flags, arg, memory_order_relaxed);
 }
 
 int av_get_cpu_flags(void)
 {
-    int flags = cpu_flags;
+    int flags = atomic_load_explicit(&cpu_flags, memory_order_relaxed);
     if (flags == -1) {
         flags = get_cpu_flags();
-        cpu_flags = flags;
+        atomic_store_explicit(&cpu_flags, flags, memory_order_relaxed);
     }
     return flags;
 }
 
 void av_set_cpu_flags_mask(int mask)
 {
-    cpu_flags = get_cpu_flags() & mask;
+    atomic_store_explicit(&cpu_flags, get_cpu_flags() & mask,
+                          memory_order_relaxed);
 }
 
 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);