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

[Marton Balint]

On Mon, 21 Mar 2022, Marton Balint wrote:

>
>
> 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().

Sorry, I meant the first one.