va: Fix a latent race condition in vabasedec.

The vabasedec's display and decoder are created/destroyed between
the gst_va_base_dec_open/close pair. All the data and event handling
functions are between this pair and so the accessing to these pointers
are safe. But the query function can be called anytime. So we need to:
1. Make these pointers operation in open/close and query atomic.
2. Hold an extra ref during query function to avoid it destroyed.

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

diff --git a/sys/va/gstvabasedec.c b/sys/va/gstvabasedec.c
index 91a115d..ac66caa 100644 (file)
--- a/sys/va/gstvabasedec.c
+++ b/sys/va/gstvabasedec.c
@@ -36,15 +36,27 @@ gst_va_base_dec_open (GstVideoDecoder * decoder)
 {
   GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
   GstVaBaseDecClass *klass = GST_VA_BASE_DEC_GET_CLASS (decoder);
+  gboolean ret = FALSE;
 
   if (!gst_va_ensure_element_data (decoder, klass->render_device_path,
           &base->display))
     return FALSE;
 
-  if (!base->decoder)
-    base->decoder = gst_va_decoder_new (base->display, klass->codec);
+  if (!g_atomic_pointer_get (&base->decoder)) {
+    GstVaDecoder *va_decoder;
+
+    va_decoder = gst_va_decoder_new (base->display, klass->codec);
+    if (va_decoder)
+      ret = TRUE;
+
+    gst_object_replace ((GstObject **) (&base->decoder),
+        (GstObject *) va_decoder);
+    gst_object_unref (va_decoder);
+  } else {
+    ret = TRUE;
+  }
 
-  return (base->decoder != NULL);
+  return ret;
 }
 
 gboolean
@@ -82,9 +94,14 @@ gst_va_base_dec_getcaps (GstVideoDecoder * decoder, GstCaps * filter)
 {
   GstCaps *caps = NULL, *tmp;
   GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
+  GstVaDecoder *va_decoder = NULL;
+
+  gst_object_replace ((GstObject **) & va_decoder, (GstObject *) base->decoder);
+
+  if (va_decoder)
+    caps = gst_va_decoder_get_sinkpad_caps (va_decoder);
 
-  if (base->decoder)
-    caps = gst_va_decoder_get_sinkpad_caps (base->decoder);
+  gst_object_unref (va_decoder);
 
   if (caps) {
     if (filter) {
@@ -108,19 +125,34 @@ gst_va_base_dec_src_query (GstVideoDecoder * decoder, GstQuery * query)
 
   switch (GST_QUERY_TYPE (query)) {
     case GST_QUERY_CONTEXT:{
-      return gst_va_handle_context_query (GST_ELEMENT_CAST (decoder), query,
-          base->display);
+      GstVaDisplay *display = NULL;
+
+      gst_object_replace ((GstObject **) & display,
+          (GstObject *) base->display);
+
+      ret = gst_va_handle_context_query (GST_ELEMENT_CAST (decoder), query,
+          display);
+
+      gst_object_unref (display);
+      break;
     }
     case GST_QUERY_CAPS:{
       GstCaps *caps = NULL, *tmp, *filter = NULL;
+      GstVaDecoder *va_decoder = NULL;
       gboolean fixed_caps;
 
+      gst_object_replace ((GstObject **) & va_decoder,
+          (GstObject *) base->decoder);
+
       gst_query_parse_caps (query, &filter);
 
       fixed_caps = GST_PAD_IS_FIXED_CAPS (GST_VIDEO_DECODER_SRC_PAD (decoder));
 
-      if (!fixed_caps && base->decoder)
-        caps = gst_va_decoder_get_srcpad_caps (base->decoder);
+      if (!fixed_caps && va_decoder)
+        caps = gst_va_decoder_get_srcpad_caps (va_decoder);
+
+      gst_object_unref (va_decoder);
+
       if (caps) {
         if (filter) {
           tmp =
@@ -149,10 +181,18 @@ static gboolean
 gst_va_base_dec_sink_query (GstVideoDecoder * decoder, GstQuery * query)
 {
   GstVaBaseDec *base = GST_VA_BASE_DEC (decoder);
+  gboolean ret = FALSE;
 
   if (GST_QUERY_TYPE (query) == GST_QUERY_CONTEXT) {
-    return gst_va_handle_context_query (GST_ELEMENT_CAST (decoder), query,
-        base->display);
+    GstVaDisplay *display = NULL;
+
+    gst_object_replace ((GstObject **) & display, (GstObject *) base->display);
+
+    ret = gst_va_handle_context_query (GST_ELEMENT_CAST (decoder), query,
+        display);
+
+    gst_object_unref (display);
+    return ret;
   }
 
   return GST_VIDEO_DECODER_CLASS (parent_class)->sink_query (decoder, query);

diff --git a/sys/va/gstvautils.c b/sys/va/gstvautils.c
index 3b1b9fe..dcfee10 100644 (file)
--- a/sys/va/gstvautils.c
+++ b/sys/va/gstvautils.c
@@ -186,19 +186,20 @@ gst_va_ensure_element_data (gpointer element, const gchar * render_device_path,
   /*  1) Check if the element already has a context of the specific
    *     type.
    */
-  if (gst_va_display_found (element, *display_ptr))
+  if (gst_va_display_found (element, g_atomic_pointer_get (display_ptr)))
     goto done;
 
   _gst_context_query (element, "gst.va.display.handle");
 
   /* Neighbour found and it updated the display */
-  if (gst_va_display_found (element, *display_ptr))
+  if (gst_va_display_found (element, g_atomic_pointer_get (display_ptr)))
     goto done;
 
   /* If no neighbor, or application not interested, use drm */
   display = gst_va_display_drm_new_from_path (render_device_path);
 
-  *display_ptr = display;
+  gst_object_replace ((GstObject **) display_ptr, (GstObject *) display);
+  gst_object_unref (display);
 
   gst_va_element_propagate_display_context (element, display);
 
@@ -231,11 +232,9 @@ gst_va_handle_set_context (GstElement * element, GstContext * context,
   }
 
   if (display_replacement) {
-    GstVaDisplay *old = *display_ptr;
-    *display_ptr = display_replacement;
-
-    if (old)
-      gst_object_unref (old);
+    gst_object_replace ((GstObject **) display_ptr,
+        (GstObject *) display_replacement);
+    gst_object_unref (display_replacement);
   }
 
   return TRUE;