From fcf2d8c350778814866a3d8736d17b98fb26cef7 Mon Sep 17 00:00:00 2001 From: Andoni Morales Alastruey Date: Wed, 21 Jun 2023 13:45:13 +0200 Subject: [PATCH] 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: --- .../gst-libs/gst/video/gstvideodecoder.c | 31 ++++++++++++------- .../tests/check/libs/videodecoder.c | 24 ++++++++++++++ 2 files changed, 44 insertions(+), 11 deletions(-) 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 b96fd47dd0..74bcab1e72 100644 --- a/subprojects/gst-plugins-base/gst-libs/gst/video/gstvideodecoder.c +++ b/subprojects/gst-plugins-base/gst-libs/gst/video/gstvideodecoder.c @@ -536,6 +536,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); @@ -2459,11 +2462,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); @@ -3381,6 +3380,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 @@ -3821,12 +3834,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 16c1e4a266..c995321e20 100644 --- 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);