[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 9:05 PM, Marton Balint wrote:
>>
>>
>>  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.
>
> avformat_free_context() calls avcodec_free_context() on the relevant 
> AVCodecContexts.

Sorry, I meant the second case, if you do the dance with 
AVCodecContext->ch_layout in avcodec_close() then 
avcodec_close()+av_freep(avctx) will leak ch_layout.map on custom layouts.

I hope I finally make sense :)

Regards,
Marton