vlc | branch: master | Rémi Denis-Courmont <[email protected]> | Wed Jun 1 21:47:38 2016 +0300| [b269f60e182c9a26f046518cb52439f88b4a3c05] | committer: Rémi Denis-Courmont
threads: fix race in vlc_cond_wait() Could lose wake-up if vlc_cond_wait() in one thread, then vlc_cond_signal() in anotherthread, then vlc_cond_wait() in a third thread. > http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=b269f60e182c9a26f046518cb52439f88b4a3c05 --- include/vlc_threads.h | 2 +- src/android/thread.c | 2 +- src/misc/threads.c | 46 ++++++++++++++++++++++++++++++---------------- src/win32/thread.c | 2 +- 4 files changed, 33 insertions(+), 19 deletions(-) diff --git a/include/vlc_threads.h b/include/vlc_threads.h index 0f4aefe..33f2a8a 100644 --- a/include/vlc_threads.h +++ b/include/vlc_threads.h @@ -326,7 +326,7 @@ typedef struct vlc_timer *vlc_timer_t; #ifdef LIBVLC_NEED_CONDVAR typedef struct { - int value; + unsigned value; } vlc_cond_t; # define VLC_STATIC_COND { 0 } #endif diff --git a/src/android/thread.c b/src/android/thread.c index be18204..8e12115 100644 --- a/src/android/thread.c +++ b/src/android/thread.c @@ -317,7 +317,7 @@ void vlc_cancel (vlc_thread_t thread_id) addr = thread_id->wait.addr; if (addr != NULL) { - atomic_fetch_and_explicit(addr, -2, memory_order_relaxed); + atomic_fetch_or_explicit(addr, 1, memory_order_relaxed); vlc_addr_broadcast(addr); } vlc_mutex_unlock(&thread_id->wait.lock); diff --git a/src/misc/threads.c b/src/misc/threads.c index 044296f..8bdcb1f 100644 --- a/src/misc/threads.c +++ b/src/misc/threads.c @@ -93,14 +93,14 @@ void (msleep)(mtime_t delay) #ifdef LIBVLC_NEED_CONDVAR #include <stdalign.h> -static inline atomic_int *vlc_cond_value(vlc_cond_t *cond) +static inline atomic_uint *vlc_cond_value(vlc_cond_t *cond) { /* XXX: ugly but avoids including vlc_atomic.h in vlc_threads.h */ - static_assert (sizeof (cond->value) <= sizeof (atomic_int), + static_assert (sizeof (cond->value) <= sizeof (atomic_uint), "Size mismatch!"); - static_assert ((alignof (cond->value) % alignof (atomic_int)) == 0, + static_assert ((alignof (cond->value) % alignof (atomic_uint)) == 0, "Alignment mismatch"); - return (atomic_int *)&cond->value; + return (atomic_uint *)&cond->value; } void vlc_cond_init(vlc_cond_t *cond) @@ -127,30 +127,36 @@ void vlc_cond_destroy(vlc_cond_t *cond) void vlc_cond_signal(vlc_cond_t *cond) { /* Probably the best documented approach is that of Bionic: increment - * the futex here, and simply load the value in cond_wait(). This has a bug + * the futex here, and simply load the value in cnd_wait(). This has a bug * as unlikely as well-known: signals get lost if the futex is incremented * an exact multiple of 2^(CHAR_BIT * sizeof (int)) times. * - * Here, we simply clear the low order bit, while cond_wait() sets it. - * It makes cond_wait() somewhat slower, but it should be free of bugs, - * leaves the other bits free for other uses (e.g. flags). + * A different presumably bug-free solution is used here: + * - cnd_signal() sets the futex to the equal-or-next odd value, while + * - cnd_wait() sets the futex to the equal-or-next even value. **/ - atomic_fetch_and_explicit(vlc_cond_value(cond), -2, memory_order_relaxed); - /* We have to wake the futex even if the low order bit is cleared, as there - * could be more than one thread queued, not all of them already awoken. */ + atomic_fetch_or_explicit(vlc_cond_value(cond), 1, memory_order_relaxed); vlc_addr_signal(&cond->value); } void vlc_cond_broadcast(vlc_cond_t *cond) { - atomic_fetch_and_explicit(vlc_cond_value(cond), -2, memory_order_relaxed); + atomic_fetch_or_explicit(vlc_cond_value(cond), 1, memory_order_relaxed); vlc_addr_broadcast(&cond->value); } void vlc_cond_wait(vlc_cond_t *cond, vlc_mutex_t *mutex) { - int value = atomic_fetch_or_explicit(vlc_cond_value(cond), 1, - memory_order_relaxed) | 1; + unsigned value = atomic_load_explicit(vlc_cond_value(cond), + memory_order_relaxed); + while (value & 1) + { + if (atomic_compare_exchange_weak_explicit(vlc_cond_value(cond), &value, + value + 1, + memory_order_relaxed, + memory_order_relaxed)) + value++; + } vlc_cancel_addr_prepare(&cond->value); vlc_mutex_unlock(mutex); @@ -164,8 +170,16 @@ void vlc_cond_wait(vlc_cond_t *cond, vlc_mutex_t *mutex) static int vlc_cond_wait_delay(vlc_cond_t *cond, vlc_mutex_t *mutex, mtime_t delay) { - int value = atomic_fetch_or_explicit(vlc_cond_value(cond), 1, - memory_order_relaxed) | 1; + unsigned value = atomic_load_explicit(vlc_cond_value(cond), + memory_order_relaxed); + while (value & 1) + { + if (atomic_compare_exchange_weak_explicit(vlc_cond_value(cond), &value, + value + 1, + memory_order_relaxed, + memory_order_relaxed)) + value++; + } vlc_cancel_addr_prepare(&cond->value); vlc_mutex_unlock(mutex); diff --git a/src/win32/thread.c b/src/win32/thread.c index 8d74d6f..88aaa5b 100644 --- a/src/win32/thread.c +++ b/src/win32/thread.c @@ -640,7 +640,7 @@ void vlc_cancel (vlc_thread_t th) EnterCriticalSection(&th->wait.lock); if (th->wait.addr != NULL) { - atomic_fetch_and_explicit(th->wait.addr, -2, memory_order_relaxed); + atomic_fetch_or_explicit(th->wait.addr, 1, memory_order_relaxed); vlc_addr_broadcast(th->wait.addr); } LeaveCriticalSection(&th->wait.lock); _______________________________________________ vlc-commits mailing list [email protected] https://mailman.videolan.org/listinfo/vlc-commits
