From ee43b6421e568e6406751222f702432d7b786507 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 016aa7dd9f..6a040863e3 100644 --- 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 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);