From cb51bd6b31a0c181b13b5bbe9a6b2ebae656cb35 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Sebastian=20Dr=C3=B6ge?= <sebastian@centricular.com>
Date: Fri, 29 Jun 2018 07:16:28 +0200
Subject: [PATCH] bufferlist: Prevent gst_buffer_list_foreach() from modifying
 non-writeable lists

Previously gst_buffer_list_foreach() could modify (drop or replace)
buffers in non-writable lists, which could cause all kinds of problems
if other code also has a reference to the list and assumes that it stays
the same.

https://bugzilla.gnome.org/show_bug.cgi?id=796692
---
 gst/gstbufferlist.c             | 19 +++++++++++++++++--
 tests/check/gst/gstbufferlist.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/gst/gstbufferlist.c b/gst/gstbufferlist.c
index b9e5d1f..21fed17 100644
--- a/gst/gstbufferlist.c
+++ b/gst/gstbufferlist.c
@@ -247,7 +247,7 @@ gst_buffer_list_foreach (GstBufferList * list, GstBufferListFunc func,
 {
   guint i, len;
   gboolean ret = TRUE;
-  gboolean list_was_writable;
+  gboolean list_was_writable, first_warning = TRUE;
 
   g_return_val_if_fail (GST_IS_BUFFER_LIST (list), FALSE);
   g_return_val_if_fail (func != NULL, FALSE);
@@ -281,7 +281,22 @@ gst_buffer_list_foreach (GstBufferList * list, GstBufferListFunc func,
 
     /* Check if the function changed the buffer */
     if (buf != buf_ret) {
-      if (buf_ret == NULL) {
+      /* If the list was not writable but the callback was actually changing
+       * our buffer, then it wouldn't have been allowed to do so.
+       *
+       * Fortunately we still have a reference to the old buffer in that case
+       * and just not modify the list, unref the new buffer (if any) and warn
+       * about this */
+      if (!list_was_writable) {
+        if (first_warning) {
+          g_critical
+              ("gst_buffer_list_foreach: non-writable list %p was changed from callback",
+              list);
+          first_warning = FALSE;
+        }
+        if (buf_ret)
+          gst_buffer_unref (buf_ret);
+      } else if (buf_ret == NULL) {
         gst_buffer_list_remove_range_internal (list, i, 1, !was_writable);
         --len;
       } else {
diff --git a/tests/check/gst/gstbufferlist.c b/tests/check/gst/gstbufferlist.c
index 4f831c2..1a483d9 100644
--- a/tests/check/gst/gstbufferlist.c
+++ b/tests/check/gst/gstbufferlist.c
@@ -508,6 +508,37 @@ GST_START_TEST (test_multiple_mutable_buffer_references)
 
 GST_END_TEST;
 
+static gboolean
+foreach_replace_buffer (GstBuffer ** buffer, guint idx, gpointer user_data)
+{
+  gst_buffer_unref (*buffer);
+  *buffer = gst_buffer_new ();
+
+  return TRUE;
+}
+
+GST_START_TEST (test_foreach_modify_non_writeable_list)
+{
+  GstBufferList *b = gst_buffer_list_new_sized (1);
+  GstBuffer *buf;
+
+  buf = gst_buffer_new ();
+  gst_buffer_list_add (b, gst_buffer_ref (buf));
+  gst_buffer_list_ref (b);
+
+  fail_if (gst_buffer_list_is_writable (b));
+
+  ASSERT_CRITICAL (gst_buffer_list_foreach (b, foreach_replace_buffer, NULL));
+
+  fail_unless (gst_buffer_list_get (b, 0) == buf);
+
+  gst_buffer_list_unref (b);
+  gst_buffer_list_unref (b);
+  gst_buffer_unref (buf);
+}
+
+GST_END_TEST;
+
 static Suite *
 gst_buffer_list_suite (void)
 {
@@ -527,6 +558,7 @@ gst_buffer_list_suite (void)
   tcase_add_test (tc_chain, test_calc_size);
   tcase_add_test (tc_chain, test_new_sized_0);
   tcase_add_test (tc_chain, test_multiple_mutable_buffer_references);
+  tcase_add_test (tc_chain, test_foreach_modify_non_writeable_list);
 
   return s;
 }
-- 
2.7.4