[FFmpeg-devel,2/2] avcodec/options: copy the coded_side_data in avcodec_copy_context()

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/options.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

On 4/22/2017 10:26 AM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/options.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/libavcodec/options.c b/libavcodec/options.c
> index b98da9378a..c721aa8d43 100644
> --- a/libavcodec/options.c
> +++ b/libavcodec/options.c
> @@ -190,15 +190,21 @@ void avcodec_free_context(AVCodecContext **pavctx)
>  #if FF_API_COPY_CONTEXT
>  static void copy_context_reset(AVCodecContext *avctx)
>  {
> +    int i;
> +
>      av_opt_free(avctx);
>      av_freep(&avctx->rc_override);
>      av_freep(&avctx->intra_matrix);
>      av_freep(&avctx->inter_matrix);
>      av_freep(&avctx->extradata);
>      av_freep(&avctx->subtitle_header);
> +    for (i = 0; i < avctx->nb_coded_side_data; i++)
> +        av_freep(&avctx->coded_side_data[i].data);
> +    av_freep(&avctx->coded_side_data);
>      av_buffer_unref(&avctx->hw_frames_ctx);
>      avctx->subtitle_header_size = 0;
>      avctx->extradata_size = 0;
> +    avctx->nb_coded_side_data = 0;
>  }
>
>  int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src)
> @@ -262,6 +268,28 @@ 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) {
> +        int i;
> +
> +        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)
> +            goto fail;
> +
> +        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)
> +                goto fail;
> +            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);
>

If src has coded_side_data and nb_coded_side_data set to non-zero 
values, as a result of the earlier call to memcpy(), it will copy those 
values into dest.  Then, if it does a goto to fail early, as a result of 
your changes to copy_context_reset(), it will call av_freep() on those 
elements in dest, even though they are owned by src.  After doing the 
memcpy() call, I think you can fix this by setting dest->coded_side_data 
to NULL and dest->nb_coded_side_data to 0.

Everything else looks good.

Aaron Levinson

On 4/23/2017 8:27 PM, Aaron Levinson wrote:
> On 4/22/2017 10:26 AM, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/options.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/libavcodec/options.c b/libavcodec/options.c
>> index b98da9378a..c721aa8d43 100644
>> --- a/libavcodec/options.c
>> +++ b/libavcodec/options.c
>> @@ -190,15 +190,21 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>  #if FF_API_COPY_CONTEXT
>>  static void copy_context_reset(AVCodecContext *avctx)
>>  {
>> +    int i;
>> +
>>      av_opt_free(avctx);
>>      av_freep(&avctx->rc_override);
>>      av_freep(&avctx->intra_matrix);
>>      av_freep(&avctx->inter_matrix);
>>      av_freep(&avctx->extradata);
>>      av_freep(&avctx->subtitle_header);
>> +    for (i = 0; i < avctx->nb_coded_side_data; i++)
>> +        av_freep(&avctx->coded_side_data[i].data);
>> +    av_freep(&avctx->coded_side_data);
>>      av_buffer_unref(&avctx->hw_frames_ctx);
>>      avctx->subtitle_header_size = 0;
>>      avctx->extradata_size = 0;
>> +    avctx->nb_coded_side_data = 0;
>>  }
>>
>>  int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext
>> *src)
>> @@ -262,6 +268,28 @@ 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) {
>> +        int i;
>> +
>> +        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)
>> +            goto fail;
>> +
>> +        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)
>> +                goto fail;
>> +            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);
>>
> 
> If src has coded_side_data and nb_coded_side_data set to non-zero
> values, as a result of the earlier call to memcpy(), it will copy those
> values into dest.  Then, if it does a goto to fail early, as a result of
> your changes to copy_context_reset(), it will call av_freep() on those
> elements in dest, even though they are owned by src.  After doing the
> memcpy() call, I think you can fix this by setting dest->coded_side_data
> to NULL and dest->nb_coded_side_data to 0.

Good catch, will send a new version of the set.

> 
> Everything else looks good.
> 
> Aaron Levinson
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel