[FFmpeg-devel] [PATCH] Fixed memory leaks associated with AVStream objects if FF_API_LAVF_AVCTX is defined

[Michael Niedermayer]

On Fri, Apr 21, 2017 at 06:05:12PM -0300, James Almer wrote:
> On 4/21/2017 6:03 PM, James Almer wrote:
> >On 4/21/2017 12:09 PM, Michael Niedermayer wrote:
> >>On Thu, Apr 20, 2017 at 11:30:13PM -0700, Aaron Levinson wrote:
> >>> From 4f27e910aca6dae6642b4d73cf07fa0d6c4b1618 Mon Sep 17 00:00:00 2001
> >>>From: Aaron Levinson <alevinsn at aracnet.com>
> >>>Date: Thu, 20 Apr 2017 23:20:20 -0700
> >>>Subject: [PATCH] Fixed memory leaks associated with AVStream objects if
> >>>  FF_API_LAVF_AVCTX is defined
> >>>
> >>>Purpose: Fixed memory leaks associated with AVStream objects if
> >>>FF_API_LAVF_AVCTX is defined.  If FF_API_LAVF_AVCTX is defined,
> >>>AVStream::codec is set to an AVCodecContext object, and this wasn't
> >>>being deallocated properly when the AVStream object was freed.  While
> >>>FF_API_LAVF_AVCTX has to do with deprecated functionality, in
> >>>practice, it will always be defined for typical builds currently,
> >>>since it is defined in libavformat\version.h if
> >>>LIBAVFORMAT_VERSION_MAJOR is less than 58, and
> >>>LIBAVFORMAT_VERSION_MAJOR is currently set to 57.
> >>>
> >>>Comments:
> >>>
> >>>-- libavformat/utils.c: Now using avcodec_free_context() to properly
> >>>    deallocate st->codec in free_stream() if FF_API_LAVF_AVCTX is
> >>>    defined.
> >>>---
> >>>  libavformat/utils.c | 4 +---
> >>>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >>please use "make fate" to test your changes
> >>
> >>This one causes many all? tests to segfault
> >
> >The issue is coded_side_data in AVStream->codec.
> >
> >ffmpeg.c calls avcodec_copy_context() to copy the encoder AVCodecContext
> >to the output's AVStream->codec because the latter is still needed
> >by some internal code in lavf/utils.c and such.
> >avcodec_copy_context() copies the coded_side_data pointer as part
> >of its memcpy call but not the buffers, and by the time
> >avcodec_free_context() is called for AVStream->codec the buffers
> >said pointer points to was already freed by the
> >avcodec_free_context() call for the encoder AVCodecContext.
> >
> >The proper solution: Remove the avcodec_copy_context() call in
> >ffmpeg.c and make libavformat stop needing AVStream->codec
> >internally. It should use AVStream->internal->avctx instead. Even
> >if this is not done now, it will anyway when the deprecation
> >period ends and it's time to remove AVStream->codec.
> >The easy but ugly solution until the above is done: Copy the
> >coded_side_data buffers in avcodec_copy_context().
> >
> >One could argue that by definition avcodec_copy_context() should
> >copy said side data, but the function is clearly marked as "do not
> >use, its behavior is ill defined" and the user is asked instead to
> >copy using an intermediary AVCodecParameters context instead.
> 
> The attached patch solves this the easy-but-ugly way.

>  options.c |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 871f5fb0223f619b031ccabdf08760e54645f7ba  0001-avcodec-options-copy-the-coded_side_data-in-avcodec_.patch
> From 0660ae9b5c8e045d8817e9994b15bbc70065f8ad Mon Sep 17 00:00:00 2001
> From: James Almer <jamrial at gmail.com>
> Date: Fri, 21 Apr 2017 17:52:02 -0300
> Subject: [PATCH] avcodec/options: copy the coded_side_data in
>  avcodec_copy_context()
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  libavcodec/options.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/libavcodec/options.c b/libavcodec/options.c
> index 7bdb0be5af..406bb34678 100644
> --- a/libavcodec/options.c
> +++ b/libavcodec/options.c
> @@ -192,6 +192,7 @@ int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src)
>  {
>      const AVCodec *orig_codec = dest->codec;
>      uint8_t *orig_priv_data = dest->priv_data;
> +    int i;
>  
>      if (avcodec_is_open(dest)) { // check that the dest context is uninitialized
>          av_log(dest, AV_LOG_ERROR,
> @@ -206,6 +207,9 @@ int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src)
>      av_freep(&dest->inter_matrix);
>      av_freep(&dest->extradata);
>      av_freep(&dest->subtitle_header);
> +    for (i = 0; i < dest->nb_coded_side_data; i++)
> +        av_freep(&dest->coded_side_data[i].data);
> +    av_freep(&dest->coded_side_data);
>  
>      memcpy(dest, src, sizeof(*dest));
>      av_opt_copy(dest, src);
> @@ -254,6 +258,26 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      alloc_and_copy_or_fail(subtitle_header, src->subtitle_header_size, 1);
>      av_assert0(dest->subtitle_header_size == src->subtitle_header_size);
>  #undef alloc_and_copy_or_fail
> +    if (src->nb_coded_side_data) {
> +        dest->nb_coded_side_data = 0;
> +        dest->coded_side_data = av_realloc_array(NULL, src->nb_coded_side_data,
> +                                                 sizeof(*dest->coded_side_data));
> +        if (!dest->coded_side_data)
> +            return AVERROR(ENOMEM);
> +
> +        for (i = 0; i < src->nb_coded_side_data; i++) {
> +            const AVPacketSideData *sd_src = &src->coded_side_data[i];
> +            AVPacketSideData *sd_dst = &dest->coded_side_data[i];
> +
> +            sd_dst->data = av_malloc(sd_src->size);
> +            if (!sd_dst->data)
> +                return AVERROR(ENOMEM);
> +            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> +            sd_dst->size = sd_src->size;
> +            sd_dst->type = sd_src->type;
> +            dest->nb_coded_side_data++;
> +        }
> +    }

the rest of the code does goto fail, doing some cleanup, this does
return directly.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170422/628149c8/attachment.sig>