[FFmpeg-devel] [PATCH 1/3] avcodec/avpacket: Always treat dst in av_packet_ref as uninitialized

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2020-02-12 16:02:21)
>> av_packet_ref() mostly treated the destination packet dst as uninitialized,
>> i.e. the destination fields were simply overwritten. But if the source
>> packet was not reference-counted, dst->buf was treated as if it pointed
>> to an already allocated buffer (if != NULL) to be reallocated to the
>> desired size.
>>
>> The documentation did not explicitly state whether the dst will be treated
>> as uninitialized, but it stated that if the source packet is not refcounted,
>> a new buffer in dst will be allocated. This and the fact that the side-data
>> as well as the codepath taken in case src is refcounted always treated the
>> packet as uninitialized means that dst should always be treated as
>> uninitialized for the sake of consistency. And this behaviour has been
>> explicitly documented.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>>  libavcodec/avcodec.h  | 2 +-
>>  libavcodec/avpacket.c | 1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index dc5807324f..982a545dc6 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -4600,7 +4600,7 @@ void av_packet_free_side_data(AVPacket *pkt);
>>   *
>>   * @see av_packet_unref
>>   *
>> - * @param dst Destination packet
>> + * @param dst Destination packet. Will be treated as initially uninitialized.
> 
> The 'initially' sounds weird to me. How about "Will be completely
> overwritten"?
> 
Changed.

>>   * @param src Source packet
>>   *
>>   * @return 0 on success, a negative AVERROR on error.
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 74845efcd2..0d9ddeee07 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -614,6 +614,7 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src)
>>          return ret;
>>  
>>      if (!src->buf) {
>> +        dst->buf = NULL;
>>          ret = packet_alloc(&dst->buf, src->size);
>>          if (ret < 0)
>>              goto fail;
>> -- 
>> 2.20.1
> 
> weak sugestion - perhaps it'd be clearer to just add a call to
> av_init_packet() to the beginning of that function

It's unnecessary on success, but I have added it on failure in [1].
> 
> Also, we might want to finally forbid non-refcounted packets completely.
> 
Then you might want to take a look at [2].

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259083.html
[2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253662.html