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

[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"?

>   * @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

Also, we might want to finally forbid non-refcounted packets completely.

-- 
Anton Khirnov