[FFmpeg-devel] [PATCH 1/2] avcodec/avpacket: fix leaks when copying side data if src == dst

[James Almer]

On 9/21/2017 7:04 PM, James Almer wrote:
> The scenario makes no sense and produces all kinds of memory leaks.
> Return 0 instead of an error as the process is pretty much a nop.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  libavcodec/avpacket.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 5ce3228166..a68e2501ad 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -225,14 +225,14 @@ failed_alloc:
>  
>  int av_copy_packet_side_data(AVPacket *pkt, const AVPacket *src)
>  {
> +    if (src == pkt)
> +        return 0;
> +
>      if (src->side_data_elems) {
>          int i;
> -        DUP_DATA(pkt->side_data, src->side_data,
> -                src->side_data_elems * sizeof(*src->side_data), 0, ALLOC_MALLOC);
> -        if (src != pkt) {
> -            memset(pkt->side_data, 0,
> -                   src->side_data_elems * sizeof(*src->side_data));
> -        }
> +        pkt->side_data = av_mallocz_array(src->side_data_elems, sizeof(*src->side_data));
> +        if (!pkt->side_data)
> +            goto failed_alloc;
>          for (i = 0; i < src->side_data_elems; i++) {
>              DUP_DATA(pkt->side_data[i].data, src->side_data[i].data,
>                      src->side_data[i].size, 1, ALLOC_MALLOC);
> 

Right, so there's one scenario for src == dst, but it's a hacky one.
Basically, something like

AVPacket tmp = *pkt;
av_copy_packet_side_data(pkt, pkt);

Which means there will be no leaks as long as tmp is properly freed
afterguards.

Changing that behavior will potentially break existing code using the
function (commit fdd1aaf87aa hints that there are some uses of this
kind), so seeing it's already inside relevant guards I'm going to
effectively deprecate it in favor of the saner av_packet_copy_props().