diff mbox

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

Message ID e79e0841-2cc6-a66f-4799-185fcafaa206@gmail.com
State New
Headers show

Commit Message

James Almer April 21, 2017, 9:05 p.m. UTC 
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@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.
From 0660ae9b5c8e045d8817e9994b15bbc70065f8ad Mon Sep 17 00:00:00 2001
From: James Almer <jamrial@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@gmail.com>
---
 libavcodec/options.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Michael Niedermayer April 22, 2017, 4:15 p.m. UTC | #1 
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@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@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@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.

[...]
diff mbox

Patch

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++;
+        }
+    }
 
     if (src->hw_frames_ctx) {
         dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);