[vlc-commits] threads: fix race in vlc_cond_wait()

[Rémi Denis-Courmont]

vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | 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);