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

[James Almer]

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