From 4b96294f19e5f6f6423e1a08119b443d7fa6ab13 Mon Sep 17 00:00:00 2001
From: Johan Sternerup <johast@axis.com>
Date: Thu, 1 Dec 2022 13:28:16 +0100
Subject: [PATCH] webrtc: Fix possible use-after-free of GstWebRTCICETransport

Because of the asynchronous resolving of mDNS ICE candidates it is
possible that GstWebRTCICE outlives webrtcbin. This in turn prolongs
the lifetime of the GstWebRTCNiceStream objects via refs in
nice_stream_map. Thus the GstWebRTCICETransport objects held in
GstWebRTCNiceStream may be invalid at the time they are accessed by
the _on_candidate_gathering_done() callback since GstWebRTCNiceStream
doesn't take a reference to them. Doing so would create a circular
reference, so instead this commit introduces weak references to the
transport objects and then we can check if the objects are valid before
accessing them.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3502>
---
 .../gst-libs/gst/webrtc/nice/nicestream.c          | 93 ++++++++++++++--------
 1 file changed, 62 insertions(+), 31 deletions(-)

diff --git a/subprojects/gst-plugins-bad/gst-libs/gst/webrtc/nice/nicestream.c b/subprojects/gst-plugins-bad/gst-libs/gst/webrtc/nice/nicestream.c
index a0dc51b..cda1c13 100644
--- a/subprojects/gst-plugins-bad/gst-libs/gst/webrtc/nice/nicestream.c
+++ b/subprojects/gst-plugins-bad/gst-libs/gst/webrtc/nice/nicestream.c
@@ -80,6 +80,21 @@ gst_webrtc_nice_stream_get_property (GObject * object, guint prop_id,
   }
 }
 
+static GWeakRef *
+weak_new (gpointer object)
+{
+  GWeakRef *weak = g_new0 (GWeakRef, 1);
+  g_weak_ref_init (weak, object);
+  return weak;
+}
+
+static void
+weak_free (GWeakRef * weak)
+{
+  g_weak_ref_clear (weak);
+  g_free (weak);
+}
+
 static void
 gst_webrtc_nice_stream_finalize (GObject * object)
 {
@@ -99,6 +114,7 @@ gst_webrtc_nice_stream_finalize (GObject * object)
     gst_object_unref (ice);
   }
 
+  g_list_foreach (stream->priv->transports, (GFunc) weak_free, NULL);
   g_list_free (stream->priv->transports);
   stream->priv->transports = NULL;
 
@@ -107,6 +123,15 @@ gst_webrtc_nice_stream_finalize (GObject * object)
   G_OBJECT_CLASS (parent_class)->finalize (object);
 }
 
+static GList *
+_delete_transport (GList ** transports, GList * link)
+{
+  GList *next = link->next;
+  weak_free (link->data);
+  *transports = g_list_delete_link (*transports, link);
+  return next;
+}
+
 static void
 _on_candidate_gathering_done (NiceAgent * agent, guint stream_id,
     GWeakRef * ice_weak)
@@ -125,10 +150,15 @@ _on_candidate_gathering_done (NiceAgent * agent, guint stream_id,
   ice->priv->gathered = TRUE;
 
   for (l = ice->priv->transports; l; l = l->next) {
-    GstWebRTCICETransport *ice = l->data;
-
-    gst_webrtc_ice_transport_gathering_state_change (ice,
-        GST_WEBRTC_ICE_GATHERING_STATE_COMPLETE);
+    GstWebRTCICETransport *trans = g_weak_ref_get (l->data);
+
+    if (trans) {
+      gst_webrtc_ice_transport_gathering_state_change (trans,
+          GST_WEBRTC_ICE_GATHERING_STATE_COMPLETE);
+      g_object_unref (trans);
+    } else {
+      l = _delete_transport (&ice->priv->transports, l);
+    }
   }
 
 cleanup:
@@ -145,37 +175,28 @@ gst_webrtc_nice_stream_find_transport (GstWebRTCICEStream * stream,
   GstWebRTCNiceStream *nice_stream = GST_WEBRTC_NICE_STREAM (stream);
 
   for (l = nice_stream->priv->transports; l; l = l->next) {
-    GstWebRTCICETransport *trans = l->data;
-    g_object_get (trans, "component", &trans_comp, NULL);
-
-    if (component == trans_comp)
-      return gst_object_ref (trans);
+    GstWebRTCICETransport *trans = g_weak_ref_get (l->data);
+    if (trans) {
+      g_object_get (trans, "component", &trans_comp, NULL);
+
+      if (component == trans_comp)
+        return trans;
+      else
+        gst_object_unref (trans);
+    } else {
+      l = _delete_transport (&nice_stream->priv->transports, l);
+    }
   }
 
   ret =
       GST_WEBRTC_ICE_TRANSPORT (gst_webrtc_nice_transport_new (nice_stream,
           component));
   nice_stream->priv->transports =
-      g_list_prepend (nice_stream->priv->transports, ret);
+      g_list_prepend (nice_stream->priv->transports, weak_new (ret));
 
   return ret;
 }
 
-static GWeakRef *
-weak_new (GstWebRTCNiceStream * stream)
-{
-  GWeakRef *weak = g_new0 (GWeakRef, 1);
-  g_weak_ref_init (weak, stream);
-  return weak;
-}
-
-static void
-weak_free (GWeakRef * weak)
-{
-  g_weak_ref_clear (weak);
-  g_free (weak);
-}
-
 static void
 gst_webrtc_nice_stream_constructed (GObject * object)
 {
@@ -214,10 +235,15 @@ gst_webrtc_nice_stream_gather_candidates (GstWebRTCICEStream * stream)
     return TRUE;
 
   for (l = nice_stream->priv->transports; l; l = l->next) {
-    GstWebRTCICETransport *trans = l->data;
-
-    gst_webrtc_ice_transport_gathering_state_change (trans,
-        GST_WEBRTC_ICE_GATHERING_STATE_GATHERING);
+    GstWebRTCICETransport *trans = g_weak_ref_get (l->data);
+
+    if (trans) {
+      gst_webrtc_ice_transport_gathering_state_change (trans,
+          GST_WEBRTC_ICE_GATHERING_STATE_GATHERING);
+      g_object_unref (trans);
+    } else {
+      l = _delete_transport (&nice_stream->priv->transports, l);
+    }
   }
 
   ice = GST_WEBRTC_ICE (g_weak_ref_get (&nice_stream->priv->ice_weak));
@@ -248,9 +274,14 @@ gst_webrtc_nice_stream_gather_candidates (GstWebRTCICEStream * stream)
   }
 
   for (l = nice_stream->priv->transports; l; l = l->next) {
-    GstWebRTCNiceTransport *trans = l->data;
+    GstWebRTCNiceTransport *trans = g_weak_ref_get (l->data);
 
-    gst_webrtc_nice_transport_update_buffer_size (trans);
+    if (trans) {
+      gst_webrtc_nice_transport_update_buffer_size (trans);
+      g_object_unref (trans);
+    } else {
+      l = _delete_transport (&nice_stream->priv->transports, l);
+    }
   }
 
 cleanup:
-- 
2.7.4