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

[James Almer <jamrial@gmail.com>]

On 3/20/2022 9:21 PM, Marton Balint wrote:
> 
> 
> 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@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 :)

Oh, i meant to say doing that dance in lavf, in the same place the first 
option called param copy, instead of said param copy call. Sorry i 
wasn't explicit.

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".