On 13/05/16 10:42, wm4 wrote:
> On Fri, 13 May 2016 10:54:17 +0300
> Andrey Turkin <andrey.tur...@gmail.com> wrote:
>
>> 2016-05-13 10:35 GMT+03:00 wm4 <nfx...@googlemail.com>:
>>
>>> On Thu, 12 May 2016 22:35:48 +0300
>>> Andrey Turkin <andrey.tur...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
