[pulseaudio-discuss] [PATCH] once: Fix race causing pa_once to sometimes run twice

[David Henningsson]

There was a race in the existing code that could cause the pa_once code
to be run twice, see:
http://lists.freedesktop.org/archives/pulseaudio-discuss/2012-April/013354.html

Therefore the existing implementation was rewritten to instead look like
the reference implementation here:
http://www.hpl.hp.com/research/linux/atomic_ops/example.php4

Signed-off-by: David Henningsson <david.henningsson at canonical.com>
---

We had some discussion about this a little while ago, but no patch was merged,
so the bug was left unfixed.
If there are no objections I'll push this one.

 src/pulsecore/once.c |   43 +++++++++++++------------------------------
 src/pulsecore/once.h |    8 ++++----
 2 files changed, 17 insertions(+), 34 deletions(-)

diff --git a/src/pulsecore/once.c b/src/pulsecore/once.c
index 4e509e0..30b35a6 100644
--- a/src/pulsecore/once.c
+++ b/src/pulsecore/once.c
@@ -24,46 +24,33 @@
 #endif
 
 #include <pulsecore/macro.h>
-#include <pulsecore/mutex.h>
 
 #include "once.h"
 
+/* See http://www.hpl.hp.com/research/linux/atomic_ops/example.php4 for the
+ * reference algorithm used here. */
+
 pa_bool_t pa_once_begin(pa_once *control) {
+    pa_mutex *m;
+
     pa_assert(control);
 
     if (pa_atomic_load(&control->done))
         return FALSE;
 
-    pa_atomic_inc(&control->ref);
-
     /* Caveat: We have to make sure that the once func has completed
      * before returning, even if the once func is not actually
      * executed by us. Hence the awkward locking. */
 
-    for (;;) {
-        pa_mutex *m;
-
-        if ((m = pa_atomic_ptr_load(&control->mutex))) {
-
-            /* The mutex is stored in locked state, hence let's just
-             * wait until it is unlocked */
-            pa_mutex_lock(m);
-
-            pa_assert(pa_atomic_load(&control->done));
-
-            pa_once_end(control);
-            return FALSE;
-        }
-
-        pa_assert_se(m = pa_mutex_new(FALSE, FALSE));
-        pa_mutex_lock(m);
-
-        if (pa_atomic_ptr_cmpxchg(&control->mutex, NULL, m))
-            return TRUE;
+    m = pa_static_mutex_get(&control->mutex, FALSE, FALSE);
+    pa_mutex_lock(m);
 
+    if (pa_atomic_load(&control->done)) {
         pa_mutex_unlock(m);
-        pa_mutex_free(m);
+        return FALSE;
     }
+
+    return TRUE;
 }
 
 void pa_once_end(pa_once *control) {
@@ -71,15 +58,11 @@ void pa_once_end(pa_once *control) {
 
     pa_assert(control);
 
+    pa_assert(!pa_atomic_load(&control->done));
     pa_atomic_store(&control->done, 1);
 
-    pa_assert_se(m = pa_atomic_ptr_load(&control->mutex));
+    m = pa_static_mutex_get(&control->mutex, FALSE, FALSE);
     pa_mutex_unlock(m);
-
-    if (pa_atomic_dec(&control->ref) <= 1) {
-        pa_assert_se(pa_atomic_ptr_cmpxchg(&control->mutex, m, NULL));
-        pa_mutex_free(m);
-    }
 }
 
 /* Not reentrant -- how could it be? */
diff --git a/src/pulsecore/once.h b/src/pulsecore/once.h
index edc8188..a478d1f 100644
--- a/src/pulsecore/once.h
+++ b/src/pulsecore/once.h
@@ -23,16 +23,16 @@
 ***/
 
 #include <pulsecore/atomic.h>
+#include <pulsecore/mutex.h>
 
 typedef struct pa_once {
-    pa_atomic_ptr_t mutex;
-    pa_atomic_t ref, done;
+    pa_static_mutex mutex;
+    pa_atomic_t done;
 } pa_once;
 
 #define PA_ONCE_INIT                                                    \
     {                                                                   \
-        .mutex = PA_ATOMIC_PTR_INIT(NULL),                              \
-        .ref = PA_ATOMIC_INIT(0),                                       \
+        .mutex = PA_STATIC_MUTEX_INIT,                                  \
         .done = PA_ATOMIC_INIT(0)                                       \
     }
 
-- 
1.7.9.5