From 2170017a1cd033b6f28e16476921022712a522d8 Mon Sep 17 00:00:00 2001 From: Wan-Teh Chang Date: Wed, 7 Dec 2016 16:16:02 -0800 Subject: [PATCH] avutil: fix data race in av_get_cpu_flags() Make the one-time initialization in av_get_cpu_flags() thread-safe. The static variables |flags|, |cpuflags_mask|, 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. Remove the |checked| variable because the invalid value of -1 for |flags| can be used to indicate the same condition. Rename |flags| to |cpu_flags| and move it to file scope. 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, who suggested the data race fix. Signed-off-by: Wan-Teh Chang --- libavutil/Makefile | 2 ++ libavutil/cpu.c | 34 ++++++++++---------- libavutil/cpu.h | 2 -- libavutil/tests/.gitignore | 1 + libavutil/tests/cpu_init.c | 65 ++++++++++++++++++++++++++++++++++++++ tests/fate/libavutil.mak | 6 ++++ 6 files changed, 92 insertions(+), 18 deletions(-) create mode 100644 libavutil/tests/cpu_init.c diff --git a/libavutil/Makefile b/libavutil/Makefile index 28372c9bef..f34c79950e 100644 --- a/libavutil/Makefile +++ b/libavutil/Makefile @@ -145,3 +145,5 @@ TESTPROGS = adler32 \ sha \ tree \ xtea \ + +TESTPROGS-$(HAVE_THREADS) += cpu_init diff --git a/libavutil/cpu.c b/libavutil/cpu.c index 7d7390a756..0109c9e8d1 100644 --- a/libavutil/cpu.c +++ b/libavutil/cpu.c @@ -17,6 +17,7 @@ */ #include +#include #include "cpu.h" #include "cpu_internal.h" @@ -42,34 +43,35 @@ #include #endif -static int cpuflags_mask = -1, checked; +static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1); -int av_get_cpu_flags(void) +static int get_cpu_flags(void) { - static int flags; - - if (checked) - return flags; - if (ARCH_AARCH64) - flags = ff_get_cpu_flags_aarch64(); + return ff_get_cpu_flags_aarch64(); if (ARCH_ARM) - flags = ff_get_cpu_flags_arm(); + return ff_get_cpu_flags_arm(); if (ARCH_PPC) - flags = ff_get_cpu_flags_ppc(); + return ff_get_cpu_flags_ppc(); if (ARCH_X86) - flags = ff_get_cpu_flags_x86(); - - flags &= cpuflags_mask; - checked = 1; + return ff_get_cpu_flags_x86(); + return 0; +} +int av_get_cpu_flags(void) +{ + int flags = atomic_load_explicit(&cpu_flags, memory_order_relaxed); + if (flags == -1) { + flags = get_cpu_flags(); + atomic_store_explicit(&cpu_flags, flags, memory_order_relaxed); + } return flags; } void av_set_cpu_flags_mask(int mask) { - cpuflags_mask = mask; - checked = 0; + 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 deff4ccf55..c205ee16f6 100644 --- a/libavutil/cpu.h +++ b/libavutil/cpu.h @@ -73,8 +73,6 @@ int av_get_cpu_flags(void); /** * Set a mask on flags returned by av_get_cpu_flags(). * This function is mainly useful for testing. - * - * @warning this function is not thread safe. */ void av_set_cpu_flags_mask(int mask); diff --git a/libavutil/tests/.gitignore b/libavutil/tests/.gitignore index c4d1d065fb..c8f37a7f90 100644 --- a/libavutil/tests/.gitignore +++ b/libavutil/tests/.gitignore @@ -5,6 +5,7 @@ /base64 /blowfish /cpu +/cpu_init /crc /des /eval diff --git a/libavutil/tests/cpu_init.c b/libavutil/tests/cpu_init.c new file mode 100644 index 0000000000..beb95c911f --- /dev/null +++ b/libavutil/tests/cpu_init.c @@ -0,0 +1,65 @@ +/* + * This file is part of Libav. + * + * Libav 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. + * + * Libav 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 Libav; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* + * This test program tests whether the one-time initialization in + * av_get_cpu_flags() has data races. + */ + +#include +#include + +#include "libavutil/cpu.h" +#include "libavutil/thread.h" + +static void *thread_main(void *arg) +{ + int *flags = arg; + + *flags = av_get_cpu_flags(); + return NULL; +} + +int main(void) +{ + 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; + + return 0; +} diff --git a/tests/fate/libavutil.mak b/tests/fate/libavutil.mak index 2824e084eb..8b17411370 100644 --- a/tests/fate/libavutil.mak +++ b/tests/fate/libavutil.mak @@ -25,6 +25,11 @@ fate-cpu: libavutil/tests/cpu$(EXESUF) fate-cpu: CMD = run libavutil/tests/cpu $(CPUFLAGS:%=-c%) $(THREADS:%=-t%) fate-cpu: REF = /dev/null +FATE_LIBAVUTIL-$(HAVE_THREADS) += fate-cpu_init +fate-cpu_init: libavutil/tests/cpu_init$(EXESUF) +fate-cpu_init: CMD = run libavutil/tests/cpu_init +fate-cpu_init: REF = /dev/null + FATE_LIBAVUTIL += fate-crc fate-crc: libavutil/tests/crc$(EXESUF) fate-crc: CMD = run libavutil/tests/crc @@ -73,5 +78,6 @@ FATE_LIBAVUTIL += fate-xtea fate-xtea: libavutil/tests/xtea$(EXESUF) fate-xtea: CMD = run libavutil/tests/xtea +FATE_LIBAVUTIL += $(FATE_LIBAVUTIL-yes) FATE-$(CONFIG_AVUTIL) += $(FATE_LIBAVUTIL) fate-libavutil: $(FATE_LIBAVUTIL)