[FFmpeg-devel] [PATCH] libavcodec/options.c: handle hw_frames_ctx where necessary

[Mark Thompson]

On 13/05/16 10:42, wm4 wrote:
> On Fri, 13 May 2016 10:54:17 +0300
> Andrey Turkin <andrey.turkin at gmail.com> wrote:
> 
>> 2016-05-13 10:35 GMT+03:00 wm4 <nfxjfg at googlemail.com>:
>>
>>> On Thu, 12 May 2016 22:35:48 +0300
>>> Andrey Turkin <andrey.turkin at gmail.com> wrote:
>>>  
>>>> Few functions didn't handle hw_frames_ctx references causing resources  
>>> leaks and even crashes.  
>>>> ---
>>>>  libavcodec/options.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c
>>>> index ea2563b..8682262 100644
>>>> --- a/libavcodec/options.c
>>>> +++ b/libavcodec/options.c
>>>> @@ -175,6 +175,7 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>>>      av_freep(&avctx->intra_matrix);
>>>>      av_freep(&avctx->inter_matrix);
>>>>      av_freep(&avctx->rc_override);
>>>> +    av_buffer_unref(&avctx->hw_frames_ctx);
>>>>
>>>>      av_freep(pavctx);
>>>>  }  
>>>
>>> I would have thought this is the responsibility of the API user?
>>>
>>>  
>> AVCodecContext documentation says it is set by a user but then managed and
>> owned by libavcodec (which is a logical thing to do for any shared
>> reference).
> 
> Even so it's a breaking API change and should be treated as such. An
> API user could for example have a separate variable with the same
> buffer ref somewhere, which would lead to a double-free. Even more so
> because an API user might have noticed the leak, and concluded the ref
> must be released manually.
> 
> Since this looks like an unintentional bug and there's no release with
> it included yet, we can probably skip major jumps. But it should still
> be mentioned in APIchanges, and be sent to Libav too.

No: this part of the patch does nothing because it's already unreffed in the
avcodec_close() called immediately above.  Maybe the unref should actually be in
avcodec_free_context() only (if treating it like rc_override and those fields),
but it shouldn't be in both.


On 12/05/16 20:35, Andrey Turkin wrote:
> @@ -197,6 +198,7 @@ int avcodec_copy_context(AVCodecContext *dest, const
AVCodecContext *src)
>      av_freep(&dest->inter_matrix);
>      av_freep(&dest->extradata);
>      av_freep(&dest->subtitle_header);
> +    av_buffer_unref(&dest->hw_frames_ctx);
>
>      memcpy(dest, src, sizeof(*dest));
>      av_opt_copy(dest, src);
> @@ -225,6 +227,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      dest->inter_matrix    = NULL;
>      dest->rc_override     = NULL;
>      dest->subtitle_header = NULL;
> +    dest->hw_frames_ctx   = NULL;
>
>  #define alloc_and_copy_or_fail(obj, size, pad) \
>      if (src->obj && size > 0) { \
> @@ -245,6 +248,12 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      av_assert0(dest->subtitle_header_size == src->subtitle_header_size);
>  #undef alloc_and_copy_or_fail
>
> +    if (src->hw_frames_ctx) {
> +        dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);
> +        if (!dest->hw_frames_ctx)
> +            goto fail;
> +    }
> +
>      return 0;
>
>  fail:
> @@ -255,6 +264,7 @@ fail:
>      av_freep(&dest->subtitle_header);
>      dest->subtitle_header_size = 0;
>      dest->extradata_size = 0;
> +    av_buffer_unref(&dest->hw_frames_ctx);
>      av_opt_free(dest);
>      return AVERROR(ENOMEM);
>  }
>

The rest of the patch looks sensible to me.

- Mark