omx: Fix another race condition in the recursive mutex

Between lock() and begin_recursion() it was possible for another thread to
try to do a recursive_lock(). This would block because the mutex was already
locked(), but not ready for recursive locking yet. unlock() would never
happen in the original thread because it was waiting for the other thread
to finish first.

Happened on the Raspberry Pi.

diff --git a/omx/gstomx.c b/omx/gstomx.c
index d4dede2..1d65f7c 100644 (file)
--- a/omx/gstomx.c
+++ b/omx/gstomx.c
@@ -538,7 +538,7 @@ gst_omx_component_set_state (GstOMXComponent * comp, OMX_STATETYPE state)
 
   g_return_val_if_fail (comp != NULL, OMX_ErrorUndefined);
 
-  gst_omx_rec_mutex_lock (&comp->state_lock);
+  gst_omx_rec_mutex_lock_for_recursion (&comp->state_lock);
   old_state = comp->state;
   GST_DEBUG_OBJECT (comp->parent, "Setting state from %d to %d", old_state,
       state);
@@ -570,7 +570,7 @@ gst_omx_component_set_state (GstOMXComponent * comp, OMX_STATETYPE state)
   /* No need to check if anything has changed here */
 
 done:
-  gst_omx_rec_mutex_unlock (&comp->state_lock);
+  gst_omx_rec_mutex_unlock_for_recursion (&comp->state_lock);
 
   if (err != OMX_ErrorNone) {
     GST_ERROR_OBJECT (comp->parent,
@@ -1076,7 +1076,7 @@ gst_omx_port_release_buffer (GstOMXPort * port, GstOMXBuffer * buf)
   GST_DEBUG_OBJECT (comp->parent, "Releasing buffer %p (%p) to port %u",
       buf, buf->omx_buf->pBuffer, port->index);
 
-  gst_omx_rec_mutex_lock (&port->port_lock);
+  gst_omx_rec_mutex_lock_for_recursion (&port->port_lock);
 
   if (port->port_def.eDir == OMX_DirInput) {
     /* Reset all flags, some implementations don't
@@ -1120,7 +1120,7 @@ gst_omx_port_release_buffer (GstOMXPort * port, GstOMXBuffer * buf)
       buf, port->index, gst_omx_error_to_string (err), err);
 
 done:
-  gst_omx_rec_mutex_unlock (&port->port_lock);
+  gst_omx_rec_mutex_unlock_for_recursion (&port->port_lock);
 
   return err;
 }
@@ -1137,7 +1137,7 @@ gst_omx_port_set_flushing (GstOMXPort * port, gboolean flush)
   GST_DEBUG_OBJECT (comp->parent, "Setting port %d to %sflushing",
       port->index, (flush ? "" : "not "));
 
-  gst_omx_rec_mutex_lock (&port->port_lock);
+  gst_omx_rec_mutex_lock_for_recursion (&port->port_lock);
   if (! !flush == ! !port->flushing) {
     GST_DEBUG_OBJECT (comp->parent, "Port %u was %sflushing already",
         port->index, (flush ? "" : "not "));
@@ -1275,7 +1275,7 @@ gst_omx_port_set_flushing (GstOMXPort * port, gboolean flush)
 done:
   GST_DEBUG_OBJECT (comp->parent, "Set port %u to %sflushing: %s (0x%08x)",
       port->index, (flush ? "" : "not "), gst_omx_error_to_string (err), err);
-  gst_omx_rec_mutex_unlock (&port->port_lock);
+  gst_omx_rec_mutex_unlock_for_recursion (&port->port_lock);
 
   return err;
 
@@ -1421,9 +1421,9 @@ gst_omx_port_allocate_buffers (GstOMXPort * port)
 
   g_return_val_if_fail (port != NULL, OMX_ErrorUndefined);
 
-  gst_omx_rec_mutex_lock (&port->port_lock);
+  gst_omx_rec_mutex_lock_for_recursion (&port->port_lock);
   err = gst_omx_port_allocate_buffers_unlocked (port);
-  gst_omx_rec_mutex_unlock (&port->port_lock);
+  gst_omx_rec_mutex_unlock_for_recursion (&port->port_lock);
 
   return err;
 }
@@ -1515,9 +1515,9 @@ gst_omx_port_deallocate_buffers (GstOMXPort * port)
 
   g_return_val_if_fail (port != NULL, OMX_ErrorUndefined);
 
-  gst_omx_rec_mutex_lock (&port->port_lock);
+  gst_omx_rec_mutex_lock_for_recursion (&port->port_lock);
   err = gst_omx_port_deallocate_buffers_unlocked (port);
-  gst_omx_rec_mutex_unlock (&port->port_lock);
+  gst_omx_rec_mutex_unlock_for_recursion (&port->port_lock);
 
   return err;
 }
@@ -1721,9 +1721,9 @@ gst_omx_port_set_enabled (GstOMXPort * port, gboolean enabled)
 
   g_return_val_if_fail (port != NULL, OMX_ErrorUndefined);
 
-  gst_omx_rec_mutex_lock (&port->port_lock);
+  gst_omx_rec_mutex_lock_for_recursion (&port->port_lock);
   err = gst_omx_port_set_enabled_unlocked (port, enabled);
-  gst_omx_rec_mutex_unlock (&port->port_lock);
+  gst_omx_rec_mutex_unlock_for_recursion (&port->port_lock);
 
   return err;
 }
@@ -1762,7 +1762,7 @@ gst_omx_port_reconfigure (GstOMXPort * port)
 
   GST_DEBUG_OBJECT (comp->parent, "Reconfiguring port %u", port->index);
 
-  gst_omx_rec_mutex_lock (&port->port_lock);
+  gst_omx_rec_mutex_lock_for_recursion (&port->port_lock);
 
   if (!port->settings_changed)
     goto done;
@@ -1809,7 +1809,8 @@ done:
   GST_DEBUG_OBJECT (comp->parent, "Reconfigured port %u: %s (0x%08x)",
       port->index, gst_omx_error_to_string (err), err);
 
-  gst_omx_rec_mutex_unlock (&port->port_lock);
+  gst_omx_rec_mutex_unlock_for_recursion (&port->port_lock);
+
   return err;
 }
 

diff --git a/omx/gstomxrecmutex.c b/omx/gstomxrecmutex.c
index bc4c9cb..6972bc2 100644 (file)
--- a/omx/gstomxrecmutex.c
+++ b/omx/gstomxrecmutex.c
@@ -51,6 +51,31 @@ gst_omx_rec_mutex_unlock (GstOMXRecMutex * mutex)
   g_mutex_unlock (&mutex->lock);
 }
 
+void
+gst_omx_rec_mutex_lock_for_recursion (GstOMXRecMutex * mutex)
+{
+  gboolean exchanged;
+
+  g_mutex_lock (&mutex->lock);
+  exchanged =
+      g_atomic_int_compare_and_exchange (&mutex->recursion_pending, FALSE,
+      TRUE);
+  g_assert (exchanged);
+}
+
+void
+gst_omx_rec_mutex_unlock_for_recursion (GstOMXRecMutex * mutex)
+{
+  gboolean exchanged;
+
+  exchanged =
+      g_atomic_int_compare_and_exchange (&mutex->recursion_pending, TRUE,
+      FALSE);
+  g_assert (exchanged);
+  g_cond_broadcast (&mutex->recursion_wait_cond);
+  g_mutex_unlock (&mutex->lock);
+}
+
 /* must be called with mutex->lock taken */
 void
 gst_omx_rec_mutex_begin_recursion (GstOMXRecMutex * mutex)
@@ -62,6 +87,11 @@ gst_omx_rec_mutex_begin_recursion (GstOMXRecMutex * mutex)
       g_atomic_int_compare_and_exchange (&mutex->recursion_allowed, FALSE,
       TRUE);
   g_assert (exchanged);
+  exchanged =
+      g_atomic_int_compare_and_exchange (&mutex->recursion_pending, TRUE,
+      FALSE);
+  g_assert (exchanged);
+  g_cond_broadcast (&mutex->recursion_wait_cond);
   g_mutex_unlock (&mutex->recursion_lock);
 }
 
@@ -76,6 +106,10 @@ gst_omx_rec_mutex_end_recursion (GstOMXRecMutex * mutex)
       g_atomic_int_compare_and_exchange (&mutex->recursion_allowed, TRUE,
       FALSE);
   g_assert (exchanged);
+  exchanged =
+      g_atomic_int_compare_and_exchange (&mutex->recursion_pending, FALSE,
+      TRUE);
+  g_assert (exchanged);
   g_mutex_unlock (&mutex->recursion_lock);
 }
 
@@ -84,9 +118,15 @@ gst_omx_rec_mutex_recursive_lock (GstOMXRecMutex * mutex)
 {
   g_mutex_lock (&mutex->recursion_lock);
   if (!g_atomic_int_get (&mutex->recursion_allowed)) {
-    /* no recursion allowed, lock the proper mutex */
-    g_mutex_unlock (&mutex->recursion_lock);
-    g_mutex_lock (&mutex->lock);
+    /* If recursion is pending wait until it happened */
+    while (g_atomic_int_get (&mutex->recursion_pending))
+      g_cond_wait (&mutex->recursion_wait_cond, &mutex->recursion_lock);
+
+    if (!g_atomic_int_get (&mutex->recursion_allowed)) {
+      /* no recursion allowed, lock the proper mutex */
+      g_mutex_lock (&mutex->lock);
+      g_mutex_unlock (&mutex->recursion_lock);
+    }
   }
 }
 

diff --git a/omx/gstomxrecmutex.h b/omx/gstomxrecmutex.h
index 4a5a0ec..92aec39 100644 (file)
--- a/omx/gstomxrecmutex.h
+++ b/omx/gstomxrecmutex.h
@@ -92,6 +92,12 @@ struct _GstOMXRecMutex {
    * This variable is protected by both locks.
    */
   volatile gint recursion_allowed;
+
+  /* Indicates whether lock is locked and recursion
+   * will be allowed soon
+   */
+  volatile gint recursion_pending;
+  GCond recursion_wait_cond;
 };
 
 void            gst_omx_rec_mutex_init (GstOMXRecMutex * mutex);
@@ -100,6 +106,9 @@ void            gst_omx_rec_mutex_clear (GstOMXRecMutex * mutex);
 void            gst_omx_rec_mutex_lock (GstOMXRecMutex * mutex);
 void            gst_omx_rec_mutex_unlock (GstOMXRecMutex * mutex);
 
+void            gst_omx_rec_mutex_lock_for_recursion (GstOMXRecMutex * mutex);
+void            gst_omx_rec_mutex_unlock_for_recursion (GstOMXRecMutex * mutex);
+
 void            gst_omx_rec_mutex_begin_recursion (GstOMXRecMutex * mutex);
 void            gst_omx_rec_mutex_end_recursion (GstOMXRecMutex * mutex);