videodecoder: fix segfault copying buffer metas

The current implementation copies metas without checking if the buffer
is writable.

The operation that needs to be done, replacing the input buffer and
copying the metas, is only part of that process. We create a new function
that does both.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5054>

diff --git a/subprojects/gst-plugins-base/gst-libs/gst/video/gstvideodecoder.c b/subprojects/gst-plugins-base/gst-libs/gst/video/gstvideodecoder.c
index 016aa7d..6a04086 100644 (file)
--- a/subprojects/gst-plugins-base/gst-libs/gst/video/gstvideodecoder.c
+++ b/subprojects/gst-plugins-base/gst-libs/gst/video/gstvideodecoder.c
@@ -542,6 +542,9 @@ static gboolean gst_video_decoder_transform_meta_default (GstVideoDecoder *
 static gboolean gst_video_decoder_handle_missing_data_default (GstVideoDecoder *
     decoder, GstClockTime timestamp, GstClockTime duration);
 
+static void gst_video_decoder_replace_input_buffer (GstVideoDecoder * decoder,
+    GstVideoCodecFrame * frame, GstBuffer ** dest_buffer);
+
 static void gst_video_decoder_copy_metas (GstVideoDecoder * decoder,
     GstVideoCodecFrame * frame, GstBuffer * src_buffer,
     GstBuffer * dest_buffer);
@@ -2470,11 +2473,7 @@ gst_video_decoder_chain_forward (GstVideoDecoder * decoder,
       GST_VIDEO_CODEC_FRAME_SET_SYNC_POINT (frame);
     }
 
-    if (frame->input_buffer) {
-      gst_video_decoder_copy_metas (decoder, frame, frame->input_buffer, buf);
-      gst_buffer_unref (frame->input_buffer);
-    }
-    frame->input_buffer = buf;
+    gst_video_decoder_replace_input_buffer (decoder, frame, &buf);
 
     if (decoder->input_segment.rate < 0.0) {
       priv->parse_gather = g_list_prepend (priv->parse_gather, frame);
@@ -3401,6 +3400,20 @@ gst_video_decoder_copy_metas (GstVideoDecoder * decoder,
   }
 }
 
+static void
+gst_video_decoder_replace_input_buffer (GstVideoDecoder * decoder,
+    GstVideoCodecFrame * frame, GstBuffer ** dest_buffer)
+{
+  if (frame->input_buffer) {
+    *dest_buffer = gst_buffer_make_writable (*dest_buffer);
+    gst_video_decoder_copy_metas (decoder, frame, frame->input_buffer,
+        *dest_buffer);
+    gst_buffer_unref (frame->input_buffer);
+  }
+
+  frame->input_buffer = *dest_buffer;
+}
+
 /**
  * gst_video_decoder_finish_frame:
  * @decoder: a #GstVideoDecoder
@@ -3841,12 +3854,8 @@ gst_video_decoder_have_frame (GstVideoDecoder * decoder)
     buffer = gst_buffer_new_and_alloc (0);
   }
 
-  if (priv->current_frame->input_buffer) {
-    gst_video_decoder_copy_metas (decoder, priv->current_frame,
-        priv->current_frame->input_buffer, buffer);
-    gst_buffer_unref (priv->current_frame->input_buffer);
-  }
-  priv->current_frame->input_buffer = buffer;
+  gst_video_decoder_replace_input_buffer (decoder, priv->current_frame,
+      &buffer);
 
   gst_video_decoder_get_buffer_info_at_offset (decoder,
       priv->frame_offset, &pts, &dts, &duration, &flags);

diff --git a/subprojects/gst-plugins-base/tests/check/libs/videodecoder.c b/subprojects/gst-plugins-base/tests/check/libs/videodecoder.c
index 16c1e4a..c995321 100644 (file)
--- a/subprojects/gst-plugins-base/tests/check/libs/videodecoder.c
+++ b/subprojects/gst-plugins-base/tests/check/libs/videodecoder.c
@@ -1315,12 +1315,18 @@ GST_START_TEST (videodecoder_playback_event_order)
 
 GST_END_TEST;
 
+/*
+ * MODE_META_COPY: takes an extra ref to the input buffer to check metas
+ *                 are copied to a writable buffer.
+ *                 see: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4912
+ */
 typedef enum
 {
   MODE_NONE = 0,
   MODE_SUBFRAMES = 1,
   MODE_PACKETIZED = 1 << 1,
   MODE_META_ROI = 1 << 2,
+  MODE_META_COPY = 1 << 3,
 } SubframeMode;
 
 static void
@@ -1379,10 +1385,19 @@ videodecoder_playback_subframe_mode (SubframeMode mode)
       gst_buffer_add_video_region_of_interest_meta (buffer, "face", 0, 0, 10,
           10);
 
+    /* Take an extra ref to check that we ensure buffer is writable when copying metas
+     * https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4912
+     */
+    if (mode & MODE_META_COPY) {
+      gst_buffer_ref (buffer);
+    }
     fail_unless (gst_pad_push (mysrcpad, buffer) == GST_FLOW_OK);
     fail_unless (gst_pad_push_event (mysrcpad,
             gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM,
                 gst_structure_new_empty ("custom1"))));
+    if (mode & MODE_META_COPY) {
+      gst_buffer_unref (buffer);
+    }
   }
   /* Send EOS */
   fail_unless (gst_pad_push_event (mysrcpad, gst_event_new_eos ()));
@@ -1540,6 +1555,14 @@ GST_START_TEST (videodecoder_playback_packetized_subframes_metadata)
 
 GST_END_TEST;
 
+GST_START_TEST (videodecoder_playback_packetized_subframes_metadata_copy)
+{
+  videodecoder_playback_subframe_mode (MODE_SUBFRAMES |
+      MODE_PACKETIZED | MODE_META_ROI | MODE_META_COPY);
+}
+
+GST_END_TEST;
+
 GST_START_TEST (videodecoder_playback_invalid_ts_packetized)
 {
   videodecoder_playback_invalid_ts_subframe_mode (MODE_PACKETIZED);
@@ -1589,6 +1612,7 @@ gst_videodecoder_suite (void)
   tcase_add_test (tc, videodecoder_playback_parsed_subframes);
   tcase_add_test (tc, videodecoder_playback_packetized_subframes);
   tcase_add_test (tc, videodecoder_playback_packetized_subframes_metadata);
+  tcase_add_test (tc, videodecoder_playback_packetized_subframes_metadata_copy);
   tcase_add_test (tc, videodecoder_playback_invalid_ts_packetized);
   tcase_add_test (tc, videodecoder_playback_invalid_ts_packetized_subframes);