[FFmpeg-devel] [PATCH 2/2] avcodec/avcodec, avpacket: Return blank packet on av_packet_ref() failure

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2020-03-27 03:25:14)
>> Up until now, it was completely unspecified what the content of the
>> destination packet dst was on error. Depending upon where the error
>> happened calling av_packet_unref() on dst might be dangerous.
>>
>> This commit changes this by making sure that dst is blank on error, so
>> unreferencing it again is safe (and still pointless). This behaviour is
>> documented.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>>  doc/APIchanges        | 4 ++++
>>  libavcodec/avcodec.h  | 3 ++-
>>  libavcodec/avpacket.c | 7 ++++---
>>  libavcodec/version.h  | 2 +-
>>  4 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 8eeaec2028..f2bb2d242b 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>  
>>  API changes, most recent first:
>>  
>> +2020-03-27 - xxxxxxxxxx - lavc 58.77.100 - avcodec.h
>> +  av_packet_ref() now guarantees to return the destination packet
>> +  in a blank state on error.
>> +
>>  2020-03-10 - xxxxxxxxxx - lavc 58.75.100 - avcodec.h
>>    Add AV_PKT_DATA_ICC_PROFILE.
>>  
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index f918d20a61..8fc0ad92c9 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -4651,7 +4651,8 @@ void av_packet_free_side_data(AVPacket *pkt);
>>   * @param dst Destination packet. Will be completely overwritten.
>>   * @param src Source packet
>>   *
>> - * @return 0 on success, a negative AVERROR on error.
>> + * @return 0 on success, a negative AVERROR on error. On error, dst
>> + *         will be blank (as if returned by av_packet_alloc()).
>>   */
>>  int av_packet_ref(AVPacket *dst, const AVPacket *src);
>>  
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 132567bc2d..c622718a45 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -610,12 +610,13 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src)
>>  {
>>      int ret;
>>  
>> +    dst->buf = NULL;
>> +
> 
> I really think av_init_packet() would be more robust against future
> changes. But I'm not going to push for that especially strongly, so feel
> free to push whichever version you prefer.
> 
Applied as is. After all, avpacket.c would be checked thoroughly in case
of a future addition to AVPacket with something that needs to be
allocated and freed (whereas lots of other places might be forgotten).

- Andreas