[FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[Andreas Rheinhardt]

James Almer:
> 
> 
> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> It can uninitialize fields that may still be used after the context
>>> was closed,
>>> so do it instead in avcodec_free_context().
>>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>   libavcodec/avcodec.c | 1 -
>>>   libavcodec/options.c | 2 +-
>>>   2 files changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>> index 38bdaad4fa..122d09b63a 100644
>>> --- a/libavcodec/avcodec.c
>>> +++ b/libavcodec/avcodec.c
>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx)
>>>         if (avctx->priv_data && avctx->codec &&
>>> avctx->codec->priv_class)
>>>           av_opt_free(avctx->priv_data);
>>> -    av_opt_free(avctx);
>>>       av_freep(&avctx->priv_data);
>>>       if (av_codec_is_encoder(avctx->codec)) {
>>>           av_freep(&avctx->extradata);
>>> diff --git a/libavcodec/options.c b/libavcodec/options.c
>>> index 33f11480a7..91335415c1 100644
>>> --- a/libavcodec/options.c
>>> +++ b/libavcodec/options.c
>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>>       av_freep(&avctx->intra_matrix);
>>>       av_freep(&avctx->inter_matrix);
>>>       av_freep(&avctx->rc_override);
>>> -    av_channel_layout_uninit(&avctx->ch_layout);
>>> +    av_opt_free(avctx);
>>>         av_freep(pavctx);
>>>   }
>>
>> This will lead to memleaks for users that use avcodec_close(avctx) +
>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded
>> encoders do this). Notice that avcodec_free_context() violates the
>> documentation of AVCodecContext.extradata (documented to not be freed
>> for decoders) and AVCodecContext.subtitle_header and
>> AVCodecContext.rc_override (documented to not be freed by lavc for
>> encoders), so there is a reason for using it instead of
>> avcodec_free_context() (even when not reusing the context).
> 
> That's an absolute mess of a situation. av_free(avctx) should not be an
> allowed or supported scenario when avcodec_free_context() exists. And
> why is the latter violating its own documentation?
> 

It is not violating its own documentation, but the documentation of the
relevant AVCodecContext fields. IIRC Anton wanted a function that just
frees the whole context, even if this meant that fields which are
documented as being owned by the user are freed. Even documenting the
current state of affairs in avcodec.h doesn't change the fact that there
is a valid reason to use avcodec_close()+av_free(), so we can't pretend
it doesn't happen.

- Andreas