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

[Marton Balint]

On Sun, 20 Mar 2022, James Almer wrote:

> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote:
>>  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
>
> Ok, do i add a codecpar copy like i suggested in 
> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? It 
> works, but it feels really weird doing that in what's the cleanup portion of 
> the function.
> Alternatively, add the dance from 
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ 
> which should have the same effect and never fail, unlike param copy.

The latter would also leak memory on avcodec_close()+av_freep().

Regards,
Marton