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

[James Almer]

On 4/25/2021 10:27 PM, Andreas Rheinhardt wrote:
> 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.

Fwiw, the idea is that once sizeof(AVPacket) is not a part of the ABI 
anymore, there will no longer be such thing as an uninitialized packet 
as far as proper API usage goes. And as such the lines below...

> 
>> +
>>       dst->side_data            = NULL;
>>       dst->side_data_elems      = 0;

          ^^^^^^^^^^^^^^^^^^^^

...should be replaced by a av_packet_free_side_data(dst) call.
Similarly, at that point using av_buffer_replace() on opaque_ref should 
become safe. But until then, you're right that doing so *will* go wrong 
on uninitialized packets.

>>       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.
> 
>> +
>> +    /**
>> +     * AVBufferRef for free use by the API user. FFmpeg will never check the
>> +     * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when
>> +     * the packet is unreferenced. av_packet_copy_props() calls create a new
>> +     * reference with av_buffer_ref() for the target packet's opaque_ref field.
>> +     *
>> +     * This is unrelated to the opaque field, although it serves a similar
>> +     * purpose.
>> +     */
>> +    AVBufferRef *opaque_ref;
>> +
>> +    /**
>> +     * Time base of the packet's timestamps.
>> +     */
>> +    AVRational time_base;
>>   } AVPacket;
>>   
>>   #if FF_API_INIT_PACKET
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>