[FFmpeg-devel] [PATCH] avpacket: ABI bump additions

[Lynne]

8 Jul 2021, 21:20 by andreas.rheinhardt at outlook.com:

> Lynne:
>
>> Apr 26, 2021, 03:27 by andreas.rheinhardt at outlook.com:
>>
>>> Lynne:
>>>
>>>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001
>>>> From: Lynne <dev at lynne.ee>
>>>> Date: Sat, 23 Jan 2021 19:56:18 +0100
>>>> Subject: [PATCH] avpacket: ABI bump additions
>>>>
>>>> ---
>>>>  libavcodec/avpacket.c |  5 +++++
>>>>  libavcodec/packet.h   | 21 +++++++++++++++++++++
>>>>  2 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>> index e32c467586..03b73b3b53 100644
>>>> --- a/libavcodec/avpacket.c
>>>> +++ b/libavcodec/avpacket.c
>>>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>>>  dst->flags                = src->flags;
>>>>  dst->stream_index         = src->stream_index;
>>>>  
>>>> +    i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>>>> +    if (i < 0)
>>>> +        return i;
>>>>
>>>
>>> 1. Don't use i here; add a new variable.
>>> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the
>>> destination packet as uninitialized and make no attempt at unreferencing
>>> its content; yet you try to reuse opaque_ref. Even worse, you might
>>> return potentially dangerous packets: If the properties were
>>> uninitialized and av_packet_copy_props() failed, then the caller were
>>> not allowed to unreference the packet even when the non-properties were
>>> set to sane values. The easiest way to fix this is to move setting
>>> opaque ref to the place after initializing side_data below and either
>>> set dst->opaque_ref to NULL before av_buffer_replace() or to not use
>>> av_buffer_replace(). It may also be best to unref it again if copying
>>> side data fails.
>>>
>>>> +
>>>>  dst->side_data            = NULL;
>>>>  dst->side_data_elems      = 0;
>>>>  for (i = 0; i < src->side_data_elems; i++) {
>>>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>>>  void av_packet_unref(AVPacket *pkt)
>>>>  {
>>>>  av_packet_free_side_data(pkt);
>>>> +    av_buffer_unref(&pkt->opaque_ref);
>>>>  av_buffer_unref(&pkt->buf);
>>>>  get_packet_defaults(pkt);
>>>>  }
>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>> index fad8341c12..c29ad18a2b 100644
>>>> --- a/libavcodec/packet.h
>>>> +++ b/libavcodec/packet.h
>>>> @@ -383,6 +383,27 @@ typedef struct AVPacket {
>>>>  int64_t duration;
>>>>  
>>>>  int64_t pos;                            ///< byte position in stream, -1 if unknown
>>>> +
>>>> +    /**
>>>> +     * for some private data of the user
>>>> +     */
>>>> +    void *opaque;
>>>>
>>>
>>> The corresponding AVFrame field is copied when copying props.
>>>
>>
>> Fixed both, thanks. Also copied the time_base field and made av_init_packet()
>> initialize all fields.
>>
>
> Your new version is still not correct: If copying side data fails, the
> function returns without initializing opaque_ref at all, thereby making
> it dangerous to unref the packet (which e.g. av_packet_ref() does).
>
> - Andreas
>

Fixed, and made sure to unref it if copying side data fails.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v2-0001-avpacket-ABI-bump-additions.patch
Type: text/x-patch
Size: 3582 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210720/9c38a5d2/attachment.bin>