From: Jan Alexander Steffens (heftig) <jsteffens@make.tv>
Date: Mon, 23 Mar 2020 11:28:12 +0000 (+0100)
Subject: gststructure: Fix gst_structure_take ownership handling
X-Git-Tag: 1.19.3~961
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a20ff6aaf48a09f46a1e63147ca080f6904e607a;p=platform%2Fupstream%2Fgstreamer.git

gststructure: Fix gst_structure_take ownership handling

The old code would leave a dangling pointer in oldstr_ptr if two threads
attempted to take the same structure into the same location at the same
time:

1. First "oldstr == newstr" check (before the loop) fails.
2. Compare-and-exchange fails, due to a second thread completing the
   same gst_structure_take.
3. Second "oldstr == newstr" check (in the loop) succeeds, loop breaks.
4. "oldstr" check succeeds, old structure gets freed.
5. oldstr_ptr now contains a dangling pointer.

This shouldn't happen in code that handles ownership sanely, so check
that we don't try to do this and complain loudly.

Also simplify the function by using a do-while loop, like
gst_mini_object_take.

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/413
---

diff --git a/gst/gststructure.c b/gst/gststructure.c
index a4ee28bcdf..d5dc4f62fe 100644
--- a/gst/gststructure.c
+++ b/gst/gststructure.c
@@ -511,12 +511,15 @@ gst_clear_structure (GstStructure ** structure_ptr)
  *     a #GstStructure to take
  * @newstr: (transfer full) (allow-none): a new #GstStructure
  *
- * Atomically modifies a pointer to point to a new object.
+ * Atomically modifies a pointer to point to a new structure.
  * The #GstStructure @oldstr_ptr is pointing to is freed and
  * @newstr is taken ownership over.
  *
  * Either @newstr and the value pointed to by @oldstr_ptr may be %NULL.
  *
+ * It is a programming error if both @newstr and the value pointed to by
+ * @oldstr_ptr refer to the same, non-%NULL structure.
+ *
  * Returns: %TRUE if @newstr was different from @oldstr_ptr
  *
  * Since: 1.18
@@ -528,22 +531,19 @@ gst_structure_take (GstStructure ** oldstr_ptr, GstStructure * newstr)
 
   g_return_val_if_fail (oldstr_ptr != NULL, FALSE);
 
-  oldstr = g_atomic_pointer_get ((gpointer *) oldstr_ptr);
-
-  if (G_UNLIKELY (oldstr == newstr))
-    return FALSE;
-
-  while (G_UNLIKELY (!g_atomic_pointer_compare_and_exchange ((gpointer *)
-              oldstr_ptr, oldstr, newstr))) {
+  do {
     oldstr = g_atomic_pointer_get ((gpointer *) oldstr_ptr);
-    if (G_UNLIKELY (oldstr == newstr))
-      break;
-  }
+    if (G_UNLIKELY (oldstr == newstr)) {
+      g_return_val_if_fail (newstr == NULL, FALSE);
+      return FALSE;
+    }
+  } while (G_UNLIKELY (!g_atomic_pointer_compare_and_exchange ((gpointer *)
+              oldstr_ptr, oldstr, newstr)));
 
   if (oldstr)
     gst_structure_free (oldstr);
 
-  return oldstr != newstr;
+  return TRUE;
 }
 
 /**