From 14fd7e0884334ee7f8784b0f84b9c6228303f009 Mon Sep 17 00:00:00 2001
From: "Jan Alexander Steffens (heftig)" <jsteffens@make.tv>
Date: Fri, 14 Feb 2020 14:26:27 +0100
Subject: [PATCH] rtmp2: Lock self->lock before OBJECT_LOCK

OBJECT_LOCK is used to protect property access only. self->lock is
used to access the RtmpConnection, mostly between the streaming thread
and the loop thread.

To avoid deadlocks involving these two locks, we obey a lock order:
If both self->lock and OBJECT_LOCK are needed, self->lock must be locked
first. Clarify this.
---
 gst/rtmp2/gstrtmp2sink.c | 25 +++++++++++++++----------
 gst/rtmp2/gstrtmp2src.c  | 22 +++++++++++++---------
 2 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/gst/rtmp2/gstrtmp2sink.c b/gst/rtmp2/gstrtmp2sink.c
index 50201e6..c1db054 100644
--- a/gst/rtmp2/gstrtmp2sink.c
+++ b/gst/rtmp2/gstrtmp2sink.c
@@ -66,11 +66,13 @@ typedef struct
   gboolean async_connect;
   guint peak_kbps;
 
-  /* stuff */
-  gboolean running, flushing;
+  /* If both self->lock and OBJECT_LOCK are needed,
+   * self->lock must be taken first */
   GMutex lock;
   GCond cond;
 
+  gboolean running, flushing;
+
   GstTask *task;
   GRecMutex task_lock;
 
@@ -309,11 +311,12 @@ gst_rtmp2_sink_set_property (GObject * object, guint property_id,
       GST_OBJECT_UNLOCK (self);
       break;
     case PROP_PEAK_KBPS:
+      g_mutex_lock (&self->lock);
+
       GST_OBJECT_LOCK (self);
       self->peak_kbps = g_value_get_uint (value);
       GST_OBJECT_UNLOCK (self);
 
-      g_mutex_lock (&self->lock);
       set_pacing_rate (self);
       g_mutex_unlock (&self->lock);
       break;
@@ -838,38 +841,40 @@ gst_rtmp2_sink_task_func (gpointer user_data)
   GTask *connector;
 
   GST_DEBUG_OBJECT (self, "gst_rtmp2_sink_task starting");
-
   g_mutex_lock (&self->lock);
+
   context = self->context = g_main_context_new ();
   g_main_context_push_thread_default (context);
   loop = self->loop = g_main_loop_new (context, TRUE);
   connector = g_task_new (self, self->cancellable, connect_task_done, NULL);
+
   GST_OBJECT_LOCK (self);
   gst_rtmp_client_connect_async (&self->location, self->cancellable,
       client_connect_done, connector);
   GST_OBJECT_UNLOCK (self);
-  g_mutex_unlock (&self->lock);
 
+  /* Run loop */
+  g_mutex_unlock (&self->lock);
   g_main_loop_run (loop);
-
   g_mutex_lock (&self->lock);
+
   g_clear_pointer (&self->loop, g_main_loop_unref);
   g_clear_pointer (&self->connection, gst_rtmp_connection_close_and_unref);
   g_cond_broadcast (&self->cond);
-  g_mutex_unlock (&self->lock);
 
+  /* Run loop cleanup */
+  g_mutex_unlock (&self->lock);
   while (g_main_context_pending (context)) {
     GST_DEBUG_OBJECT (self, "iterating main context to clean up");
     g_main_context_iteration (context, FALSE);
   }
-
   g_main_context_pop_thread_default (context);
-
   g_mutex_lock (&self->lock);
+
   g_clear_pointer (&self->context, g_main_context_unref);
   g_ptr_array_set_size (self->headers, 0);
-  g_mutex_unlock (&self->lock);
 
+  g_mutex_unlock (&self->lock);
   GST_DEBUG_OBJECT (self, "gst_rtmp2_sink_task exiting");
 }
 
diff --git a/gst/rtmp2/gstrtmp2src.c b/gst/rtmp2/gstrtmp2src.c
index 0e7598e..305edcb 100644
--- a/gst/rtmp2/gstrtmp2src.c
+++ b/gst/rtmp2/gstrtmp2src.c
@@ -60,11 +60,13 @@ typedef struct
   GstRtmpLocation location;
   gboolean async_connect;
 
-  /* stuff */
-  gboolean running, flushing;
+  /* If both self->lock and OBJECT_LOCK are needed,
+   * self->lock must be taken first */
   GMutex lock;
   GCond cond;
 
+  gboolean running, flushing;
+
   GstTask *task;
   GRecMutex task_lock;
 
@@ -599,38 +601,40 @@ gst_rtmp2_src_task_func (gpointer user_data)
   GTask *connector;
 
   GST_DEBUG_OBJECT (self, "gst_rtmp2_src_task starting");
-
   g_mutex_lock (&self->lock);
+
   context = self->context = g_main_context_new ();
   g_main_context_push_thread_default (context);
   loop = self->loop = g_main_loop_new (context, TRUE);
   connector = g_task_new (self, self->cancellable, connect_task_done, NULL);
+
   GST_OBJECT_LOCK (self);
   gst_rtmp_client_connect_async (&self->location, self->cancellable,
       client_connect_done, connector);
   GST_OBJECT_UNLOCK (self);
-  g_mutex_unlock (&self->lock);
 
+  /* Run loop */
+  g_mutex_unlock (&self->lock);
   g_main_loop_run (loop);
-
   g_mutex_lock (&self->lock);
+
   g_clear_pointer (&self->loop, g_main_loop_unref);
   g_clear_pointer (&self->connection, gst_rtmp_connection_close_and_unref);
   g_cond_broadcast (&self->cond);
-  g_mutex_unlock (&self->lock);
 
+  /* Run loop cleanup */
+  g_mutex_unlock (&self->lock);
   while (g_main_context_pending (context)) {
     GST_DEBUG_OBJECT (self, "iterating main context to clean up");
     g_main_context_iteration (context, FALSE);
   }
-
   g_main_context_pop_thread_default (context);
-
   g_mutex_lock (&self->lock);
+
   g_clear_pointer (&self->context, g_main_context_unref);
   gst_buffer_replace (&self->message, NULL);
-  g_mutex_unlock (&self->lock);
 
+  g_mutex_unlock (&self->lock);
   GST_DEBUG_OBJECT (self, "gst_rtmp2_src_task exiting");
 }
 
-- 
2.7.4