[FFmpeg-devel] [PATCH 1/3] avcodec/avcodec: prevent ch_layout from being uninitialized in ff_codec_close()

[James Almer]

On 5/1/2024 5:26 PM, Andreas Rheinhardt wrote:
> James Almer:
>> It's a user-set parameter shared with AVCodecParameters, so it should only
>> be freed by avcodec_free_context().
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavcodec/avcodec.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>> index 888dd76228..fc8a40e4db 100644
>> --- a/libavcodec/avcodec.c
>> +++ b/libavcodec/avcodec.c
>> @@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>   
>>   av_cold void ff_codec_close(AVCodecContext *avctx)
>>   {
>> +    AVChannelLayout ch_layout;
>>       int i;
>>   
>>       if (!avctx)
>> @@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
>>   
>>       if (avctx->priv_data && avctx->codec && avctx->codec->priv_class)
>>           av_opt_free(avctx->priv_data);
>> +
>> +    // Work around av_opt_free() unsetting ch_layout
>> +    ch_layout = avctx->ch_layout;
>> +    memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout));
>>       av_opt_free(avctx);
>> +    avctx->ch_layout = ch_layout;
>> +
>>       av_freep(&avctx->priv_data);
>>       if (av_codec_is_encoder(avctx->codec)) {
>>           av_freep(&avctx->extradata);
> 
> This and the other patches will cause memleaks for users that use
> allocated channel layouts and avcodec_close()+av_free() (this is
> deprecated, not forbidden).

That's awful, but guess it needs to be supported until avcodec_close() 
is gone, so I'm withdrawing this patch.

> 
> Furthermore, where does the rule "user-set parameters shared with
> AVCodecParameters should only be freed by avcodec_free_context()" come
> from? It is news to me.

It's not a rule, it's the ideal/expected behavior seeing the crash 
Michael found, where the only shared field cleared during 
avcodec_close() was ch_layout because it may contain allocated data and 
can be set through an AVOption.
If you're copying params between codecpar and avctx, the latter should 
not have only one of the relevant fields nuked on an internal failure.