[FFmpeg-devel] [PATCH 3/3] avutil/opt: Preserve nb_channels in opt_free

[James Almer]

On 4/30/2024 9:38 PM, Michael Niedermayer wrote:
> On Tue, Apr 30, 2024 at 06:27:23PM -0300, James Almer wrote:
>> On 4/29/2024 9:48 PM, Michael Niedermayer wrote:
>>> Fixes: division by 0
>>> Fixes: decoder modifying demuxer channels on failure
>>> Fixes: -sseof -5 -i zgclab/ffmpeg_crash/poc3
>>>
>>> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>    libavutil/opt.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index ecbf7efe5fb..24c08e4bc06 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -132,9 +132,11 @@ static void opt_free_elem(const AVOption *o, void *ptr)
>>>            av_dict_free((AVDictionary **)ptr);
>>>            break;
>>> -    case AV_OPT_TYPE_CHLAYOUT:
>>> +    case AV_OPT_TYPE_CHLAYOUT: {
>>> +        int nb_channels = ((AVChannelLayout *)ptr)->nb_channels;
>>>            av_channel_layout_uninit((AVChannelLayout *)ptr);
>>> -        break;
>>> +        ((AVChannelLayout *)ptr)->nb_channels = nb_channels;
>>> +        break;}
>>>        default:
>>>            break;
>>
>> A little bit of context would be helpful here. What's using nb_channels
>> after av_opt_free was called and where?
> 
> demuxer sets nb_channels
> find stream info copies codec params to context
> find stream info tries opening decoder
> decoder, refuses, and opt_free_elem() is called on cleanup
> context now has 0 channels
> context gets copied into params of demuxer
> demuxer goes like i have set the channels to a non zero value let me devide by them
> and oops
> 
> there is more than one position in this chain of events this can be fixed
> 
> thx

I think we should prevent avcodec_open2() from clearing a user-set 
parameter when closing, rather than change how av_opt_free works.

Like so:

> 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);