rtpjitterbuffer: Properly free internal packets queue in finalize()

As we override the GLib item with our own structure, we cannot use any
function from GList or GQueue that would try to free the RTPJitterBufferItem.
In this patch, we move away from g_queue_new() which forces using
g_queue_free(). This this function could use g_slice_free() if there is any items
left in the queue. Passing the wrong size to GSLice may cause data corruption
and crash.

A better approach would be to use a proper intrusive linked list
implementation but that's left as an exercise for the next person
running into crashes caused by this.

Be ware that this regression was introduced 6 years ago in the following
commit [0], the call to flush() looked useless, as there was a g_queue_free()
afterward.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
[0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/commit/479c7642fd953edf1291a0ed4a3d53618418019c

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/573>

diff --git a/gst/rtpmanager/rtpjitterbuffer.c b/gst/rtpmanager/rtpjitterbuffer.c
index 1c54ea8e998f7a6855b9775103039d5feecf6ab0..6ef7df2abfa2f5f3cd4709d9634968fbc21ba412 100644 (file)
--- a/gst/rtpmanager/rtpjitterbuffer.c
+++ b/gst/rtpmanager/rtpjitterbuffer.c
@@ -87,7 +87,7 @@ rtp_jitter_buffer_init (RTPJitterBuffer * jbuf)
 {
   g_mutex_init (&jbuf->clock_lock);
 
-  jbuf->packets = g_queue_new ();
+  g_queue_init (&jbuf->packets);
   jbuf->mode = RTP_JITTER_BUFFER_MODE_SLAVE;
 
   rtp_jitter_buffer_reset_skew (jbuf);
@@ -112,7 +112,10 @@ rtp_jitter_buffer_finalize (GObject * object)
   if (jbuf->pipeline_clock)
     gst_object_unref (jbuf->pipeline_clock);
 
-  g_queue_free (jbuf->packets);
+  /* We cannot use g_queue_clear() as it would pass the wrong size to
+   * g_slice_free() which may lead to data corruption in the slice allocator.
+   */
+  rtp_jitter_buffer_flush (jbuf, NULL, NULL);
 
   g_mutex_clear (&jbuf->clock_lock);
 
@@ -385,7 +388,7 @@ get_buffer_level (RTPJitterBuffer * jbuf)
   guint64 level;
 
   /* first buffer with timestamp */
-  high_buf = (RTPJitterBufferItem *) g_queue_peek_tail_link (jbuf->packets);
+  high_buf = (RTPJitterBufferItem *) g_queue_peek_tail_link (&jbuf->packets);
   while (high_buf) {
     if (high_buf->dts != -1 || high_buf->pts != -1)
       break;
@@ -393,7 +396,7 @@ get_buffer_level (RTPJitterBuffer * jbuf)
     high_buf = (RTPJitterBufferItem *) g_list_previous (high_buf);
   }
 
-  low_buf = (RTPJitterBufferItem *) g_queue_peek_head_link (jbuf->packets);
+  low_buf = (RTPJitterBufferItem *) g_queue_peek_head_link (&jbuf->packets);
   while (low_buf) {
     if (low_buf->dts != -1 || low_buf->pts != -1)
       break;
@@ -678,7 +681,7 @@ no_skew:
 static void
 queue_do_insert (RTPJitterBuffer * jbuf, GList * list, GList * item)
 {
-  GQueue *queue = jbuf->packets;
+  GQueue *queue = &jbuf->packets;
 
   /* It's more likely that the packet was inserted at the tail of the queue */
   if (G_LIKELY (list)) {
@@ -999,7 +1002,7 @@ rtp_jitter_buffer_insert (RTPJitterBuffer * jbuf, RTPJitterBufferItem * item,
   g_return_val_if_fail (jbuf != NULL, FALSE);
   g_return_val_if_fail (item != NULL, FALSE);
 
-  list = jbuf->packets->tail;
+  list = jbuf->packets.tail;
 
   /* no seqnum, simply append then */
   if (item->seqnum == -1)
@@ -1236,7 +1239,7 @@ rtp_jitter_buffer_pop (RTPJitterBuffer * jbuf, gint * percent)
 
   g_return_val_if_fail (jbuf != NULL, NULL);
 
-  queue = jbuf->packets;
+  queue = &jbuf->packets;
 
   item = queue->head;
   if (item) {
@@ -1277,7 +1280,7 @@ rtp_jitter_buffer_peek (RTPJitterBuffer * jbuf)
 {
   g_return_val_if_fail (jbuf != NULL, NULL);
 
-  return (RTPJitterBufferItem *) jbuf->packets->head;
+  return (RTPJitterBufferItem *) jbuf->packets.head;
 }
 
 /**
@@ -1299,7 +1302,7 @@ rtp_jitter_buffer_flush (RTPJitterBuffer * jbuf, GFunc free_func,
   if (free_func == NULL)
     free_func = (GFunc) rtp_jitter_buffer_free_item;
 
-  while ((item = g_queue_pop_head_link (jbuf->packets)))
+  while ((item = g_queue_pop_head_link (&jbuf->packets)))
     free_func ((RTPJitterBufferItem *) item, user_data);
 }
 
@@ -1371,7 +1374,7 @@ rtp_jitter_buffer_num_packets (RTPJitterBuffer * jbuf)
 {
   g_return_val_if_fail (jbuf != NULL, 0);
 
-  return jbuf->packets->length;
+  return jbuf->packets.length;
 }
 
 /**
@@ -1392,8 +1395,8 @@ rtp_jitter_buffer_get_ts_diff (RTPJitterBuffer * jbuf)
 
   g_return_val_if_fail (jbuf != NULL, 0);
 
-  high_buf = (RTPJitterBufferItem *) g_queue_peek_tail_link (jbuf->packets);
-  low_buf = (RTPJitterBufferItem *) g_queue_peek_head_link (jbuf->packets);
+  high_buf = (RTPJitterBufferItem *) g_queue_peek_tail_link (&jbuf->packets);
+  low_buf = (RTPJitterBufferItem *) g_queue_peek_head_link (&jbuf->packets);
 
   if (!high_buf || !low_buf || high_buf == low_buf)
     return 0;
@@ -1429,8 +1432,8 @@ rtp_jitter_buffer_get_seqnum_diff (RTPJitterBuffer * jbuf)
 
   g_return_val_if_fail (jbuf != NULL, 0);
 
-  high_buf = (RTPJitterBufferItem *) g_queue_peek_tail_link (jbuf->packets);
-  low_buf = (RTPJitterBufferItem *) g_queue_peek_head_link (jbuf->packets);
+  high_buf = (RTPJitterBufferItem *) g_queue_peek_tail_link (&jbuf->packets);
+  low_buf = (RTPJitterBufferItem *) g_queue_peek_head_link (&jbuf->packets);
 
   while (high_buf && high_buf->seqnum == -1)
     high_buf = (RTPJitterBufferItem *) high_buf->prev;

diff --git a/gst/rtpmanager/rtpjitterbuffer.h b/gst/rtpmanager/rtpjitterbuffer.h
index ffc27c1b39e9d09bcef9b0f95b582fb1bf1b7cae..8accee4b4007ae2ce57e33258c7cdf509b3d6daf 100644 (file)
--- a/gst/rtpmanager/rtpjitterbuffer.h
+++ b/gst/rtpmanager/rtpjitterbuffer.h
@@ -73,7 +73,7 @@ GType rtp_jitter_buffer_mode_get_type (void);
 struct _RTPJitterBuffer {
   GObject        object;
 
-  GQueue        *packets;
+  GQueue         packets;
 
   RTPJitterBufferMode mode;